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

[Feature] Allow underzooming with single-copy world #4612

Closed

Conversation

larsmaxfield
Copy link
Contributor

@larsmaxfield larsmaxfield commented Aug 27, 2024

Launch Checklist

Fixes #4510, #4591 by allowing the map to be underzoomed when renderWorldCopies=false:

Here is a demo where you can toggle renderWorldCopies:

unbounded
https://larsmaxfield.github.io/swings/maplibre-underzoom/render-world-copies.html

The solution is a simple change to getConstrained() in src/geo/transform.ts where when renderWorldCopies=false the viewport (screen) is assumed to be a square with side length equal to the smallest screen dimension reduced by a boundsRatio factor of 50%. This allows the map to be panned and zoomed out 50% beyond the bounds along the narrowest screen dimension.

This change does not override minZoom — it always obeys that value.

Since users can pan and zoom 50% beyond the bounds, it means that a map with a specified maxBounds (and renderWorldCopies=false) will allow users to pan and zoom beyond those bounds. Here are two demos of that:

tall bounds
https://larsmaxfield.github.io/swings/maplibre-underzoom/tall-bounds.html

wide bounds
https://larsmaxfield.github.io/swings/maplibre-underzoom/wide-bounds.html

Fixes #4510, #4591

  • 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.
  • Add an entry to CHANGELOG.md under the ## main section.

@larsmaxfield larsmaxfield changed the title [FeatureAllow underzoom [Feature] Allow underzooming with single-copy world Aug 27, 2024
@codecov-commenter
Copy link

codecov-commenter commented Aug 27, 2024

Codecov Report

Attention: Patch coverage is 20.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 65.51%. Comparing base (2200020) to head (61eede3).
Report is 3 commits behind head on main.

Files Patch % Lines
src/geo/transform.ts 20.00% 3 Missing and 1 partial ⚠️

❗ There is a different number of reports uploaded between BASE (2200020) and HEAD (61eede3). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (2200020) HEAD (61eede3)
9 8
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #4612       +/-   ##
===========================================
- Coverage   87.94%   65.51%   -22.44%     
===========================================
  Files         247      247               
  Lines       33616    33620        +4     
  Branches     2375     1405      -970     
===========================================
- Hits        29564    22025     -7539     
- Misses       3087    10775     +7688     
+ Partials      965      820      -145     

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

@larsmaxfield larsmaxfield marked this pull request as draft August 27, 2024 08:47
@larsmaxfield
Copy link
Contributor Author

I need help with these (@HarelM or others?):

  • I do not know if the out-of-maxbounds behavior should be considered a breaking change. By setting renderWorldCopies=true (or simply not setting it at all) then maxBounds will behave as before. Also, if maxBounds is being set, I can't foresee a case where one would care to set renderWorldCopies false.

  • I do not know what new tests (if any) are appropriate for this.

@HarelM
Copy link
Collaborator

HarelM commented Aug 27, 2024

Yes, this is a breaking change according to current implementation.
The way this is implemented right now is not configurable in any way and therefore also problematic.
I belive a better design is missing here in terms of hiw a user would use it, what options he has, etc.
I would also look into minzoom definition to better support this feature, maybe setting minzoom to a negative number is a better way to solve this, IDK...

unit tests are needed to make sure this the functionally is captured, similar to the unit tests that failed as part if this changes.

@larsmaxfield
Copy link
Contributor Author

larsmaxfield commented Aug 27, 2024

Thanks for the tips.

I agree that giving users control over this would be useful.

Using minzoom does seem easier at first glance but then the relative visible out-of-bounds area becomes dependent on viewport size where the map would always be (for example) 256px wide regardless of screen size.

The design I propose maintains a standard ratio of visible out-of-bounds areas that keeps a standard balance of map area to underzoomed viewport area. This ensures a fully zoomed out small and tall viewport (like a phone) appears the same on a large and wide viewport (like a monitor). This ratio is currently 50%:

Wide monitor:
large-wide

Tall monitor:
large-tall

Tall phone:
small-tall

I'll give it some thought for further options or other approaches and update here.

@HarelM
Copy link
Collaborator

HarelM commented Aug 27, 2024

The 0.5 ratio is probably what you need, but it might not be what other people need... :-)

@larsmaxfield
Copy link
Contributor Author

Indeed 👍 I'll look to add that as a setting and put a demo here showing what happens when it is adjusted.

(Also, let me know if it's better that I hold this discussion back in #4510 instead of in this PR — I'm not aware of best practices for where it's best to hold them.)

@HarelM
Copy link
Collaborator

HarelM commented Aug 27, 2024

Since there are others subsrived to the issue itself I think it will be better to continue the discussion there.

@larsmaxfield
Copy link
Contributor Author

Closing this draft PR as I'm taking a different design approach. Will discuss further in #4510.

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.

[Feature] Allow "underzooming" to show entire map on non-square viewports when renderWorldCopies=false
3 participants