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.

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

Fix marker flying off near horizon #3704

Merged
merged 7 commits into from
Feb 17, 2024

Conversation

sbachinin
Copy link
Collaborator

Fixes #2871

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

The problem here is smartWrap.
It's not designed for 3d world.
It measures distances between points based on their screen position. The goal is to provide smooth transition of a Marker/Popup from central globe to the side globes.
This technique is safe only when there is no terrain and no high pitch/bearing.
Now there are many scenarios in which smartWrap will cause quirky behaviour. (Though they are very unlikely to happen for a sensible user).
#2871 is only one of such cases. Another one for example.

All these quirks are reproducible for Popups too.

They mostly happen when you "look" rather eastwards or westwards and the marker is (almost) at the same latitude as the camera.

In 3d world smartWrap seems to be conceptually wrong. Sometimes a very distant point (-360 or +360 deg) appears very close (in terms of screen pixels) to a current point due to high pitch or whatever. SmartWrap decides that the marker belongs to that distant globe and assigns a new lng accordingly. This is the reason for the current bug.

Ideal solution would mean a complete rewrite of smartWrap. And it has to be something extra smart. But considering how small the bugs are, it's better to fix some minor mistakes.

In this PR I suggest 2 small improvements that are simple and just make sense. They also look safe in terms of preserving the existing smartWrap behavior.

Current bug remains in theory possible but after these changes I can't reproduce it at all. Also the 2nd bug from video above disappears too.

What these fixes do.

  1. SmartWrap compared an elevated "prior position" with non-elevated "possible new positions". For this reason priorPos is often considered closer to the "very distant" position than it actually is.
    To fix this, I pass a non-elevated prior position to smartWrap. Unfortunately I had to introduce a separate property on a Marker class to save this flatPos.
  2. Smart-wrapped position should be applied only if it's below horizon.
marker.flying.before.after.mp4

As for tests, I have yet no idea how to make it.

BTW existing smartWrap tests were written only for Popup but not for Marker. Don't know if it's a problem.

Let me know if I'm writing too much.

@codecov-commenter
Copy link

codecov-commenter commented Feb 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3bea9a1) 87.04% compared to head (5106423) 87.06%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3704      +/-   ##
==========================================
+ Coverage   87.04%   87.06%   +0.02%     
==========================================
  Files         240      240              
  Lines       32224    32237      +13     
  Branches     1978     1984       +6     
==========================================
+ Hits        28048    28066      +18     
+ Misses       3286     3269      -17     
- Partials      890      902      +12     

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

src/util/smart_wrap.ts Outdated Show resolved Hide resolved
@sbachinin sbachinin force-pushed the Fix-marker-flying-near-horizon branch from c33dab3 to 832320d Compare February 17, 2024 07:21
@HarelM
Copy link
Member

HarelM commented Feb 17, 2024

Changelog is missing, otherwise this looks good.

@sbachinin
Copy link
Collaborator Author

Shall I add smartWrap tests in this pr?

@HarelM
Copy link
Member

HarelM commented Feb 17, 2024

Your call, you are well familiar with this method now, so adding unit tests to it should be fairly simple for you.
Having said that, this method is fully covered according to codecov, so it might be an overkill.
I leave the decision to you.

@sbachinin
Copy link
Collaborator Author

Ok, I'll add smartWrap tests here

@HarelM
Copy link
Member

HarelM commented Feb 17, 2024

THANKS!

@HarelM HarelM merged commit b9023bc into maplibre:main Feb 17, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Marker flies off in the distance when chaning view angles
3 participants