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

[REGRESSION] Implicit Tiling Repeating request #761

Closed
AnthonyGlt opened this issue Sep 25, 2024 · 9 comments · Fixed by #762
Closed

[REGRESSION] Implicit Tiling Repeating request #761

AnthonyGlt opened this issue Sep 25, 2024 · 9 comments · Fixed by #762
Labels
bug Something isn't working
Milestone

Comments

@AnthonyGlt
Copy link
Contributor

Description

Not exactly the same as #728
I've noticed that after moving the camera around, we start doing the same requests again and again, infinitely.

Could be link to the latest changes concerning the cache management, WDYT ?

To Reproduce

Steps to reproduce the behavior:

  1. Load the tileset from https://github.com/AnthonyGlt/data/tree/main/arbres_rieux_glb
  2. Rotate / Move the camera around
  3. Check the network request
Screencast.from.23-09-2024.17.59.56.webm
@AnthonyGlt AnthonyGlt added the bug Something isn't working label Sep 25, 2024
@gkjohnson gkjohnson added this to the v0.3.38 milestone Sep 25, 2024
@gkjohnson
Copy link
Contributor

Thanks - I think it's due to this section:

// if the cache is full due to newly loaded memory then lets discard this tile - it will
// be loaded again later from the disk cache if needed.
if ( lruCache.isFull() ) {
lruCache.remove( tile );
} else {
lruCache.updateMemoryUsage( tile );
}

Which will unload a newly loaded tile if it turns out the memory used by tiles recently loaded in parallel have already pushed the cache over the threshold. The issue is that sometimes the tiles are marked as taking no memory (tile subtrees) are being unnecessarily unloaded since they won't push the memory cache any further. And tiles that have had memory requirements previously cached (and therefore already accounted for) are being unloaded even though they're contributing to the fullness factor already. So we wind up in a loop where tiles are loaded, the cache is full, the tile that caused it to be full is unloaded, causing the cache to be unfull, and a new load is started.

The following should fix it and hopefully be robust:

  • Don't perform the check if tiles are marked as using no memory and just keep them in the cache.
  • Check if the tile memory is "unknown" by the LRUCache and the tile is therefore unaccounted for in the "fullness" before deciding to evict the tile after load.

This memory cache has turned out to be complicated - but I think it's a good feature to have. Thanks for reporting.

@AnthonyGlt
Copy link
Contributor Author

Thank you for the explanation !
It's very interesting to understand what is happening under the hood 👍

What do you mean by

the tile memory is "unknown" by the LRUCache

Does it concern the newly loaded tiles ? Or like as you referred to it before, the subtrees which have no memory ?

The cache management looks tricky but you're right, it is a good feature :)

@gkjohnson
Copy link
Contributor

the tile memory is "unknown" by the LRUCache

We can only know how much memory a tile will take up once it has been loaded so when we start loading for the first time the final memory footprint it "unknown" (treated as 0) but we optimistically add it in into the cache anyway assuming it's not already full. Once the tile is loaded we estimate the bytes required here and then we update the memory usage in the cache here. Of course since multiple "unknown" tiles are loading at once then it's possible we overloaded the cache since we didn't know how much memory would required. In this case we dispose of a newly loaded tile if the cache was filled by other unknown-size tiles while it was being loaded.

If the tile is unloaded and loaded again then we can know ahead of time the memory required and account for it to avoid loading other tiles just to unload them.

the subtrees which have no memory ?

The cache is more realistically measuring the GPU memory requirements. Of course the subtrees use CPU memoy but I think this is less sensitive. So they are just marked as "0" which should mean they're "free" as far as the memory requirements are concerned.

@gkjohnson
Copy link
Contributor

Load the tileset from https://github.com/AnthonyGlt/data/tree/main/arbres_rieux_glb

You may know this, already, but it's worth mentioning that the tile geometry in this tile set are very large. Update to 14 mb per tile in some cases. This is going to cause data to load more slowly and fill up the cache very quickly. It's fortunate that it helped find this bug, though 😁

@AnthonyGlt
Copy link
Contributor Author

Yes I've noticed it. It's good for testing :)

@AnthonyGlt
Copy link
Contributor Author

AnthonyGlt commented Sep 30, 2024

Is that why the all the tiles don't load ? We stop loading once the cache is full I guess.
Screenshot from 2024-09-30 10-13-51
image

@gkjohnson
Copy link
Contributor

gkjohnson commented Sep 30, 2024

Yeah that's right - once the cache is full no more tiles will load. Ideally you won't be reaching the cache cap, though. I believe Cesium has a setting that will progressively raise the error target if the cache is full to avoid these kinds of situations, which we could do here (see #695).

edit
See the Cesium implementation here.

@AnthonyGlt
Copy link
Contributor Author

Mmh interesting 🤔

@gkjohnson
Copy link
Contributor

You can also adjust the cache to allow for more memory usage, as well. Right now the max cap defaults to 0.4 GB or ~400MB which seemed reasonable for something like mobile devices in addition to whatever else needs to be rendered. If you know you'll have more headroom you can raise the cap.

And to be clear this won't always result in gaps in the tile set. The traversal algorithm will only render a set of child tiles if all children have been loaded when using the "REPLACE" refinement. In that case you'll just wind up with parent tiles that don't refine because the children won't load.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants