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

"Kick" only when the current tile is renderable #1042

Merged
merged 6 commits into from
Dec 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
- Fixed a raster overlay bug that could cause unnecessary upsampling with failed or missing overlay tiles.
- Fixed a bug in `SubtreeFileReader::loadBinary` that prevented valid subtrees from loading if they did not contain binary data.
- Fixed a bug in the `Tileset` selection algorithm that could cause detail to disappear during load in some cases.
- Improved the "kicking" mechanism in the tileset selection algorithm. The new criteria allows holes in a `Tileset`, when they do occur, to be filled with loaded tiles more incrementally.

### v0.42.0 - 2024-12-02

Expand Down
11 changes: 10 additions & 1 deletion Cesium3DTilesSelection/src/Tileset.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1430,7 +1430,16 @@ Tileset::TraversalDetails Tileset::_visitTile(
lastFrameSelectionResult == TileSelectionState::Result::Rendered &&
pRenderContent && pRenderContent->getLodTransitionFadePercentage() < 1.0f;

if (kickDueToNonReadyDescendant || kickDueToTileFadingIn) {
// Only kick the descendants of this tile if it is renderable, or if we've
// exceeded the loadingDescendantLimit. It's pointless to kick the descendants
// of a tile that is not yet loaded, because it means we will still have a
// hole, and quite possibly a bigger one.
bool wantToKick = kickDueToNonReadyDescendant || kickDueToTileFadingIn;
bool willKick = wantToKick && (traversalDetails.notYetRenderableCount >
this->_options.loadingDescendantLimit ||
tile.isRenderable());

if (willKick) {
// Kick all descendants out of the render list and render this tile instead
// Continue to load them though!
queuedForLoad = _kickDescendantsAndRenderTile(
Expand Down
63 changes: 39 additions & 24 deletions Cesium3DTilesSelection/test/TestTilesetSelectionAlgorithm.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -563,31 +563,32 @@ TEST_CASE("Test additive refinement") {
SECTION("Load external tilesets") {
ViewState viewState = zoomToTileset(tileset);

// 1st frame. Root will get rendered first and 5 of its children are loading
// since they meet sse
// 1st frame. Root, its child, and its four grandchildren will all be
// rendered because they meet SSE, even though they're not loaded yet.
Comment on lines +566 to +567
Copy link
Contributor

Choose a reason for hiding this comment

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

I see "render" getting used even when tiles aren't loaded in yet -- would it be more accurate to say tiles get "selected" vs. "rendered"?

Though, it seems like this verbiage is built into the API already (tilesToRenderThisFrame), so maybe it's better to stick with "render". But I hope our documentation clarifies that render ~= selection, and render != content show up on screen 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah this is a good point. It's a bit of a quirk of this whole system. Arguably, tiles that are not renderable should never be selected/rendered, in general. That would make sense, and be least likely to surprise people. My thought process when I went the other way was that - just maybe! - it could be useful to know which tiles would be rendered if they were loaded. For example, maybe you could render a grey cloudy thing in the space of their bounding box, and that would be better than a hole.

So whether this is a good idea or not, Cesium Native currently refers to selection as "rendering", and the rendered tiles aren't necessarily actually renderable. Changing this might be an excellent idea, but I think it's outside the scope of this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

But I hope our documentation clarifies that render ~= selection, and render != content show up on screen

I think it probably makes that apparent, but doesn't say so explicitly.

{
ViewUpdateResult result = tileset.updateView({viewState});

// root is rendered first
const std::vector<Tile*>& ttr = result.tilesToRenderThisFrame;
REQUIRE(ttr.size() == 7);

REQUIRE(root->getState() == TileLoadState::Done);
REQUIRE(!doesTileMeetSSE(viewState, *root, tileset));
REQUIRE(root->getChildren().size() == 1);
REQUIRE(std::find(ttr.begin(), ttr.end(), pTilesetJson) != ttr.end());
REQUIRE(std::find(ttr.begin(), ttr.end(), root) != ttr.end());

// root's children don't have content loading right now, so only root get
// rendered
const Tile& parentB3DM = root->getChildren().front();
REQUIRE(parentB3DM.getState() == TileLoadState::ContentLoading);
REQUIRE(!doesTileMeetSSE(viewState, parentB3DM, tileset));
REQUIRE(parentB3DM.getChildren().size() == 4);
REQUIRE(std::find(ttr.begin(), ttr.end(), &parentB3DM) != ttr.end());

for (const Tile& child : parentB3DM.getChildren()) {
REQUIRE(child.getState() == TileLoadState::ContentLoading);
REQUIRE(doesTileMeetSSE(viewState, child, tileset));
REQUIRE(std::find(ttr.begin(), ttr.end(), &child) != ttr.end());
}

REQUIRE(result.tilesToRenderThisFrame.size() == 1);
REQUIRE(result.tilesToRenderThisFrame.front() == pTilesetJson);

REQUIRE(result.tilesFadingOut.size() == 0);

REQUIRE(result.tilesVisited == 7);
Expand All @@ -600,20 +601,26 @@ TEST_CASE("Test additive refinement") {
{
ViewUpdateResult result = tileset.updateView({viewState});

// root is rendered first
const std::vector<Tile*>& ttr = result.tilesToRenderThisFrame;
REQUIRE(ttr.size() == 8);

// root is done loading and rendered.
REQUIRE(root->getState() == TileLoadState::Done);
REQUIRE(!doesTileMeetSSE(viewState, *root, tileset));
REQUIRE(root->getChildren().size() == 1);
REQUIRE(std::find(ttr.begin(), ttr.end(), root) != ttr.end());

// root's children don't have content loading right now, so only root get
// rendered
// root's child is done loading and rendered, too.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// root's child is done loading and rendered, too.
// root's children are done loading and rendered, too.

Copy link
Member Author

Choose a reason for hiding this comment

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

There's only one in this case though, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh sorry, I got confused by the lines below this (REQUIRE(parentB3DM.getChildren().size() == 4);) and thought the comment was referring to this check. Nevermind 🙂

const Tile& parentB3DM = root->getChildren().front();
REQUIRE(parentB3DM.getState() == TileLoadState::Done);
REQUIRE(!doesTileMeetSSE(viewState, parentB3DM, tileset));
REQUIRE(parentB3DM.getChildren().size() == 4);
REQUIRE(std::find(ttr.begin(), ttr.end(), &parentB3DM) != ttr.end());

for (const Tile& child : parentB3DM.getChildren()) {
// parentB3DM's children are all done loading and are rendered.
REQUIRE(child.getState() == TileLoadState::Done);
REQUIRE(std::find(ttr.begin(), ttr.end(), &child) != ttr.end());

if (*std::get_if<std::string>(&child.getTileID()) !=
"tileset3/tileset3.json") {
Expand All @@ -622,21 +629,20 @@ TEST_CASE("Test additive refinement") {
// external tilesets get unconditionally refined
REQUIRE(root->getUnconditionallyRefine());

// expect the children to meet sse and begin loading the content
// expect the children to meet sse and begin loading the content,
// while also getting rendered.
REQUIRE(child.getChildren().size() == 1);
REQUIRE(
doesTileMeetSSE(viewState, child.getChildren().front(), tileset));
REQUIRE(
child.getChildren().front().getState() ==
TileLoadState::ContentLoading);
REQUIRE(
std::find(ttr.begin(), ttr.end(), &child.getChildren().front()) !=
ttr.end());
}
}

REQUIRE(result.tilesToRenderThisFrame.size() == 3);
REQUIRE(result.tilesToRenderThisFrame[0] == pTilesetJson);
REQUIRE(result.tilesToRenderThisFrame[1] == root);
REQUIRE(result.tilesToRenderThisFrame[2] == &parentB3DM);

REQUIRE(result.tilesFadingOut.size() == 0);

REQUIRE(result.tilesVisited == 8);
Expand Down Expand Up @@ -1554,21 +1560,30 @@ void runUnconditionallyRefinedTestCase(const TilesetOptions& options) {
pRawLoader->createRootTile(),
options);

// On the first update, we should render the root tile, even though nothing is
// loaded yet.
// On the first update, we should refine down to the grandchild tile, even
// though no tiles are loaded yet.
initializeTileset(tileset);
const Tile& child = tileset.getRootTile()->getChildren()[0];
const Tile& grandchild = child.getChildren()[0];
CHECK(
tileset.getRootTile()->getLastSelectionState().getResult(
tileset.getRootTile()->getLastSelectionState().getFrameNumber()) ==
TileSelectionState::Result::Refined);
CHECK(
child.getLastSelectionState().getResult(
tileset.getRootTile()->getLastSelectionState().getFrameNumber()) ==
TileSelectionState::Result::Refined);
CHECK(
grandchild.getLastSelectionState().getResult(
tileset.getRootTile()->getLastSelectionState().getFrameNumber()) ==
TileSelectionState::Result::Rendered);

// On the third update, the grandchild load will still be pending.
// But the child is unconditionally refined, so we should render the root
// instead of the child.
// After the third update, the root and child tiles have been loaded, while
// the grandchild has not. But the child is unconditionally refined, so we
// can't render that one. Therefore the root tile should be rendered, after
// the child and grandchild are kicked.
initializeTileset(tileset);
initializeTileset(tileset);
const Tile& child = tileset.getRootTile()->getChildren()[0];
const Tile& grandchild = child.getChildren()[0];
CHECK(
tileset.getRootTile()->getLastSelectionState().getResult(
tileset.getRootTile()->getLastSelectionState().getFrameNumber()) ==
Expand Down