Skip to content

Conversation

kang-matthew
Copy link

Launch Checklist

Fixes: #5839.

Bug: When globe + terrain are enabled and panning toward the arctic circle or Antarctica enlarges the globe. Happens only on terrain, and it does not do this when only globe is enabled. (Related: Zoom Behavior)

Fix: The fix is to change one of the handler functions to use handleMapControlsPan used in non-terrain, which adds the appropriate zoom adjustment with respect to the deltas. Before it uses setCenter directly without factoring zoom adjustment.

Note: This does not fix the issue for Key Pan (by up or down arrow keys). The globe is enlarged without zoom adjustment for both terrain and non-terrain when panning towards the poles using arrow keys.

After Fix Visuals:

fix-5839.mov
  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
  • Briefly describe the changes in this PR.
  • Link to related issues.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality.
  • Document any changes to public APIs.
  • Post benchmark scores.
  • Add an entry to CHANGELOG.md under the ## main section.

@acalcutt
Copy link
Contributor

acalcutt commented May 9, 2025

Does this still work when you are at a high zoom where you can see terrain? Is seems like the code removed was mapping a point on the screen which I am guessing is supposed to account for the terrain when grabbing.

@HarelM
Copy link
Collaborator

HarelM commented May 9, 2025

I'm surprised no test failed.
Yes, this code should help terrain panning, you can't simply remove it.
You might want to add the current projection state to the if.
But also to where terrain movement is initialized.

@kang-matthew
Copy link
Author

@acalcutt @HarelM Thank you to both of you! It's my first attempt at an issue and I wasn't familiar with the panning implementations. I'm not sure if I perfectly understand it, but here's what I thought:

My understanding is that terrain mode should drag the map by the pan delta, instead of moving the picked point itself. And in the panning implementations (handleMapControlSpan) in different projections - Vertical Perspective and Mercator - vertical perspective (globe projection) should drag the map by the pan delta, and the mercator projection should move the picked point.

Hence for the case when we have mercator projection and terrain mode, we should not call handleMapControlSpan, as it would move the point around instead of panning by delta. And that's why we should add the current projection state to the if?

Thanks again!

@HarelM
Copy link
Collaborator

HarelM commented May 17, 2025

Terrain should move the picked point as in heavy terrain the picked point could be a mountain and you want to move it and not by "simple" delta as the case for no terrain (mercator).
Globe and terrain were both added and I'm not sure the solution to panning both is well defined.
But the idea is that pen delta is not a good fit for terrain due to the above use case.

@acalcutt
Copy link
Contributor

Just curious looking at the changes and because I don't know. When in globe mode, does it switch to mercator projection at some zoom level? probably not right?

Seems to me like maybe zoom level could play part in when to change the dragging action, but I think (harel?) may have mentioned before in another PR something like that is a bit hacky.

@HarelM
Copy link
Collaborator

HarelM commented May 18, 2025

The definition of globe is vertical persoective until zoom 11 then gradually switch to mercator when in zoom 12 it's fully mercator.

@acalcutt
Copy link
Contributor

I created this test page so I could test the code in this PR
https://stackblitz.com/edit/web-platform-mfx8cj79?file=index.html

which I am comparing to 5.5.0 in
https://stackblitz.com/edit/web-platform-kfwcsncc?file=index.html

It does seem like it at least improves #5839, though i want to do a bit more comparison.

@shreeharshshinde
Copy link
Contributor

shreeharshshinde commented Jul 25, 2025

Hi @HarelM, quick question — in the terrain+drag block, we use tr.setCenter(...) instead of cameraHelper.handleMapControlsPan(...) like in other cases. Was there a specific reason for avoiding the helper there? Just wondering if using the helper uniformly could simplify things or if setCenter is needed for terrain accuracy. Thanks!

@HarelM
Copy link
Collaborator

HarelM commented Aug 6, 2025

Yes, I believe this is intentional as I mentioned earlier where you can grab a mountain when terrain is on, which is not the case for non-terrain map.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants