Level paging

[timoore]

Description

Issue number or link

Author checklist

• I have submitted a Contributor License Agreement (only needed once).
• I have done a full self-review of my code.
• I have updated CHANGES.md with a short summary of my change (for user-facing changes).
• I have added or updated unit tests to ensure consistent code coverage as necessary.
• I have updated the documentation as necessary.

Remaining Tasks

Unit tests

Testing plan

Reviewer checklist

Thank you for taking the time to review this PR. By approving a PR you are taking as much responsibility for these changes as the author.

As you review, please go through the checklist below:

• Review and run all parts of the test plan on this branch and verify it matches expectations.
  • If the issue is a bug please make sure you can reproduce the bug in the main branch and then checkout this branch to make sure it actually solved the issue.
• Review the code and make sure you do not have any remaining questions or concerns. You should understand the code change and the chosen approach. If you are not confident or have doubts about the code, please do not hesitate to ask questions.
  • Code should follow the C++ Style Guide
• Review the unit tests and make sure there are no missing tests or edge cases.
• Review documentation changes and updates to CHANGES.md to make sure they accurately cover the work in this PR.
• Verify that the Contributor License Agreement has been submitted, if needed.

[j9liu]

Thanks for the PR @timoore!

It's true that we should move towards a generalizable criteria for tile selection. However, I think this interface still needs some development before we can truly release it. As it stands, the simple geometric error approach introduced in #1435 is now being convoluted by an "error measure" function that could potentially replace the geometric error computation, even though it is being used in the same logical paths throughout tile selection.

Instead of requiring computeErrorMeasure and meetsErrorThreshold to be implemented, it would be cleaner and more efficient to implement a shouldRefineTile. This function could expose ViewState, Tile info, etc. to allow for more flexible refinement logic. The default behavior would be to compute the SSE of a tile.

However, the goal of this feature was to implement level-of-detail selection based on the depth of the tree. As we had previously discussed offline, the cleanest approach with our current architecture would be to put this as a parameter on TilesetOptions, then to check for the depth throughout the tile selection code. Unless we're confident within the time constraints that we could do this interface right (which personally, I am not), we should pivot to the simpler approach. We can always come back to abstracting this interface later.

[timoore]

Thanks for the PR @timoore!

However, the goal of this feature was to implement level-of-detail selection based on the depth of the tree. As we had previously discussed offline, the cleanest approach with our current architecture would be to put this as a parameter on TilesetOptions, then to check for the depth throughout the tile selection code. Unless we're confident within the time constraints that we could do this interface right (which personally, I am not), we should pivot to the simpler approach. We can always come back to abstracting this interface later.

I went with attaching this property to the view instead of the tileset because, it seems, changing it requires reloading the tileset in the current Cesium Native and and client implementations. Maybe this isn't a big deal -- changing the screen space error already requires a reload -- but changing these level properties, geometric error, area for paging etc. is something that should be fast to execute.

[j9liu]

changing it requires reloading the tileset in the current Cesium Native and and client implementations

@timoore This isn't true of Unreal. ACesium3DTileset::updateOptionsFromTilesetProperties allows configuring of cesium-native TilesetOptions without a tileset reload.

This is true of Unity, but it's because of our lack of implementation. CesiumGS/cesium-unity#412 was opened to address this, but we never got the signed CLA to review and merge.

[timoore]

changing it requires reloading the tileset in the current Cesium Native and and client implementations

@timoore This isn't true of Unreal. ACesium3DTileset::updateOptionsFromTilesetProperties allows configuring of cesium-native TilesetOptions without a tileset reload.

This is true of Unity, but it's because of our lack of implementation. CesiumGS/cesium-unity#412 was opened to address this, but we never got the signed CLA to review and merge.

Okay then! I really thought that the tileset options were immutable after tileset creation.

I'm not sure if moving the level check into the rest of the tileset selection code really simplifies things: the level is completely equivalent to a tile geometric error, and is in fact the same thing for implicit tilesets. I'm also not sure about the interface presented to the clients of Cesium Native. I will take another look at this.

[j9liu]

the level is completely equivalent to a tile geometric error

They are related mathematically, but I don't believe this should manifest in the public API.

The related feature requests (CesiumGS/cesium-unreal#1811 and #1143, to name a few) imply control over which LOD is chosen. Although geometric error correlates to LOD, something like geometricErrorThreshold or desiredGeometricError makes it hard to estimate to get the LOD you want, without digging into the details of a tileset.

The easiest way to identify LODs, without regard for a tileset's internal organization, would be an integer level. If we allow users to specify an integer level on the TilesetOptions in some way, then there's more freedom to do what we need under-the-hood, even if it means we convert back to geometric error. We should keep in mind that not all tilesets are implicit, and some may have varying tree depth / geometric error that we don't anticipate.

Feel free to propose alternatives but I think this is easiest, especially to meet the October release milestone.

[timoore]

Here's a summary of the offline meeting attended by @j9liu, @azrogers, @laurenfrederick, and myself. We decided that the current approach is fine for short-term needs to get new functionality into Cesium Native, but probably needs more work for a long-term solution. The long-term issue is making explicit the interaction of a ViewState with ViewGroup objects. Multiple view states can be passed to a view group for selection, but the "most detailed" tile always wins and is returned in the resulting list of tiles, so an error measure predicate might end up being a no-op.

We talked through how the measure handler supports tile selection by depth in the tileset and agreed that tests and documention are needed to illustrate this for a user. These are incoming.

I explained that the tileset selection algorithm relies on some kind of value that can be computed, compared, and then passed down in when recursively visiting tiles, and that it makes sense to keep using a double for this until we understand the algorithm better.

The structure of the current code tries to keep the existing SSE path fast while allowing novel selection methods to be specified by the Cesium Native user. It would be good to verify that the existing algorithm can be implemented completely with error measure handler; it might be necessary to pass the ViewState object to the handler as an additional argument to really make that possible. Again, more tests and documentation will make that clearer.

[j9liu]

Thanks @timoore for the writeup. I also remember that we agreed to replace ViewState's geometricErrorThreshold entirely with the new ErrorMeasureHandler, because the latter makes it redundant. Is that correct?

[timoore]

Thanks @timoore for the writeup. I also remember that we agreed to replace ViewState's geometricErrorThreshold entirely with the new ErrorMeasureHandler, because the latter makes it redundant. Is that correct?

Yup, that's right! Thanks for noting that.

[j9liu]

Thanks @timoore ! This looks a lot cleaner, and I have no functional feedback. However, there's some doc suggestions and a few naming changes just so we're precise about what we mean by "error" around the code. This will help with with future maintainability.

[j9liu]

If _errorMeasureHandler is defined then this is no longer screen space error we're computing, so this is somewhat dishonest. Could we rename for computeTileError or something similar?

[timoore]

I've held off on this one only because it's the name of the function called in the tileset selection algorithm, even if "error" is now more general than screen space error. Maybe computeTileError is the thing.