maplibre-gl-js icon indicating copy to clipboard operation
maplibre-gl-js copied to clipboard

fix incorrect bounds with fitBounds when crossing antimeridian

Open YoelRidgway opened this issue 1 year ago • 2 comments

If you add bounds that cross the 180th meridian (antimeridian), it causes the view to zoom out so that the northeast point of the bounds is to the left and the southeast point of the bounds is to the right. See below for two examples using map.fitBounds() for the islands of Fiji - current and expected:

Current (not correct): https://output.jsbin.com/yitecurala

Expected (my fix): https://output.jsbin.com/janaferezi

Launch Checklist

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

YoelRidgway avatar Aug 28 '24 16:08 YoelRidgway

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 87.77%. Comparing base (34dd6b6) to head (06dd907). Report is 43 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4620      +/-   ##
==========================================
- Coverage   87.97%   87.77%   -0.21%     
==========================================
  Files         247      247              
  Lines       33622    33633      +11     
  Branches     2157     2173      +16     
==========================================
- Hits        29580    29521      -59     
- Misses       3076     3133      +57     
- Partials      966      979      +13     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Aug 28 '24 16:08 codecov-commenter

Thanks for taking the time to contribute!

HarelM avatar Aug 28 '24 17:08 HarelM

@HarelM I've come across an interesting case... What should happen if the sw and ne points are flipped? In the current version, fitBounds() will act as if nothing happened. However, with adjustAntiMeridian(), it puts the ne point to the east and the sw point to the west. Is this undesirable? I feel like LngLatBounds is supposed to take sw and ne in order so its normal that it assumes they have been input in correctly, otherwise the bounds could just be defined as any two points, e.g p0 and p1.

YoelRidgway avatar Aug 29 '24 15:08 YoelRidgway

That's exactly what I aimed for in terms of writing tests 😃. You can have two points on the globe and you need to decide if you always take the "shotest" arc, or you use the arc based on the "direction" (vector) between the points. I think the shortest is problematic in terms of cases it supports (it can't show a wide map), so the only possible solution (without adding a new flag) is to assume order and have that define the "direction". What do you think?

HarelM avatar Aug 29 '24 18:08 HarelM

Yes I totally agree. I think this is the most logical way of going about things. I just hope there isn't someone who has been using fitBounds() the wrong way all this time only to have everything messed up after the change!

YoelRidgway avatar Aug 29 '24 18:08 YoelRidgway

It's probably important to document this as part of the method documentation, just to be on the safe side.

HarelM avatar Aug 29 '24 18:08 HarelM

Version 5 is the next breaking change version, I believe I'll start looking at it in the next month or so, we can consider adding this change only as part of this breaking change version. But I don't know if this is really a behavior change and not simply a bug fix...

HarelM avatar Aug 29 '24 18:08 HarelM

Even a bug fix breaks someones workflow! ;) https://xkcd.com/1172/

YoelRidgway avatar Aug 29 '24 19:08 YoelRidgway

Thanks! Can you add a changelog entry please? Also worth mentioning something in fit bounds docs (public API).

HarelM avatar Aug 30 '24 08:08 HarelM