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

Bugfix: _cameraForBoxAndBearing not fitting bounds properly when using asymettrical camera viewport and bearing #3591

Merged
merged 7 commits into from
Jan 23, 2024

Conversation

fcwheat
Copy link
Contributor

@fcwheat fcwheat commented Jan 15, 2024

I work with rotated maps a lot and noticed that fitting to bounds was not functioning as expected for some cases. In digging into this came to realize that the NW-SE points on the bounding box were always being fit to even in cases when fitting to these points leads to the NE-SW points being out of frame.

The code was updated to consider all corners of the bounding box when rotating and fitting.

In performing the update it highlighted two existing test cases which were incorrect and so these were updated as well. A new test case was added which is similar to the case presented in the JS Fiddle.

I also added a small degreesToRadians utility function since it felt cleaner to use this than repeatedly typing degrees * Math.PI / 180

JS Fiddle with a demo of the bug: https://jsfiddle.net/z4xma2wk/3/

Actual result when fitting bounds with 45deg rotation:
too_zoomed

Approx. expected result when fitting bounds with 45deg rotation:
correct_zoom

Launch Checklist

  • 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.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality.
  • Update changelog

@fcwheat fcwheat changed the title Fit bounds with bearing fix Bugfix: _cameraForBoxAndBearing not fitting bounds properly when using asymettrical camera viewport and bearing Jan 15, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jan 15, 2024

Codecov Report

Attention: 14 lines in your changes are missing coverage. Please review.

Comparison is base (8aaa935) 74.74% compared to head (641fa8e) 85.90%.
Report is 38 commits behind head on main.

Files Patch % Lines
src/ui/camera.ts 51.72% 14 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #3591       +/-   ##
===========================================
+ Coverage   74.74%   85.90%   +11.15%     
===========================================
  Files         243      244        +1     
  Lines       19156    48297    +29141     
  Branches     4239     5152      +913     
===========================================
+ Hits        14319    41488    +27169     
- Misses       4837     6809     +1972     

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

@HarelM
Copy link
Collaborator

HarelM commented Jan 16, 2024

Can you check if merging from main improve the code coverage percentage?
Otherwise, assuming this is well covered with the new tests, I think it can be merged.

@HarelM
Copy link
Collaborator

HarelM commented Jan 16, 2024

Please also add a changelog entry.

@fcwheat
Copy link
Contributor Author

fcwheat commented Jan 22, 2024

@HarelM this should be all set now. Let me know if there is anything else I missed.

Codecov

@HarelM HarelM merged commit 533de48 into maplibre:main Jan 23, 2024
15 checks passed
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.

3 participants