Compose with WebMercatorExtents to reduce duplication

src/main/java/com/conveyal/analysis/controllers/DataSourceController.java

@@ -35,7 +35,7 @@
 import static com.conveyal.analysis.util.JsonUtil.toJson;
 import static com.conveyal.file.FileCategory.DATASOURCES;
 import static com.conveyal.file.FileStorageFormat.SHP;
-import static com.conveyal.r5.analyst.WebMercatorGridPointSet.parseZoom;
+import static com.conveyal.r5.analyst.WebMercatorExtents.parseZoom;
 import static com.mongodb.client.model.Filters.eq;
 
 /**

src/main/java/com/conveyal/analysis/controllers/OpportunityDatasetController.java

@@ -59,7 +59,7 @@
 import static com.conveyal.analysis.datasource.DataSourceUtil.detectUploadFormatAndValidate;
 import static com.conveyal.analysis.util.JsonUtil.toJson;
 import static com.conveyal.file.FileCategory.GRIDS;
-import static com.conveyal.r5.analyst.WebMercatorGridPointSet.parseZoom;
+import static com.conveyal.r5.analyst.WebMercatorExtents.parseZoom;
 import static com.conveyal.r5.analyst.progress.WorkProductType.OPPORTUNITY_DATASET;
 
 /**

src/main/java/com/conveyal/analysis/datasource/derivation/AggregationAreaDerivation.java

@@ -36,7 +36,7 @@
 import java.util.zip.GZIPOutputStream;
 
 import static com.conveyal.file.FileStorageFormat.SHP;
-import static com.conveyal.r5.analyst.WebMercatorGridPointSet.parseZoom;
+import static com.conveyal.r5.analyst.WebMercatorExtents.parseZoom;
 import static com.conveyal.r5.analyst.progress.WorkProductType.AGGREGATION_AREA;
 import static com.conveyal.r5.util.ShapefileReader.GeometryType.POLYGON;
 import static com.google.common.base.Preconditions.checkArgument;

src/main/java/com/conveyal/r5/analyst/GridTransformWrapper.java

@@ -37,8 +37,8 @@ public GridTransformWrapper (WebMercatorExtents targetGridExtents, Grid sourceGr
     // This could certainly be made more efficient (but complex) by forcing sequential iteration over opportunity counts
     // and disallowing random access, using a new PointSetIterator class that allows reading lat, lon, and counts.
     private int transformIndex (int i) {
-        final int x = (i % targetGrid.width) + targetGrid.west - sourceGrid.extents.west;
-        final int y = (i / targetGrid.width) + targetGrid.north - sourceGrid.extents.north;
+        final int x = (i % targetGrid.extents.width) + targetGrid.extents.west - sourceGrid.extents.west;
+        final int y = (i / targetGrid.extents.width) + targetGrid.extents.north - sourceGrid.extents.north;
         if (x < 0 || x >= sourceGrid.extents.width || y < 0 || y >= sourceGrid.extents.height) {
             // Point in target grid lies outside source grid, there is no valid index. Return special value.
             return -1;

src/main/java/com/conveyal/r5/analyst/LinkageCache.java

@@ -95,7 +95,7 @@ public LinkedPointSet load(Key key) {
                             key.streetLayer,
                             key.streetMode
                     );
-                    if (basePointSetLinkage != null && basePointSet.zoom == keyPointSet.zoom) {
+                    if (basePointSetLinkage != null && basePointSet.extents.zoom == keyPointSet.extents.zoom) {
                         LOG.info("Cutting linkage for {} out of existing linkage for {}.", keyPointSet, basePointSet);
                         return new LinkedPointSet(basePointSetLinkage, keyPointSet);
                     }

src/main/java/com/conveyal/r5/analyst/WebMercatorExtents.java

@@ -8,6 +8,7 @@
 import org.locationtech.jts.geom.Envelope;
 import org.opengis.referencing.crs.CoordinateReferenceSystem;
 
+import java.io.Serializable;
 import java.util.Arrays;
 
 import static com.conveyal.r5.analyst.Grid.latToFractionalPixel;
@@ -29,10 +30,15 @@
  *      and OpportunityGrid (AKA Grid) which adds opportunity counts. These can compose, not necessarily subclass.
  *      Of course they could all be one class, with the opportunity grid nulled out when there is no density.
  */
-public class WebMercatorExtents {
+public class WebMercatorExtents implements Serializable {
 
-    private static final int MIN_ZOOM = 9;
-    private static final int MAX_ZOOM = 12;
+    /**
+     * Default Web Mercator zoom level for grids (origin/destination layers, aggregation area masks, etc.).
+     * Level 10 is probably ideal but will quadruple calculation relative to 9.
+     */
+    public static final int DEFAULT_ZOOM = 9;
+    public static final int MIN_ZOOM = 9;
+    public static final int MAX_ZOOM = 12;
     private static final int MAX_GRID_CELLS = 5_000_000;
 
     /** The pixel number of the westernmost pixel (smallest x value). */
@@ -98,6 +104,12 @@ public static WebMercatorExtents forPointsets (PointSet[] pointSets) {
         }
     }
 
+    public static int parseZoom(String zoomString) {
+        int zoom = (zoomString == null) ? DEFAULT_ZOOM : Integer.parseInt(zoomString);
+        checkArgument(zoom >= MIN_ZOOM && zoom <= MAX_ZOOM);
+        return zoom;
+    }
+
     /**
      * Create the minimum-size immutable WebMercatorExtents containing both this one and the other one.
      * Note that WebMercatorExtents fields are immutable, and this method does not modify the instance in place.

src/main/java/com/conveyal/r5/analyst/WebMercatorGridPointSet.java

@@ -27,49 +27,27 @@ public class WebMercatorGridPointSet extends PointSet implements Serializable {
 
     public static final Logger LOG = LoggerFactory.getLogger(WebMercatorGridPointSet.class);
 
-    /**
-     * Default Web Mercator zoom level for grids (origin/destination layers, aggregation area masks, etc.).
-     * Level 10 is probably ideal but will quadruple calculation relative to 9.
-     */
-    public static final int DEFAULT_ZOOM = 9;
-
-    public static final int MIN_ZOOM = 9;
-
-    public static final int MAX_ZOOM = 12;
-
-    /** web mercator zoom level */
-    public final int zoom;
-
-    /** westernmost pixel  */
-    public final int west;
-
-    /** northernmost pixel */
-    public final int north;
-
-    /** The number of pixels across this grid in the east-west direction. */
-    public final int width;
-
-    /** The number of pixels from top to bottom of this grid in the north-south direction. */
-    public final int height;
+    public final WebMercatorExtents extents;
 
     /** Base pointset; linkages will be shared with this pointset */
     public final WebMercatorGridPointSet basePointSet;
 
     /**
      * Create a new WebMercatorGridPointSet.
-     *
      * @oaram basePointSet the super-grid pointset from which linkages will be copied or shared, or null if no
      *        such grid exists.
      */
-    public WebMercatorGridPointSet(int zoom, int west, int north, int width, int height, WebMercatorGridPointSet basePointSet) {
-        this.zoom = zoom;
-        this.west = west;
-        this.north = north;
-        this.width = width;
-        this.height = height;
+    public WebMercatorGridPointSet(WebMercatorExtents extents, WebMercatorGridPointSet basePointSet) {
+        // All fields of extents are final, no need to make a protective copy.
+        this.extents = extents;
         this.basePointSet = basePointSet;
     }
 
+    /** The resulting PointSet will not have a null basePointSet, so should generally not be used for linking. */
+    public WebMercatorGridPointSet (WebMercatorExtents extents) {
+        this(extents, null);
+    }
+
     /**
      * Constructs a grid point set that covers the entire extent of the supplied transport network's street network.
      * This usually serves as the base supergrid pointset for other smaller grids in the same region.
@@ -80,31 +58,20 @@ public WebMercatorGridPointSet (TransportNetwork transportNetwork) {
 
     /**
      * TODO specific data types for Web Mercator and WGS84 floating point envelopes
-     * @param wgsEnvelope an envelope in floating-point WGS84 degrees
+     * @param wgsEnvelope an envelope in floating-point WGS84 degrees. The envelope will be expanded ever so slightly
+     *                    to ensure any objects within it are sure to fall within the resulting mercator extents, even
+     *                    if subject to loss of precision during later coordinate calculations.
      */
     public WebMercatorGridPointSet (Envelope wgsEnvelope) {
         LOG.info("Creating WebMercatorGridPointSet with WGS84 extents {}", wgsEnvelope);
         checkWgsEnvelopeSize(wgsEnvelope, "grid point set");
-        this.zoom = DEFAULT_ZOOM;
-        int west = lonToPixel(wgsEnvelope.getMinX(), zoom);
-        int east = lonToPixel(wgsEnvelope.getMaxX(), zoom);
-        int north = latToPixel(wgsEnvelope.getMaxY(), zoom);
-        int south = latToPixel(wgsEnvelope.getMinY(), zoom);
-        this.west = west;
-        this.north = north;
-        this.height = south - north;
-        this.width = east - west;
+        this.extents = WebMercatorExtents.forBufferedWgsEnvelope(wgsEnvelope, WebMercatorExtents.DEFAULT_ZOOM);
         this.basePointSet = null;
     }
 
-    /** The resulting PointSet will not have a null basePointSet, so should generally not be used for linking. */
-    public WebMercatorGridPointSet (WebMercatorExtents extents) {
-        this(extents.zoom, extents.west, extents.north, extents.width, extents.height, null);
-    }
-
     @Override
     public int featureCount() {
-        return height * width;
+        return extents.height * extents.width;
     }
 
     @Override
@@ -115,38 +82,40 @@ public double sumTotalOpportunities () {
 
     @Override
     public double getLat(int i) {
-        long y = i / this.width + this.north;
-        return pixelToLat(y, zoom);
+        long y = i / extents.width + extents.north;
+        return pixelToLat(y, extents.zoom);
     }
 
     @Override
     public double getLon(int i) {
-        long x = i % this.width + this.west;
-        return pixelToLon(x, zoom);
+        long x = i % extents.width + extents.west;
+        return pixelToLon(x, extents.zoom);
     }
 
     @Override
     public TIntList getPointsInEnvelope (Envelope envelopeFixedDegrees) {
         // Convert fixed-degree envelope to floating, then to world-scale web Mercator pixels at this grid's zoom level.
         // This is not very DRY since we do something very similar in the constructor and elsewhere.
-        int west = lonToPixel(fixedDegreesToFloating(envelopeFixedDegrees.getMinX()), zoom);
-        int east = lonToPixel(fixedDegreesToFloating(envelopeFixedDegrees.getMaxX()), zoom);
-        int north = latToPixel(fixedDegreesToFloating(envelopeFixedDegrees.getMaxY()), zoom);
-        int south = latToPixel(fixedDegreesToFloating(envelopeFixedDegrees.getMinY()), zoom);
+        // These are the integer pixel numbers containing the envelope, so iteration below must be inclusive of the max.
+        final int z = extents.zoom;
+        int west = lonToPixel(fixedDegreesToFloating(envelopeFixedDegrees.getMinX()), z);
+        int east = lonToPixel(fixedDegreesToFloating(envelopeFixedDegrees.getMaxX()), z);
+        int north = latToPixel(fixedDegreesToFloating(envelopeFixedDegrees.getMaxY()), z);
+        int south = latToPixel(fixedDegreesToFloating(envelopeFixedDegrees.getMinY()), z);
         // Make the envelope's pixel values relative to the edges of this WebMercatorGridPointSet, rather than
         // absolute world-scale coordinates at this zoom level.
-        west -= this.west;
-        east -= this.west;
-        north -= this.north;
-        south -= this.north;
+        west -= extents.west;
+        east -= extents.west;
+        north -= extents.north;
+        south -= extents.north;
         TIntList pointsInEnvelope = new TIntArrayList();
         // Pixels are truncated toward zero, and coords increase toward East and South in web Mercator, so <= south/east.
         for (int y = north; y <= south; y++) {
-            if (y < 0 || y >= this.height) continue;
+            if (y < 0 || y >= extents.height) continue;
             for (int x = west; x <= east; x++) {
-                if (x < 0 || x >= this.width) continue;
+                if (x < 0 || x >= extents.width) continue;
                 // Calculate the 1D (flattened) index into this pointset for the grid cell at (x,y).
-                int pointIndex = y * this.width + x;
+                int pointIndex = y * extents.width + x;
                 pointsInEnvelope.add(pointIndex);
             }
         }
@@ -162,8 +131,8 @@ public double getOpportunityCount (int i) {
     //@Override
     // TODO add this to the PointSet interface
     public String getPointId (int i) {
-        int y = i / this.width;
-        int x = i % this.width;
+        int y = i / extents.width;
+        int x = i % extents.width;
         return x + "," + y;
     }
 
@@ -172,14 +141,17 @@ public int getPointIndexContaining (Coordinate latLon) {
     }
 
     public int getPointIndexContaining (double lon, double lat) {
-        int x = lonToPixel(lon, zoom) - west;
-        int y = latToPixel(lat, zoom) - north;
-        return y * width + x;
+        int x = lonToPixel(lon, extents.zoom) - extents.west;
+        int y = latToPixel(lat, extents.zoom) - extents.north;
+        return y * extents.width + x;
     }
 
     @Override
-    public String toString () {
-        return "WebMercatorGridPointSet{" + "zoom=" + zoom + ", west=" + west + ", north=" + north + ", width=" + width + ", height=" + height + ", basePointSet=" + basePointSet + '}';
+    public String toString() {
+        return "WebMercatorGridPointSet{" +
+                "extents=" + extents +
+                ", basePointSet=" + basePointSet +
+                '}';
     }
 
     @Override
@@ -189,13 +161,8 @@ public Envelope getWgsEnvelope () {
 
     @Override
     public WebMercatorExtents getWebMercatorExtents () {
-        return new WebMercatorExtents(west, north, width, height, zoom);
-    }
-
-    public static int parseZoom(String zoomString) {
-        int zoom = (zoomString == null) ? DEFAULT_ZOOM : Integer.parseInt(zoomString);
-        checkArgument(zoom >= MIN_ZOOM && zoom <= MAX_ZOOM);
-        return zoom;
+        // WebMercatorExtents are immutable so we can return this without making a protective copy.
+        return extents;
     }
 
 }

src/main/java/com/conveyal/r5/analyst/WebMercatorGridPointSetCache.java

@@ -16,6 +16,7 @@
  * subsequent tasks.
  *
  * Alternatively PointSets could have semantic equality, but current design is that just the keys and extents do.
+ * Freeform pointsets can be quite big so comparing them semantically is expected to be inefficient in the worst case.
  *
  * This cache is not serialized anywhere. For now it's created fresh when the worker starts up and held in a
  * static field for the life of the worker. It is primed upon receiving the first request.
@@ -24,52 +25,41 @@ public class WebMercatorGridPointSetCache {
 
     private Map<GridKey, WebMercatorGridPointSet> cache = new ConcurrentHashMap<>();
 
-    public WebMercatorGridPointSet get (int zoom, int west, int north, int width, int height, WebMercatorGridPointSet base) {
-        GridKey key = new GridKey();
-        key.zoom = zoom;
-        key.west = west;
-        key.north = north;
-        key.width = width;
-        key.height = height;
-        key.base = base;
-
+    public WebMercatorGridPointSet get(WebMercatorExtents extents, WebMercatorGridPointSet base) {
+        GridKey key = new GridKey(extents, base);
         // This works even in a multithreaded environment; ConcurrentHashMap's contract specifies that this will
         // be called at most once per key. So ConcurrentHashMap could probably replace a lot of our LoadingCaches.
         return cache.computeIfAbsent(key, GridKey::toPointset);
     }
 
-    public WebMercatorGridPointSet get(WebMercatorExtents extents, WebMercatorGridPointSet base) {
-        return get(extents.zoom, extents.west, extents.north, extents.width, extents.height, base);
-    }
-
     /**
-     * TODO make this GridKey a subclass of WebMercatorGridExtents or compose them.
+     * This is essentially a WebMercatorGridPointSet without the PointSet implementation methods and with semantic
+     * equality on the extents, for looking up single WebMercatorGridPointSet instances (which like all PointSets
+     * have identity equality).
      */
     private static class GridKey {
-        public int zoom;
-        public int west;
-        public int north;
-        public int width;
-        public int height;
-        public WebMercatorGridPointSet base;
+
+        public final WebMercatorExtents extents;
+        public final WebMercatorGridPointSet base;
+
+        public GridKey(WebMercatorExtents extents, WebMercatorGridPointSet base) {
+            // All fields of extents are final, no need to make a protective copy.
+            this.extents = extents;
+            this.base = base;
+        }
 
         public WebMercatorGridPointSet toPointset () {
-            return new WebMercatorGridPointSet(zoom, west, north, width, height, base);
+            return new WebMercatorGridPointSet(extents, base);
         }
 
         public int hashCode () {
-            return west + north * 31 + width * 63 + height * 99 + (base == null ? 0 : base.hashCode() * 123);
+            return extents.hashCode() + (base == null ? 0 : base.hashCode() * 123);
         }
 
         public boolean equals (Object o) {
             if (o instanceof GridKey) {
                 GridKey other = (GridKey) o;
-                return zoom == other.zoom &&
-                        west == other.west &&
-                        north == other.north &&
-                        width == other.width &&
-                        height == other.height &&
-                        base == other.base;
+                return extents.equals(other.extents) && (base == other.base);
             } else {
                 return false;
             }

src/main/java/com/conveyal/r5/kryo/KryoNetworkSerializer.java

@@ -46,7 +46,8 @@ public abstract class KryoNetworkSerializer {
      * We considered using an ISO date string as the version but that could get confusing when seen in filenames.
      *
      * History of Network Version (NV) changes:
-     * nv3 use Kryo 5 serialization format
+     * nv4 2023-11-02 WebMercatorGridPointSet now contains nested WebMercatorExtents
+     * nv3 2023-18-01 use Kryo 5 serialization format
      * nv2 2022-04-05
      * nv1 2021-04-30 stopped using r5 version string (which caused networks to be rebuilt for every new r5 version)
      */

src/main/java/com/conveyal/r5/streets/EgressCostTable.java

@@ -377,15 +377,15 @@ public static EgressCostTable geographicallyCroppedCopy (LinkedPointSet subLinka
                         int targetInSuperLinkage = distanceTable[i];
                         int distance = distanceTable[i + 1];
 
-                        int superX = targetInSuperLinkage % superGrid.width;
-                        int superY = targetInSuperLinkage / superGrid.width;
+                        int superX = targetInSuperLinkage % superGrid.extents.width;
+                        int superY = targetInSuperLinkage / superGrid.extents.width;
 
-                        int subX = superX + superGrid.west - subGrid.west;
-                        int subY = superY + superGrid.north - subGrid.north;
+                        int subX = superX + superGrid.extents.west - subGrid.extents.west;
+                        int subY = superY + superGrid.extents.north - subGrid.extents.north;
 
                         // Only retain distance information for points that fall within this sub-grid.
-                        if (subX >= 0 && subX < subGrid.width && subY >= 0 && subY < subGrid.height) {
-                            int targetInSubLinkage = subY * subGrid.width + subX;
+                        if (subX >= 0 && subX < subGrid.extents.width && subY >= 0 && subY < subGrid.extents.height) {
+                            int targetInSubLinkage = subY * subGrid.extents.width + subX;
                             newDistanceTable.add(targetInSubLinkage);
                             newDistanceTable.add(distance); // distance to target does not change when we crop the pointset
                         }