Factor out common vector tile code #796
Conversation
This change factors out common code across the `GtfsVectorTileMaker` and the `NetworkTileController`. It moves manny common functions into the `MapTile` util. This is because many operations are dependent on the `envelope` that is produced by that util. Additionally, the remaining functionality of the `GtfsVectorTileMaker` was eseentially boiled down to just creating and querying the caches.
Since the cache can be used for anything, not just vector tiles. Also, factors out the "GeometryCache" part.
Creates a buffered envelope that can be re-used while clipping. Also, add comments.
| @@ -39,11 +41,15 @@ | |||
| * A basic example client for browsing the tiles is at src/main/resources/vector-client | |||
| */ | |||
| public class GtfsController implements HttpController { | |||
There was a problem hiding this comment.
We should rename this to something more specific like GtfsVectorTileController, especially since we actually serve up the contents of GTFS feeds on other endpoints.
| private final GTFSCache gtfsCache; | ||
| private final GtfsVectorTileMaker gtfsVectorTileMaker; | ||
| private final GtfsGeometryCache gtfsGeometryCache; |
There was a problem hiding this comment.
I would probably want to rename this to something like GtfsSpatialIndexCache but we decided to merge it into GtfsCache so won't need to decide on a new name.
| final int x = Integer.parseInt(req.params("x")); | ||
| final int y = Integer.parseInt(req.params("y")); | ||
| byte[] pbfMessage = this.gtfsVectorTileMaker.getTile(bundleScopedFeedId, z, x, y); | ||
| MapTile tile = new MapTile(Integer.parseInt(req.params("z")), Integer.parseInt(req.params("x")), Integer.parseInt(req.params("y"))); |
There was a problem hiding this comment.
Embedding these method calls as parameters in another method call can degrade debugging experience. I often create variables like this just for readability and debugging (if you pause on the MapTile constructor call, you can very easily see the z/x/y parameter values). The human eye is really good at seeing repeated patterns in stacked lines - stacking these vertically makes it apparent that the exact same thing is happening three times with different parameter names. The JVM should optimize all of this out anyway in non-debug operation, and even if it doesn't this does not execute frequently enough to be a performance issue. Alternate point of view is that adding identifiers creates more places to make a typing mistake, but I actually think the redundancy makes this more readable and maintainable.
| final int y = Integer.parseInt(req.params("y")); | ||
| byte[] pbfMessage = this.gtfsVectorTileMaker.getTile(bundleScopedFeedId, z, x, y); | ||
| MapTile tile = new MapTile(Integer.parseInt(req.params("z")), Integer.parseInt(req.params("x")), Integer.parseInt(req.params("y"))); | ||
| var patterns = tile.clipAndSimplifyLinesToTile( |
There was a problem hiding this comment.
These identifiers could be more specific - they're not just the patterns and stops, but maybe the patternGeometries and stopGeometries.
| * than this many tile units. These are minuscule at 1/4096 of the tile width or height. | ||
| */ | ||
| private static final int LINE_SIMPLIFY_TOLERANCE = 5; | ||
| // This must match UI code. |
There was a problem hiding this comment.
Comment can be expanded to make it clear what it matches in the UI code.
| import java.util.List; | ||
|
|
||
| /** | ||
| * Utilities for creating a slippy map tile and generating it's enveloper. Also for encapsulating it's properties that |
There was a problem hiding this comment.
Javadoc should be updated to reflect the fact that this is now vector tile specific, but could eventually be combined with raster tile uses, or be made a subclass of a general tile (not raster or vector specific). It's kind of surprising that we define tile2lat() here and apparently nowhere else in the project - maybe we really don't deal with any raster tiles in the Java code.
| */ | ||
| private static final int LINE_SIMPLIFY_TOLERANCE = 5; | ||
|
|
||
| // Add a buffer to the tile envelope to make sure any artifacts are outside it. |
There was a problem hiding this comment.
I will expand this comment a bit.
| /** | ||
| * Cache sets of geometry in a JTS STRtree using a Caffeine LoadingCache. | ||
| */ | ||
| public class GeometryCache<T extends Geometry> { |
There was a problem hiding this comment.
Should be renamed - it caches spatial indexes, not just geometries. Summarizing discussion: this may be "premature abstraction" (not optimization) since long term we're not certain to maintain two separate spatial index caches. Most reasons that would lead us to maintain two or more of them would also mean they potentially benefit from having different expiration and size eviction parameters. These hard-wired constants are not really constant across all potential spatial index caches, so this doesn't quite match the implied level of abstraction.
It's also not clear whether factoring this code out makes it easier or harder to read. If we retain this abstraction I'll update the Javadoc to include the above comments.
| } | ||
|
|
||
| /** | ||
| * Find a List of all geometry within a given envelope. |
There was a problem hiding this comment.
Update comment to clarify that this overselects, and finds things that intersect the envelope (rather than contained within the envelope).
| import java.util.Objects; | ||
|
|
||
| /** | ||
| * This class maintains spatial indexes of data from GTFS feeds. |
There was a problem hiding this comment.
It's important that this indexes only geometries for display from the GTFS feeds and does not attempt to provide full indexing for everything available in the GTFS API. Will update Javadoc to clarify.
This change factors out common code across the
GtfsVectorTileMakerand theNetworkTileController. It moves many common functions into theMapTileutil. This was done because many operations are dependent on theenvelopethat is produced by that util. Additionally, the remaining functionality of theGtfsVectorTileMakerwas boiled down to just creating and querying the caches, so I renamed itGtfsGeometryCacheand moved it into thecom.conveyal.gtfspackage.Note: the naming could certainly be improved. Suggestions welcome.