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.

Sign up for GitHub

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 to bottom

Drag globe near poles with versor #5330

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Drag globe near poles with versor #5330

wants to merge 7 commits into from
+238 −114

Conversation

jcolot
Copy link

@jcolot jcolot commented Jan 12, 2025 •
edited by HarelM
Loading

Launch Checklist

For points that are not on the globe, do nothing.

  • 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.

@codecov Codecov
Copy link

codecov bot commented Jan 12, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 57.72358% with 52 lines in your changes missing coverage. Please review.

Project coverage is 91.77%. Comparing base (f1d58f2) to head (ebb68d9).
Report is 17 commits behind head on main.

Files with missing lines Patch % Lines
...c/geo/projection/vertical_perspective_transform.ts 56.36% 48 Missing ⚠️
...o/projection/vertical_perspective_camera_helper.ts 20.00% 4 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #5330      +/-   ##
==========================================
- Coverage   91.84%   91.77%   -0.08%     
==========================================
  Files         282      282              
  Lines       38908    38957      +49     
  Branches     6827     6832       +5     
==========================================
+ Hits        35735    35751      +16     
- Misses       3046     3078      +32     
- Partials      127      128       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jcolot jcolot mentioned this pull request Jan 14, 2025
Open
@HarelM HarelM changed the title Drag globe near poles with versor (#5296) Drag globe near poles with versor Jan 14, 2025
HarelM
HarelM reviewed Jan 15, 2025
View reviewed changes
src/geo/projection/vertical_perspective_transform.ts
@@ -49,187 +50,247 @@ export class VerticalPerspectiveTransform implements ITransform {
get pixelsToClipSpaceMatrix(): mat4 {
return this._helper.pixelsToClipSpaceMatrix;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Better to avoid these kind of changes as part of this PR, I would recommend moving this to a different PR if this is truly needed.

@kubapelc
Copy link
Collaborator

I played around with this for a while, here are some thoughts on the controls:

  • I think it would be very good UX-wise to find a way to make the whole screen usable for panning, not just the part covered by the globe.
  • The map still rolls slightly when panning around Europe at zoom levels around 5 to 7. At this point the map looks pretty flat, maybe it would be more intuitive to transition to fixed-bearing panning at this point?
  • The bearing change from the new controls sometimes clashes with the "snap to north" behaviour MapLibre does (eg. in the case described above). Maybe it should be disabled if fixed bearing panning isn't in use?

It works quite nicely overall!

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

@HarelM HarelM HarelM left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Globe panning is reverted when North pole facing the user
3 participants
@jcolot @kubapelc @HarelM