Introduce a common interface for geometric features to reduce unnecessary allocations
@mourner is already working on this.
Since Mar 24, 2021.
- Dominant language
- TypeScript
- Stars
- 12.4k
- Forks
- 2.4k
- PR merge metrics
- No merged PRs in 30d
Description
The way GL JS represents geometry data in the code is highly inconsistent, where different parts of the code both produce and consume it in subtly different formats. This leads to a lot of superflous JS allocations as we need to create intermediary transformations of data for everything to work together.
- `vector-tile-js` outputs geometries as `Array>` (where `Point` is essentially an `{x, y}` object), regardless of geometry type. This leads to point and line features using more arrays than necessary, and no distinction between polygons with holes and multipolygons — the latter requiring explicit ring classification code that produces yet more wrapping arrays.
- This ring classification code is present in two versions — `mapbox-gl-js` (classifying before triangulation for fill, fill-extrusion & symbol layers), and `vector-tile-js` for the `toGeoJSON` routine.
- `geojson-vt` and `supercluster` produce a different format with `[[x, y], [x, y], ...]` nested arrays. To make it suitable for consumption in code that expects `vector-tile-js`-like format, there's a `GeoJSONWrapper` utility that builds the necessary feature class hierarchy around each `geojson-vt` feature, and additionally rebuilds all the geometry into the `Array` format. In the process, it looses the polygon classification information, which has to be reconstructed again with the `classifyRings` routine, adding even more unnecessary processing. Additionally, the wrapping code transforms these geometries twice — once for feeding into the bucket code in the worker, and the second time for feeding into `vt-pbf` to encode into a vector tile and send back to the main thread for feature querying. Unnecessary transformations of geometry here reach ridiculous levels.
- Both temporary representations (either `[x, y]` or `{x, y}` points) use way more memory than a flat representation (`[x, y, x, y]`) could, introducing a lot of GC churn.
- Additionally, weakly defined & inconsistent geometry format makes designing a potential custom sources API difficult.
Previous discussions around this (internally at https://github.com/mapbox/gl-internal/issues/786) stalled because the proposals there were too ambitious and widely scoped — introducing a flexible format that covers a ton of use cases, supports streaming APIs etc. We could get most of the benefits with a much simpler approach and less effort. Here's my proposal:
- [ ] Adopt the "flat geometries" format, similar to the one used internally in geojson-vt (https://github.com/mapbox/geojson-vt/pull/90), clearly defining it in one place. Lines and polygon rings would be represented as arrays of numbers (`[x, y, x, y]`), optionally typed (e.g `Int16Array` when coming from `vector-tile-js`). At the same time, nesting would follow GeoJSON format and be dependent on geometry type (`Array` for `Point`, `MultiPoint` and `LineString`; `Array` for `Polygon` and `MultiLineString`; `Array>` for `MultiPolygon`).
- [ ] Switch `vector-tile-js` to output the new format, classifying rings once internally.
- [ ] Switch `geojson-vt` to the new format.
- [ ] Switch `supercluster` to the new format.
- [ ] Switch `vt-pbf` to consume the new format, ditching its two possible input formats in favor of a new one.
- [ ] Switch `mapbox-gl-js` to consume the new format, removing the need for `GeoJSONWrapper` and explicit ring classification. This would involve rewriting `bucket` logic to operate on flat arrays instead of relying on `Point` objects with methods like `unit`, and be the most time-consuming part of the push.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Assessment
This issue has not been assessed yet.