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

Clamp unproject to mercator bounds in all projections #10992

Merged
merged 4 commits into from
Sep 20, 2021

Conversation

mourner
Copy link
Member

@mourner mourner commented Sep 6, 2021

This PR fixes the issue where the map breaks when you pan outside the world in alternate projections — it does so by adding clamping logic to each individual projection so that unproject never produces NaN or values outside of the Web Mercator bounds (-180, -85.05.., 180, 85.05..), and you can't pan in a way that puts center of the map outside of valid geographic bounds. Notes:

  • Clamping has to happen inside individual unproject (sometimes in the middle of calculations, e.g. in Albers) because you can't clamp once a value turns into NaN.
  • Theoretically, some projections could be clamped beyond Web Mercator limits (e.g. 90 latitude and not 85.05+), but it might be better for consistency to clamp to the same range everywhere, especially since our vector and raster tiles only cover the Web Mercator range, and panning outside it might be counterintuitive.
  • Projections that can have world copy rendering only clamp lat, not lng, to keep the current behavior of panning seamlessly through anti-meridian (which doesn't yet work for wgs84 projection but keeps the default behavior of mercator).
  • There's a known issue of the map going wild jumping chaotically during fast panning inertia. Not critical to address in this PR but we should ticket this and follow up.
  • This approach feels pretty intuitive. You can't move the center of the map outside the rendered area, but you can still grab the map in empty areas for panning. getCenter()/etc. will always return valid results. And we avoid the issue of breaking the return type signature when porting this to native.
  • In the future, we should introduce wrapping for Albers so that it could represent areas like western islands of Alaska & far eastern Russia seamlessly instead of having a hard cut (e.g. so that "anti-meridian" moves to a different vertical tile bound).

@mourner mourner requested a review from ansis September 6, 2021 08:01
@ryanhamley
Copy link
Contributor

ryanhamley commented Sep 7, 2021

I'm seeing the projections jump from the left edge to right and vice versa. I understand why this is happening based on the clamping and wrapping, but it seems like undesired behavior, at least when world wrapping isn't turned on.
Untitled

@mourner
Copy link
Member Author

mourner commented Sep 8, 2021

@ryanhamley nice catch! Fixed this in the commit above. Generally, I should work on overhauling our constraining / bounds / wrapping / worldCopy logic. Let's leave that for a separate PR though, so that we can merge this more minimal one that at least gets us to a state where map doesn't break on interaction.

@ryanhamley
Copy link
Contributor

This looks good, but there's one other thing I'm seeing. Go to http://localhost:9966/debug/projections.html#2.6/25.98/-83.31/35.7 for example (in the default Albers projection) and you can see that it's impossible to pan the map so that the southern tip of South America comes into view. The same happens at http://localhost:9966/debug/projections.html#2.6/4.02/154.71/179.1 where you can't view the upper left corner of the map. I know realistically, there's no reason to center an Albers projection on the US then try to view Patagonia, but it feels weird to not be able to pan to certain parts of the map. Do we just accept this behavior and document it (and maybe note that conical projections should generally be used with a map bounds)?

@mourner
Copy link
Member Author

mourner commented Sep 9, 2021

@ryanhamley No, that's what I was going to look at next in a separate PR as mentioned above. It's the constraining logic in transform which currently doesn't take projections into account.

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.

Sweet. This looks great then!

Copy link
Contributor

@ansis ansis left a comment

Choose a reason for hiding this comment

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

Web Mercator bounds (-180, -85.05.., 180, 85.05..),

This is going to change existing behavior for map.unproject(...) in mercator, right? I think this currently returns values outside this range for both latitude and longitude. I'd say the clamping should be to whatever makes sense for each projection, which I think means no clamping for mercator.

I think all of the projections implemented so far should be able to support the full latitude range

@mourner mourner merged commit 947ca58 into projections Sep 20, 2021
@mourner mourner deleted the projections-clamp branch September 20, 2021 15:49
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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants