-
Notifications
You must be signed in to change notification settings - Fork 2.2k
feat(core): Improved GlobeController #9913
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
base: master
Are you sure you want to change the base?
Conversation
chrisgervang
left a comment
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 is much appreciated. There aren't any "before" comparisons, which is fine, but would you say there is anything significantly different?
It'd be great to have some tests added for the maths, and a pass taken on the docs
| /** | ||
| * End panning | ||
| * Must call if `panStart()` was called | ||
| * Must call if `panStart()` was not called |
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'm a little confused - when are you supposed to call this? Could we rewrite this as the positive form rather than the negative?
The current behavior is on the website https://deck.gl/examples/globe-view, I would go as far as to say it’s basically unusable |
| * @return {Object} props of the new viewport | ||
| */ | ||
| panByPosition(coords: number[], pixel: number[]): any { | ||
| panByPosition(coords: number[], pixel: number[], startPixel?: number[]): any { |
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 assumption (at least with the other viewports) is that startPixel is this.project(coords). Is it not the case with globe?
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.
No, as you can start the interaction outside the globe - try it on the website example, it’s impossible to rotate the globe, and contrast to how MapLibre behaves
Closes #9676
Background
The
GlobeControllerhas always been flaky, but since the changes made in order to integrate with Maplibre Globe, the controls in deck.gl are hopelessly broken: https://deck.gl/examples/globe-viewOne of the causes for the breakage is the introduction of a
scaleAdjustparameter which:New handling
This PR changes the way the controls work, to bring them closer to what Maplibre is doing:
Zoom adjustment
The new requirement that "globe and web mercator projection results converge" means we have to treat zoom differently in globe view, and it is arguably a bit counterintuitive as it no longer is related to size of the globe on-screen.
This PR replaces the
scaleAdjustparameter with azoomAdjustparameter that is mathematically equivalent, but easier to use in the code, in particular when it comes to clipping thezoomtominZoomandmaxZoom.Testing
Here is how the new behavior looks:
Panning
Zooming
Transition
Note that it is not trivial to add these changes to the
ControllerandViewStateclasses without breaking the controls either in the Maplibre Globe view or the standard Web Mercator controlsChange List
scaleAdjustwithzoomAdjustpanByPositionhandler forGlobeViewportstartPixeltopanByPositionhandlers