search

conveyal / r5

Developed to power Conveyal's web-based interface for scenario planning and land-use/transport accessibility analysis, R5 is our routing engine for multimodal (transit/bike/walk/car) networks with a particular focus on public transit
https://conveyal.com/learn
MIT License
272 stars 71 forks source link

Compose with WebMercatorExtents to reduce duplication #900

Closed abyrd closed 8 months ago

abyrd commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

@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 commented 8 months ago

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