Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[projections] Adaptive geometry resampling for alternative projections #10753

Merged
merged 2 commits into from
Jun 8, 2021

Conversation

mourner
Copy link
Member

@mourner mourner commented Jun 7, 2021

Closes #10756. Implements adaptive geometry resampling when reprojecting lines and polygons into non-mercator projections, so that segments that were straight in Mercator but become curved when projected have the least amount of additional points to represent the geometry accurately.

In the demo, for a low-zoom Albers USA view, we would generate ~3 times the amount of original points. With this code, it adds under 3% instead, while not impacting anything visually.

The logic is similar to Douglas-Peucker simplification but works in reverse — if a midpoint of a curve is far enough from the midpoint between two curve endpoints, we add a point in the middle and recurse to two halves. The resulting code is relatively compact and seems pretty fast too, although we might want to eliminate recursion some time later to make sure that it never hits maximum call stack error.

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • post benchmark scores
  • manually test the debug page

@mourner mourner added the skip changelog Used for PRs that do not need a changelog entry label Jun 7, 2021
@mourner mourner changed the title Adaptive geometry resampling for alternative projections [projections] Adaptive geometry resampling for alternative projections Jun 7, 2021
Copy link
Contributor

@ryanhamley ryanhamley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks really good. I tried integrating it with my branch and Mercator and other projections work as expected. I made a few suggestions but this is great.

Comment on lines 55 to 65
function reproject(p) {
const lng = lngFromMercatorX((canonical.x + p.x / featureExtent) / z2);
const lat = latFromMercatorY((canonical.y + p.y / featureExtent) / z2);
const {x, y} = projection.project(lng, lat);
return new Point(
(x * cs.scale - cs.x) * EXTENT,
(y * cs.scale - cs.y) * EXTENT
);
}

function addResampled(resampled, startMerc, endMerc, startProj, endProj) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two functions don't need to be declared in the loadGeometry method

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was initially a closure because it captures canonical, featureExtent and cs (tile transform), all of which would have to be moved to arguments in a quite a verbose way if we move it out. We could do it but it's also not a bottleneck.

