Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compose with WebMercatorExtents to reduce duplication #900

Merged
merged 7 commits into from
Nov 2, 2023

Conversation

abyrd
Copy link
Member

@abyrd abyrd commented Oct 19, 2023

WebMercatorGridPointSet contained a lot of duplicated fields and logic from WebMercatorExtents.
Objects constructed with WebMercatorGridPointSet constructor were often one unit too narrow (not consistent with WebMercatorExtents).

With this PR, WebMercatorExtents becomes Serializable and will be stored in TransportNetwork files so this requires a network version bump and should ideally be included in a release version with other such changes.

@abyrd abyrd force-pushed the merge-extents-with-pointset branch from d0f0f9c to a095984 Compare October 19, 2023 17:08
@abyrd abyrd added the nv-update Implies changes to the serialized network (either format or logical content changes) label Oct 19, 2023
@abyrd abyrd force-pushed the merge-extents-with-pointset branch from a095984 to b5a4cdb Compare October 19, 2023 19:55
Height and width in WebMercatorGridPointSet constructor were often one
unit too narrow (not consistent with WebMercatorExtents).
@abyrd abyrd force-pushed the merge-extents-with-pointset branch from b5a4cdb to fad70b3 Compare October 19, 2023 20:04
trevorgerhardt
trevorgerhardt previously approved these changes Oct 28, 2023
Copy link
Member

@trevorgerhardt trevorgerhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This refactor looks good to me 👍

Base automatically changed from merge-pixel-functions to dev November 2, 2023 09:11
@abyrd abyrd dismissed trevorgerhardt’s stale review November 2, 2023 09:11

The base branch was changed.

@abyrd abyrd marked this pull request as ready for review November 2, 2023 09:13
@abyrd
Copy link
Member Author

abyrd commented Nov 2, 2023

The main reason I left this as a draft was uncertainty around whether there were serialized objects lying around that would become incompatible. Because changing WebMercatorGridPointSet to have a nested WebMercatorExtents required making the latter serializable, we've clearly got old WebMercatorGridPointSets around that we need to deserialize without conflicts. I believe that's only in the serialized network files, which would only be read by the workers, which understand the idea of versioned network files. I just need to make sure this isn't used anywhere else before merging it.

@abyrd
Copy link
Member Author

abyrd commented Nov 2, 2023

Some serialization systems (e.g. for serializing to JSON) will not only serialize fields but also public getter methods. The result of WebMercatorExtents.getMercatorEnvelopeMeters could be serialized in this case. But I believe we're only serializing this class with Kryo, whose default serializer writes out only the fields of the class. For standard Java serialization the serialization version ID is important but messy to manually maintain. We should only be serializing this class with Kryo, not other systems, and I think that's currently the case.

@abyrd
Copy link
Member Author

abyrd commented Nov 2, 2023

As I was reviewing this I noticed another obvious place where WebMercatorExtents were being copied and compared field by field. I added a commit that updates WebMercatorGridPointSetCache.GridKey in a similar way, composing with WebMercatorExtents.

Following up on my comment above, I don't see anywhere WebMercatorGridPointSet is being stored except in TransportNetworks so I think this PR will be sufficiently backward compatible.

@abyrd abyrd enabled auto-merge November 2, 2023 11:18
@abyrd
Copy link
Member Author

abyrd commented Nov 2, 2023

@trevorgerhardt or @ansoncfit please give this one another look to verify the semantic equality logic in WebMercatorGridPointSetCache and then feel free to approve/merge.

@abyrd
Copy link
Member Author

abyrd commented Nov 2, 2023

Build is now passing (code now compiling with changed symbolic constant). I think this is ready for final review.

@abyrd abyrd merged commit 0052049 into dev Nov 2, 2023
3 checks passed
@abyrd abyrd deleted the merge-extents-with-pointset branch November 2, 2023 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
nv-update Implies changes to the serialized network (either format or logical content changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants