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 tile selection bug, add some helpful tools for debugging #1032

Merged
merged 5 commits into from
Dec 13, 2024

Conversation

kring
Copy link
Member

@kring kring commented Dec 9, 2024

Fixes #1028

The sketchiness described in that issue was a legitimate bug in the tile selection algorithm. Here's the scenario:

  1. Frame 1: We start off zoomed in, looking a level 15 tile (among others). Because "Preload Ancestors" is disabled (or we just haven't waited long enough), there are no less-detailed tiles loaded that cover the area of this level 15 tile.
  2. Frame 2: The user zooms out, so now we'd ideally select that level 15 tile's parent, a level 14 tile. We start the process to download it, but it's not available yet. So we continue showing the level 15 tile (thanks to ancestorMeetsSse).
  3. Frame 3: The user zooms out more, so now we'd ideally select the level 14 tile's parent, at level 13. We start downloading that. We're still showing the level 15 tile.
  4. Frame 4: The load of the level 14 tiles completes. So we select this tile for rendering instead of the previously-selected level 15 tile.
  5. Still in Frame 4: While in _visitTile for the level 13 tile, we execute this code after traversing children, including our selected-for-rendering level 14:
bool kickDueToNonReadyDescendant =
    !traversalDetails.allAreRenderable &&
    !traversalDetails.anyWereRenderedLastFrame;

And this evaluates to true. So the level 14 gets kicked out, and the level 13 is selected instead. Only the level 13 isn't even loaded yet, so we just made detail (the level 15 tile) disappear. Wait, why does this evaluate to true?

Well, allAreRenderable is false because some of the level 13 tile's children are loaded and some are not. We're zooming out with a very incomplete tile tree, so it's expected that there will be some tiles missing around the edges. No problem here.

anyWereRenderedLastFrame is also false because this is the first time we're rendering the level 14 tile. So that's accurate, too. But hang on a minute... even though we've never rendered this level 14 tile before, we have rendered the level 15 tile in the same location. So there were tiles rendered last frame in this space. The value of this flag should be true.

So this PR ensures that it is. It works by tweaking how the TraversalDetails are computed for a single tile. Previously, it would only set anyWereRenderedLastFrame to true if the single tile's last frame state was Renderable. Now, it will also check if the last frame state was Refined, and, if so, it will check to see if its children were rendered last frame. This check is recursive as necessary, in case one of the children was also refined last frame.

In addition to the above bug fix, this PR:

  • Adds a const version of Tileset::forEachLoadedTile.
  • Adds DebugTileStateDatabase class.
  • Adds CesiumAsync::SqliteHelper, containing functions for working with SQLite.

The DebugTileStateDatabase is a handy class to help debug tile selection problems by logging the state of every tile to a SQLite database every frame. I'll open a separate cesium-unreal PR soon that demonstrates how to use this. I found it really helpful to track down this problem.

@j9liu j9liu self-requested a review December 11, 2024 14:49
Copy link
Contributor

@j9liu j9liu left a comment

Choose a reason for hiding this comment

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

Thanks @kring ! I'm not familiar with SQLite so I couldn't give much feedback on those sections, but the rest seems to make sense.

Just caught a few typos, but nothing major. Also, is there any particular case I should use to test this?

Cesium3DTilesSelection/src/Tileset.cpp Outdated Show resolved Hide resolved
CesiumAsync/include/CesiumAsync/SqliteHelper.h Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
@kring
Copy link
Member Author

kring commented Dec 11, 2024

Thanks for the review @j9liu!

is there any particular case I should use to test this?

Did you see the corresponding PR in cesium-unreal?
CesiumGS/cesium-unreal#1568

@kring
Copy link
Member Author

kring commented Dec 11, 2024

Oh hang on I thought you were asking about testing the new debugging tools. You're asking about how to test the bug. Let me get back to you with instructions on reproducing that.

@kring
Copy link
Member Author

kring commented Dec 11, 2024

The bug can be reproduced in Unreal using the instructions at the top of #1028. Start with the first level of the Samples project, and make the following changes:

  • Add a DebugColorizeTilesRasterOverlay to Cesium World Terrain. Designate it Overlay1.
  • Turn off the two "Preload" options.
  • Set Maximum Simultaneous Tile Loads to 1.

Zoom in close anywhere on the globe, looking straight down. Press Ctrl-1 to save this view, save the level, and exit Unreal. Delete the request cache, which is found at %userprofile%\AppData\Local\UnrealEngine\5.2\cesium-request-cache.sqlite or similar. Start up Unreal and record a video using the game bar or whatever. The tips in #1568 will help. Zoom directly away from the Earth while continuing to look straight down.

Now watch the video carefully. As you zoom out, newly-exposed areas will initially be blank, and that's fine. But you should never see a spot that used to be covered by a tile suddenly turn blank. That's the bug. It should happen pretty much every time in main, and not at all with this PR.

@j9liu
Copy link
Contributor

j9liu commented Dec 13, 2024

Thanks @kring ! Weirdly I can't reproduce the error that you're describing on main, but I also don't have access to Xbox gamebar on my laptop... so maybe it's picking up something that OBS can't.

But I'll take your word for it! At least the branch doesn't make anything worse. 😛 Going to merge after CI passes

@j9liu j9liu merged commit 9f41a38 into main Dec 13, 2024
22 checks passed
@j9liu j9liu deleted the any-were-rendered branch December 13, 2024 16:36
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.

Tile selection sketchiness
2 participants