-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
correct zoom, bearing and shear for projections #10976
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great to me! The zoom and bearing adjustment behavior is a massive improvement — felt really intuitive to me when browsing and switching between different projections. I also like how the code for it is separated from everything else.
The only moment that felt off is that we're breaking the contract of the location under the cursor remaining the same when scroll-zooming — e.g. user sees SF in the corner and tries to zoom in to it, but the map rotates and they end up in Sacramento. I'm not sure how easy it is to fix so that rotation adjustment happens around the cursor — it might get very involved, so we can merge as is and explore this later.
I initially wanted to suggest some microoptimizations but it seems like this code isn't on a hot path (_calcMatrices
is not called frequently) so it's fine. Maybe worth disabling the whole adjustment path when it's Mercator but not a big deal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot to add — we should probably add code comments to explain some of the logic, e.g. how exactly zoom and shear adjustments get calculated.
Also, is it OK if the locations we pick for adjustment use the given location as a top-left corner rather than center? E.g. for zoom, it picks a second location 1km to the right, and for shear, it makes a 1x1 km square to the right and bottom. Should we change it so that both are "centered" around the original location?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'll have to thoroughly document these changes but this looks great. I agree with Vlad that some code comments would be nice, but otherwise 👍
const angleAdjust = -Math.atan(pdy / pdx) / Math.PI * 180; | ||
|
||
const mc3 = MercatorCoordinate.fromLngLat(loc); | ||
const offset = 1 / 40000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the 40000 number is used in a few places. it might make sense to pull it out into a constant that explains where this figure comes from
I think this should be ok since the difference should be negligible. The could be changed pretty easily if you think it's important |
* 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>
fix #10715
Corrects scale, bearing and shear for projections. It does this by applying corrections to the worldToCamera matrix.
Corrections are applied gradually as you zoom based on a hardcoded zoom range. My attempts to automatically derive a good range from each projection didn't work out. The range needs to consider both the amount of distortion at that zoom and how the change feels while zooming. The latter is somewhat subjective.
There is no external configuration for this. Each projection has it's own hardcoded settings.