Fixing refinement of mixed empty and non-empty tiles

[weegeekps]

Description

Adds a short circuit in executeEmptyTraversal for when descendants may not be loaded, but the tile may have empty or implicit content. This resolves some selection issues with implicit tilesets.

We believe this is the root cause for several instances where the wrong tiles are being selected with implicit tilesets. With the release of the Reality Tiler, we're seeing this more and more often, with bugs similar to: https://github.com/CesiumGS/tilers/issues/1297

Issue number and link

Fixes #9356.

Testing plan

I tested a variety of scenarios and tilesets, including but not excluding:

• The private tileset that triggered this issue.
• Google Photorealistic Tiles
• San Francisco Aerometrex
• Cesium World Terrain
• Cesium World Bathemetry

Author checklist

• I have submitted a Contributor License Agreement
• I have added my name to CONTRIBUTORS.md
• I have updated CHANGES.md with a short summary of my change
• I have added or updated unit tests to ensure consistent code coverage
• I have update the inline documentation, and included code examples where relevant
• I have performed a self-review of my code

[weegeekps]

@ggetz So, @lilleyse and I took a closer look at the test failures for replacement refinement - selects upwards when traversal stops at empty tile and replacement refinement - selects root when sse is not met and subtree is not refinable (1). In both cases we think we can delete those tests as the behavior they're testing against is actually what we want in these cases.

Also, the main dataset for testing this is private. Anyone internal please just ask me for the data and I can provide a sandcastle with it.

[lilleyse]

I think this should be root.hasEmptyContent || allDescendantsLoaded.

Which means that traversal is allowed to stop at empty tiles but is not allowed to stop at external tilesets and implicit subtrees since they are placeholder tiles that aren't really part of the tileset structure.

cesium-native accomplishes this by setting their geometric error to infinity.

[weegeekps]

Yeah, that makes sense. I tested it out against the problem dataset and Google and everything works as expected. Pushing up the change now.

[lilleyse]

If a placeholder tile has an empty child we're going to see the same problems that came up in #9356.

To fix that I think another line should be changed:

const traverse = (tile.hasTilesetContent || tile.hasImplicitContent) && canTraverse(tile);

In other words, stop traversal if an empty tile is hit, but continue if an external tileset or implicit subtree is hit.

Also, the emptyLeaf code can be removed. It's no longer necessary.

[lilleyse]

I think there could be a cleaner way to do this and scrap executeEmptyTraversal, but I'm not exactly sure what that would look like.

[lilleyse]

Maybe final version of the function? I made a couple other tweaks:

function executeEmptyTraversal(root, frameState) {
  const {
    canTraverse,
    updateTile,
    loadTile,
    touchTile,
  } = Cesium3DTilesetTraversal;
  let allDescendantsLoaded = true;
  const stack = emptyTraversal.stack;
  stack.push(root);

  while (stack.length > 0) {
    emptyTraversal.stackMaximumLength = Math.max(
      emptyTraversal.stackMaximumLength,
      stack.length
    );

    const tile = stack.pop();
    const children = tile.children;
    const childrenLength = children.length;

    // Only traverse if the tile is an external tileset or implicit subtree
    const traverse = (tile.hasTilesetContent || tile.hasImplicitContent) && canTraverse(tile);

    // Traversal stops but the external tileset or implicit subtree hasn't loaded yet
    // There will be holes if the parent tries to refine to its children, so don't refine
    if (!traverse && !tile.contentReady) {
      allDescendantsLoaded = false;
    }

    updateTile(tile, frameState);
    if (!tile.isVisible) {
      // Load tiles that aren't visible since they are still needed for the parent to refine
      loadTile(tile, frameState);
      touchTile(tile, frameState);
    }

    if (traverse) {
      for (let i = 0; i < childrenLength; ++i) {
        const child = children[i];
        stack.push(child);
      }
    }
  }

  return root.hasEmptyContent || allDescendantsLoaded;
}

[ggetz]

@lilleyse I'm seeing holes in the Google tileset with this approach.

[weegeekps]

I'm seeing the same thing as Gabby. The problem is the change to the initialization of const traverse.

I do think we can get rid of the emptyLeaf exception. It doesn't appear that is needed any longer according to my testing. Let me run the unit tests locally to confirm and I'll push up that change in a few moments.

[weegeekps]

@ggetz @lilleyse When you two get a chance, please take a look at my latest push. It works pretty well in my testing and all of the tests pass with @ggetz's modifications.

[lilleyse]

Sorry I haven't had a chance to get back to this.

I probably wont have a chance to debug what's wrong with my code, so I'd be ok with proceeding with @weegeekps's changes. Let's just watch out for bugs where the root tile of an external tileset or implicit subtree is empty since I don't think that's being properly handled.

[ggetz]

Thanks @lilleyse! We're happy to review any further changes here. Let's proceed given @weegeekps's changes are a noticeable improvement on the existing behavior.

[ggetz]

@weegeekps @lilleyse I'm seeing good results in my testing with this version. Happy to merge assuming @lilleyse does not have any additional feedback.

[ggetz]

Thanks @weegeekps and @lilleyse !