-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
jump back to world zero when crossing the antimeridian in flyTo and other camera operations. Fixes #2071 #4451
Conversation
8d3179a
to
ed37227
Compare
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.
Thanks for working on this!
src/geo/lng_lat.js
Outdated
@@ -42,7 +42,11 @@ class LngLat { | |||
* wrapped.lng; // = -73.9749 | |||
*/ | |||
wrap() { | |||
return new LngLat(wrap(this.lng, -180, 180), this.lat); | |||
if (this.lng <= -180 || this.lng > 180) { |
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.
Any particular reason to change this behavior? We'd want to at least update the API documentation above, which says that wrap
always returns a new object.
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.
Since my changes meant that LngLat.wrap is called a lot more (every camera change due to an interaction), I wanted to ensure that creating the new LngLat object wouldn't impact performance.
Although on further testing, map interaction seems to have no degradation without this check. So I'm not sure whether to leave it in and update the API docs or just remove this change, and leave it as is.
Thoughts?
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.
If there's no performance impact, let's leave it as is.
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.
Agreed, fixed, and squashed commits.
cf935b2
to
4920f1b
Compare
097cfbe
to
fb580e0
Compare
fb580e0
to
6a3357b
Compare
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 world sometimes flashes white when you cross the anti-meridian
I didn't observe this in my testing. If you're still seeing it, can you open an issue with steps to replicate?
I only noticed it occasionally, what happens is the map is there and after the jump it redraws part of the map (a vector tile) so the map flashes the background colour while this happens. I agree that we should merge this and follow up with a new issue to address that. |
Launch Checklist
Some background in By default, snap back to "w=0" world after panning operation (i.e. "worldCopyJump") #2071. When
flyTo
decides to cross the anti-meridian from world 0, it will end up in either world -1 or 1. BecauseMarker
DOM elements are only drawn in one place and only on world 0, they don't show up at the moment.This PR aims to ensure that when flyTo is about to step across the anti-meridian into world -1 or 1, it teleports to ensure it lands back in world 0, and so
Marker
elements appear.It also ensure's that other camera operations like
jumpTo
,easeTo
don't leave the map in a non world 0 state.write tests for all new functionality
document any changes to public APIs
I feel this should be transparent to the public API, it's a private behaviour.
post benchmark scores
manually test the debug page
I wrote a new debug page to test this, but haven't committed it.
the world sometimes flashes white when you cross the anti-meridian, I'm not sure how to fix this.
only jump back to world 0 when renderWorldCopies is true