if (i === 0 || feature.type === 1) {
resampled.push(clampPoint(pointProj));
} else {
addResampled(resampled, prevMerc, pointMerc, prevProj, pointProj);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flow doesn't like this because it thinks that addResampled can be called with prevMerc and prevProj as undefined variables. I added if (!startMerc || !startProj) return; at the start of addResampled to get around that but maybe there's a better way.

@@ -48,40 +47,57 @@ function resample(ring) {
*/
export default function loadGeometry(feature: VectorTileFeature, canonical): Array<Array<Point>> {
if (!canonical) return [];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my PR, I had to delete this to get unit tests to pass. Instead of returning an empty array, we need to retain the original logic if canonical is undefined/null and avoid reprojecting unnecessarily. We can refactor reproject to do this without changing the rest of the logic in loadGeometry.

function reproject(p, featureExtent, canonical) {
    let x, y;
    if (canonical) {
        const cs = projection.tileTransform(canonical);
        const z2 = Math.pow(2, canonical.z);
        const lng = lngFromMercatorX((canonical.x + p.x / featureExtent) / z2);
        const lat = latFromMercatorY((canonical.y + p.y / featureExtent) / z2);
        const projectedLngLat = projection.project(lng, lat);
        x = (projectedLngLat.x * cs.scale - cs.x) * EXTENT;
        y = (projectedLngLat.y * cs.scale - cs.y) * EXTENT;
    } else {
        const scale = EXTENT / featureExtent;
        x = Math.round(p.x * scale);
        y = Math.round(p.y * scale);
    }

    return new Point(x, y);
}

const err = pointToLineDist(midProj.x, midProj.y, startProj.x, startProj.y, endProj.x, endProj.y);

if (err >= 1) {
// TODO make sure we never reach max call stack
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: TODO comments cause lint to fail 🤷

@mourner
Copy link
Member Author

mourner commented Jun 8, 2021

@ryanhamley thanks for the review! Addressed in the commit above. I didn't look into Flow compliance because I thought we would land your PR that addresses it first, and then rebase mine making sure it keeps everything green, but we can do either way if rebasing on your side isn't too much trouble.

Copy link
Contributor

@ryanhamley ryanhamley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@mourner mourner merged commit c56bac9 into projections Jun 8, 2021
@mourner mourner deleted the projections-adaptive-resample branch June 8, 2021 16:04
ryanhamley pushed a commit that referenced this pull request Jun 8, 2021
#10753)

* implement adaptive resampling of reprojected geometry

* address feedback
ansis added a commit that referenced this pull request Oct 19, 2021
* rough projection support

* projections stencil clipping and refactor (#410)

* Enable stencil clipping for line and fill layers

* Use buffers from tile

* Refactor tile bounds buffers

* Rename things to not exclusively be RasterBounds

* Create projections directory

* More refactoring

* Combine matrix calculations

* Cleanup

* Refactor projections to new folder

* Begin debug work

* Tile boundaries are working

* Refactor indexbuffer and segmentvector to per painter

* merge projectx and projecty functions

* nits

Co-authored-by: Ansis Brammanis <ansis@mapbox.com>

* Projections fix location issues (#414)

* Add debug page

* Add projection option

* Wire up projection code with worker

* Rename projections folder to projection

* Refactor and update free camera

* Add Projection type and fix center calculations

* Fix bug with transform._center

* Make Winkel projection noop for now

* Update demo HTML and CSS

* temp remove undistortion

Co-authored-by: Ansis Brammanis <ansis@mapbox.com>

* [projections] Adaptive geometry resampling for alternative projections (#10753)

* implement adaptive resampling of reprojected geometry

* address feedback

* Refactor projections code to get all tests passing (#10732)

* [projections] Simplify and optimize tile transform code (#10780)

* simplify projections tile transform

* skip resampling for mercator

* [projections] Fix performance regression in draw_background (#10747)

* separate tiles for background layers

* additional lint & flow fixes

* try fixing tests

* try fixing render tests

Co-authored-by: Ansis Brammanis <ansis@mapbox.com>

* Pin chrome to version 91 (#10887) (#10896)

* Pin to chrome version 91

* Pin chrome version for test-browser

Co-authored-by: Arindam Bose <arindam.bose@mapbox.com>

* Refactor raw projections, handle projection options (#10913)

* refactor raw projections, handle projection options

* add unprojection to winkel tripel

* fix flow

* Pin chrome to version 91 (#10887)

* Pin to chrome version 91

* Pin chrome version for test-browser

* fix lint

* remove to superfluous sin calls

Co-authored-by: Arindam Bose <arindam.bose@mapbox.com>

* Fix bearing for non-mercator projections (#10781)

* Use adaptive resampling with MARTINI & Earcut for non-Mercator tiles (#10980)

* use adaptive resampling and earcut for non-Mercator tile bounds

* fix unit test

* use adaptive MARTINI mesh for non-Mercator raster tiles

* Clamp unproject to valid geo range in alternate projections (#10992)

* clamp unproject to mercator bounds in all projections

* fix marker test

* avoid wrapping center for non-Mercator projections

* extend alt projections clamping to full lat range

* correct zoom, bearing and shear for projections (#10976)

* fix zoom, bearing and skew for projections

* refactor adjustments

* lint

* add comments

* Fix circle and heatmap on alternate projections (#11074)

* fix circle & heatmap on alternate projections (blunder)

* fix unit test

* fix pitch, line-width and other properties for projections (#11080)

and:
- fix fill-extrusions
- remove global projection variable to allow multiple maps on one page
- avoid recalculating tileTransform

* Add Equal Earth, Natural Earth and Lambert Conformal Conic projections (#11091)

* Fix constraining logic for alternate projections (#11092)

* adaptive bbox for projections, refactor resampling

* better precision for adaptive bounds

* remove leftover

* fix zoom/shear adjustments near poles

* optimize tile transform

* fix lint

* attempt to fix tests

* simplify, clarify and consolidate constraining logic

* minor renames in transform

* safer clamping for zoom adjustments

* Projections public API (#11002)

Co-authored-by: Ansis Brammanis <ansis@mapbox.com>

* fix conflicts

* fix seams around alternate-projected tiles (#11119)

* fix unit tests

* remove alaska

* Basic support for custom maxBounds in alternate projections (#11121)

* rudimentary support for custom maxBounds in alternate projections

* fix flow

* fix image and video sources in alternate projections (#11123)

* clean up debug pages

* remove uncessary deg <--> rad conversions

* fix filename casing

* fix queryRenderedFeatures for alternate projections (#11125)

* Projections fixups (#11127)

* disable terrain and fog for alternate projections (#11126)

* Lazily instantiate projected tile debug buffers and release projected buffers when tiles are unloaded (#11128)

* enable lod tile loading for projections (#11129)

* enable lod tile loading for projections

to significantly reduce the number of tiles at low zoom levels

* use Math.hypot(...)

Co-authored-by: Vladimir Agafonkin <agafonkin@gmail.com>

* add comments

Co-authored-by: Vladimir Agafonkin <agafonkin@gmail.com>

* allow map.setProjection(null)

* add limitations

* avoid recreating tile buffer

Co-authored-by: Karim Naaji <karim.naaji@gmail.com>

* fix assertion error

* fix requires

* center projections vertically

Center projections vertically in 0 to 1 range. This shouldn't matter but
there is some constraining behavior that is currently affected by this.

* Fix tile buffer destroyed but not reset (#11134)

* mention settin bounds in projection docs

Co-authored-by: Ryan Hamley <ryan.hamley@mapbox.com>
Co-authored-by: Vladimir Agafonkin <agafonkin@gmail.com>
Co-authored-by: Arindam Bose <arindam.bose@mapbox.com>
Co-authored-by: Karim Naaji <karim.naaji@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip changelog Used for PRs that do not need a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants