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 level of detail at high pitch #4741

Closed
wants to merge 6 commits into from

Conversation

NathanMOlson
Copy link

This (failing) unit test demonstrates the issue reported here: #3983 and #4703

@NathanMOlson NathanMOlson marked this pull request as draft September 23, 2024 05:53
@codecov-commenter
Copy link

codecov-commenter commented Sep 23, 2024

Codecov Report

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

Project coverage is 86.35%. Comparing base (718e0cb) to head (c4a1995).

Files with missing lines Patch % Lines
src/geo/transform.ts 84.61% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4741      +/-   ##
==========================================
- Coverage   87.95%   86.35%   -1.61%     
==========================================
  Files         247      247              
  Lines       33656    33663       +7     
  Branches     2165     2237      +72     
==========================================
- Hits        29603    29068     -535     
- Misses       3082     3517     +435     
- Partials      971     1078     +107     

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

@NathanMOlson NathanMOlson changed the title Add failing unit test for level of detail at high pitch Fix level of detail at high pitch Sep 23, 2024
@NathanMOlson
Copy link
Author

This fixes #3983 and improves #4703 by calculating the desired zoom level for each tile independently. Each tile zoom is calculated by scaling the center point zoom by the ratio of (3D) distance-to-tile to distance-to-center. Old behavior is preserved when there is no terrain and pitch < 60 degrees.

Before at 70 degrees pitch:
before_lodfix
After:
after_lodfix

Before at 80 degrees pitch:
before_lodfix_80
After:
after_lodfix_80

@NathanMOlson NathanMOlson marked this pull request as ready for review September 23, 2024 06:39
@HarelM
Copy link
Collaborator

HarelM commented Sep 23, 2024

Thanks for taking the time to open this PR!
We are working on the globe branch where we moved a lot of the tile fetching logic to a different file.
There are two options here from my point of view:

  1. Merge this to main and then merge/rewrite this in globe branch.
  2. Merge this to globe branch and wait for version 5 release.

Let me know which works better for you.

Can you also show before and after of the showtilesboundries when terrain is disabled?

@NathanMOlson
Copy link
Author

The previously-posted images are with terrain disabled. Here is the before/after with terrain enabled:

Before:
before_lodfix_terrain
After:
after_lodfix_terrain

@NathanMOlson
Copy link
Author

Thanks for taking the time to open this PR! We are working on the globe branch where we moved a lot of the tile fetching logic to a different file. There are two options here from my point of view:

  1. Merge this to main and then merge/rewrite this in globe branch.
  2. Merge this to globe branch and wait for version 5 release.

Let me know which works better for you.

I just took a look at the globe branch and it looks like there's been some extensive rearrangement. So I think this will need a rewrite either way. Given that there will be no new 4.x releases, I don't see a reason to merge it to main. I'll work on moving it to globe.

@HarelM
Copy link
Collaborator

HarelM commented Sep 23, 2024

I can carve out a new release if 4.x if this is something you absolutely need, but I would prefer to simply move this to 5.x if this is not urgent for you as it might take around 2 month to release a stable 5.x version.

@NathanMOlson
Copy link
Author

I'm happy to move this to 5.x.

@NathanMOlson
Copy link
Author

Superceded by #4750

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.

3 participants