Cost rasters and LTS from shapefiles

[abyrd]

This combines preliminary work from Q3 2021 and rebases it onto the current development branch. This branch will add the following features:

• Map matching a shapefile to OSM, using specified attribute of the shapefile as biking LTS
• Draping the road network over a raster and assigning costs from the resulting profiles along edges

If we store these to files, then this will require a change to the Network Version of the serialized TransportNetworks.

To facilitate testing and experimentation, these new types of data may be added with the following custom modification types. The source files may be uploaded via the Data Sources tab, and their IDs (copied from the URL) substituted in to the dataSourceId field.

{
  "name": "LTS from Shapefile",
  "r5type": "shapefile-lts",
  "dataSourceId": "61f277140e46af79fd6d5367",
  "ltsAttribute": "lts_ov"
}

{
  "name": "Elevation from GeoTIFF Raster",
  "r5type": "raster-cost",
  "costFunction": "TOBLER",
  "dataSourceId": "61f277140e46af79fd6d5367"
}

{
  "name": "Sun cost from GeoTIFF Raster",
  "r5type": "raster-cost",
  "costFunction": "SUN",
  "dataSourceId": "61f277140e46af79fd6d5367"
}

[abyrd]

A few observations on tile generation: when you zoom in, you can readily see the smoothness of the shapes improve as it fetches more detailed map tiles. This is intended behavior - lower zoom tiles are more simplified since they contain more features. The backend memory use increases while it's building tiles, but this seems ephemeral. After triggering a GC, I see very little ongoing memory impact even with spatial indexes built for 4 feeds. We'll want to check this out with some really big feeds like NL or CH.

[abyrd]

Now using JTS intersection operation to clip, and handling cases that produce MultiLineStrings. Cases like the DC blue line exiting and re-entering a tile, and the long straight segments of the LA metrolink lines converging on Union Station now display properly. This should be ready for final review and merge.

[trevorgerhardt]

This code is mostly a duplicate of the code in the BrokerController (in fact it was originally copied from there). Should we abstract this out as a helper method in the broker itself?

   /**
     * Get or start worker.
     */
    public synchronized String getOrStartWorker(WorkerCategory workerCategory, WorkerTags workerTags) {
        String address = getWorkerAddress(workerCategory);
        if (address == null) {
            // There are no workers that can handle this request. Request some.
            createOnDemandWorkerInCategory(workerCategory, workerTags);
        } else {
            // Workers exist in this category, clear out any record that we're waiting for one to start up.
            // FIXME the tracking of which workers are starting up should really be encapsulated using a "start up if needed" method.
            recentlyRequestedWorkers.remove(workerCategory);
        }
        return address;
    }

At the very least, we shouldn't leave the replicated FIXME in two different places.

[abyrd]

I definitely agree, this should be factored out into a shared method. In fact, as a best practice we should avoid duplicating this much code even when making a prototype because it then requires us to spot and re-merge the duplicates months down the line - we should factor out early.

[trevorgerhardt]

Should the GtfsVectorTileMaker be moved into the com.conveyal.gtfs package since it's not a controller?

[abyrd]

Summarizing discussion on the call today, we will be splitting all functionality of GtfsVectorTileMaker into GtfsCache and MapTile (renamed to VectorTile until we actually merge it with any similar raster tile classes). MapTile is not a utility class strictly speaking, and its use is currently limited to vector tile generation. We tentatively planned to create a com.conveyal.analysis.vectortile package to situate related code under the application layer (analysis here refers to the application built on top of r5 and gtfs-lib, not just the action of analyzing things).

[trevorgerhardt]

Should the shape and stop indexes be moved into the GTFSCache? If they shouldn't, should they at least be extracted into their own file in case they ever want to be used outside of the GtfsVectorTileMaker? Additionally, is there a better way to distinguish between the type of cache data they contain vs what is currently in the GTFSCache? Possibly renaming GTFSCache -> GtfsMapDbCache and extracting the shape and stop indexes into a GtfsGeoSpatialInMemoryCache? I don't think these names distinguish between the two well enough, but I wanted to give an example.

