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

Reuse prefetched tiles to avoid empty screen #2668

Conversation

alexcristici
Copy link
Collaborator

@alexcristici alexcristici commented Jul 31, 2024

When zooming out if needed tile is not loaded or not in cache it will try to use the prefetched tiles from a higher zoom level.

Before:

Before.MP4

After:

After.MP4

Recordings are from Debug iPhone 6s so the behavior to be more visible, release build is much faster.

Before:

Before.MP4

After:

After.MP4

@alexcristici alexcristici self-assigned this Jul 31, 2024
@alexcristici alexcristici marked this pull request as ready for review July 31, 2024 17:50
Copy link

github-actions bot commented Jul 31, 2024

Bloaty Results (iOS) 🐋

Compared to main

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  +0.0%    +664  [ = ]       0    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results-ios/pr-2668-compared-to-main.txt

Copy link

github-actions bot commented Jul 31, 2024

Bloaty Results 🐋

Compared to main

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  +0.0% +10.8Ki  +0.0% +1.94Ki    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-2668-compared-to-main.txt

Compared to d387090 (legacy)

    FILE SIZE        VM SIZE    
 --------------  -------------- 
   +27% +31.0Mi  +420% +25.1Mi    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-2668-compared-to-legacy.txt

Copy link

github-actions bot commented Jul 31, 2024

Benchmark Results ⚡

Benchmark                                                     Time             CPU      Time Old      Time New       CPU Old       CPU New
------------------------------------------------------------------------------------------------------------------------------------------
OVERALL_GEOMEAN                                            -0.0137         -0.0135             0             0             0             0

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/benchmark-results/pr-2668-compared-to-main.txt

@louwers
Copy link
Collaborator

louwers commented Jul 31, 2024

This test is failing:

[ RUN      ] Map.PrefetchTiles
/home/runner/work/maplibre-native/maplibre-native/source/test/map/prefetch.test.cpp:63: Failure
Expected equality of these values:
  tiles.size()
    Which is: 19
  expected.size()
    Which is: 10
[  FAILED  ] Map.PrefetchTiles (318 ms)

@alexcristici
Copy link
Collaborator Author

This test is failing:

[ RUN      ] Map.PrefetchTiles
/home/runner/work/maplibre-native/maplibre-native/source/test/map/prefetch.test.cpp:63: Failure
Expected equality of these values:
  tiles.size()
    Which is: 19
  expected.size()
    Which is: 10
[  FAILED  ] Map.PrefetchTiles (318 ms)

I will have to update the tests.

@alexcristici alexcristici changed the title Reuse rendered tiles to avoid empty screen Reuse prefetched tiles to avoid empty screen Aug 1, 2024
@alexcristici
Copy link
Collaborator Author

Tests are passing, PR is ready for review.

@louwers
Copy link
Collaborator

louwers commented Aug 2, 2024

In my testing the old tiles stick around too long, which has unexpected visual results, especially when the density of data at a higher zoom level very is different.

image

Before

Screen.Recording.2024-08-02.at.15.47.11.mov

After

Screen.Recording.2024-08-02.at.15.46.15.mov

@alexcristici
Copy link
Collaborator Author

@louwers Should be fine now. Please check again.

@louwers
Copy link
Collaborator

louwers commented Aug 2, 2024

That looks better!

@alexcristici
Copy link
Collaborator Author

pre-commit.ci autofix

@alexcristici
Copy link
Collaborator Author

@louwers Not sure why precommit is failing.

Copy link
Collaborator

@TimSylvester TimSylvester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Have you tried it in overscaled and wrapped cases? It doesn't look like the new tests consider them.

@alexcristici
Copy link
Collaborator Author

Looks good. Have you tried it in overscaled and wrapped cases? It doesn't look like the new tests consider them.

Yes, overscaled works good. algorithm::updateRenderables function is based on overscaled tile logic.

@alexcristici alexcristici merged commit 5ec97c3 into maplibre:main Aug 14, 2024
38 checks passed
@alexcristici alexcristici deleted the reuse-rendered-tiles-to-avoid-empty-screen branch August 14, 2024 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants