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 5486 #5518

Merged
merged 3 commits into from
Feb 20, 2025
Merged

Fix 5486 #5518

merged 3 commits into from
Feb 20, 2025

Conversation

NathanMOlson
Copy link
Contributor

Fixes #5486.

Do not allow variable zoom tiles just because the top padding is non-zero. Only allow variable zoom at high pitch or when terrain is enabled.

  • Briefly describe the changes in this PR.
  • Link to related issues.
  • Write tests for all new functionality.
  • Add an entry to CHANGELOG.md under the ## main section.

…ng is non-zero. Only allow variable zoom at high pitch or when terrain is enabled.
Copy link

codecov bot commented Feb 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.95%. Comparing base (a99fe93) to head (d959ece).
Report is 22 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5518      +/-   ##
==========================================
- Coverage   91.96%   91.95%   -0.01%     
==========================================
  Files         282      282              
  Lines       39050    39050              
  Branches     6855     6853       -2     
==========================================
- Hits        35911    35909       -2     
- Misses       3012     3013       +1     
- Partials      127      128       +1     

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

@HarelM
Copy link
Collaborator

HarelM commented Feb 18, 2025

Thanks! Do you think a render test can help here?

@NathanMOlson
Copy link
Contributor Author

@HarelM No. The bug was in the way covering tiles were calculated when the top padding was non-zero. The new unit test covers that. The rendering problem shown in the original bug report was because the source did not contain the lower-zoom tiles that were being requested.

Current MapLibre behavior is not to request higher zoom tiles when the requested zoom is not available. This seems like a reasonable default to prevent an explosion of tile requests, but it may be worth looking into a way to change that behavior (outside the scope of this PR).

@HarelM
Copy link
Collaborator

HarelM commented Feb 19, 2025

In a render test we can have the numbers source and make sure all the tiles are from the same zoom, which is what this fix is about as far as I understand.
Before this fix there's a way to receive different zoom level, right?
This is just a thought, I'm fine with merging this as is too.

@NathanMOlson
Copy link
Contributor Author

Before this fix there's a way to receive different zoom level, right?

Correct.

I think the existing unit test is more appropriate than a render test.

@HarelM HarelM merged commit b61bb03 into maplibre:main Feb 20, 2025
17 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.

rendering incomplete when using setPadding()
2 participants