[abyrd]

Summarizing discussion on the call today: we will move these indexes into GTFSCache, but leave these spatial index caches as two top-level LoadingCaches within GTFSCache rather than making the spatial indexes fields on the GTFSFeed values managed by the GTFSCache. This is not because we necessarily consider this an ideal design long term, but because 1. it retains the current level of flexibility with respect to eviction policies, allowing the geometry caches to hold less objects and evict them sooner than the feed cache. This is important because we haven't used this in production yet and unlike the GTFSFeeds the spatial indexes have potentially unlimited memory usage. We want to observe memory usage in practice and be able to tune these parameters. 2. it's easier to reason about reference chains and loops when they're not nested, 3. even though lazy loading synchronization would be relatively simple as it would be per-GTFSFeed instance, it's nice to uniformly use the well-audited per-key locking of LoadingCache.

[trevorgerhardt]

Should this be moved into a private static final int TILE_EXTENT = 4096;?

[abyrd]

Yes, we'll add symbolic constants for all magic numbers. This is just left over from prototyping.

[trevorgerhardt]

Should this be moved into a private static final int TILE_SIZE = 256;?

[abyrd]

Yes, we'll add symbolic constants for all magic numbers. This is just left over from prototyping. These also need some comments explaining that they're the usual standard values, and what they do.

[trevorgerhardt]

Should we factor this out?

    private static Coordinate projectCoordinate (Coordinate c, Envelope envelope) {
        double px = ((c.x - envelope.getMinX()) * TILE_EXTENT) / envelope.getWidth();
        double py = ((envelope.getMaxY() - c.y) * TILE_EXTENT) / envelope.getHeight();
        return new Coordinate(px, py);
    }

[abyrd]

Yes, this can be factored out. I think the original code that clones a coordinate and sets its fields was the result of a series of refactors, starting from a version that tried to avoid new object creation. In the end we create a lot of small objects in the vector tile generation, which is not a big deal for contemporary garbage collectors.

[trevorgerhardt]

These appear to be hard coded testing IDs. Should they be removed?

[abyrd]

Yes, just left over from experiments and prototyping.

[trevorgerhardt]

Should clipScaleAndSimplify and common static values like LINE_SIMPLIFY_TOLERANCE, TILE_EXTENT, and TILE_SIZE be factored out into a TileUtils file?

[abyrd]

Summarizing discussion from today's call: they're not quite static utility methods, as the goal is to reuse the envelope and buffered envelope of a particular tile as fields on an object. So we want to factor them out, but to a concrete class in a vector tile-specific package.

[abyrd]

I merged in @trevorgerhardt's cleanup and added some more of my own based on our conversation and comments added to #796. Some minor changes that were not discussed on the call:

1. I reintroduced type information where it had been replaced with var declarations. This is a question of style, but var is not used at all in the rest of the code base and arguably makes type information more obscure in most cases. This is usually compensated for by IDEs which then sort of defeats the shortening effect of var: an IDE will infer the type and display it inline, so changing long x to var x ends up displaying var x: long to make sure you're getting the type you expect. If we change to using var this should probably be a consistent decision for the whole project.
2. I reintroduced some Javadoc that was deleted when code was moved from one file to another. This was not the most well-composed documentation but did take some effort to pull together and could be very useful to someone approaching vector tile generation in the future. I cleaned it up a bit when I added it back.
3. I pulled out some method calls that had been inlined as parameters in another method call. In optimized JVM operation this should have no effect on performance, but it does help when debugging to see the values before they're passed into the method, and it can be easier to read to have symbolic names in the parameter positions.

[trevorgerhardt]

Does this comment need to be addressed? At the very least, is it clear in the documentation provided to the clients that use this feature?

[abyrd]

This may be properly handled because affectsStreetLayer() returns true, but I’d like to retrace how that works to make sure it handles this case. Of course additional documentation will be in order if egress is not always affected by modifications.