Extract `Tile` selection algorithms from `Tileset` into free functions by calebbuffa · Pull Request #1342 · CesiumGS/cesium-native

calebbuffa · 2026-04-09T21:17:49Z

Description

This PR breaks tile selection algorithms out of private methods in the Tileset class and into free functions. The purpose is to reduce the surface area of Tileset and make the selection algorithms independently testable.

Changes

1) — `tileStateUpdater` callback: decouple traversal from loading

The traversal helpers (_visitTileIfNeeded, etc.) previously called _pTilesetContentManager->updateTileContent() directly, creating a compile-time dependency on TilesetContentManager inside the selection algorithm. A std::function<void(Tile&)> tileStateUpdater field was added to TilesetFrameState. Tileset::updateViewGroup injects the implementation via a lambda; traversal calls the callback. The traversal has no #include dependency on TilesetContentManager.

Files: TilesetFrameState.h, Tileset.cpp

2) — `selectTiles()`: pure free function for tile LOD selection

The entire traversal algorithm (_visitTileIfNeeded, _visitTile, _renderLeaf, _renderInnerTile, _visitVisibleChildrenNearToFar, _kickDescendantsAndRenderTile, _loadAndRenderAdditiveRefinedTile, frustum/fog cull, occlusion check, SSE computation) was extracted from Tileset private methods into free functions in a new translation unit TilesetSelection.cpp. The public entry point is:

ViewUpdateResult selectTiles(
    const TileSelectionContext& ctx,   // options + externals
    const TilesetFrameState& frameState,
    Tile& rootTile);

Tileset::updateViewGroup is now a thin coordinator: it builds frameState, calls selectTiles(), and the result is returned directly. The private traversal method declarations and TraversalDetails/CullResult structs are gone from Tileset.h.

Files: TilesetSelection.h (new), TilesetSelection.cpp (new), Tileset.h, Tileset.cpp

Issue number or link

N/A.

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.

Testing plan

Regression: All existing tests in TestTilesetSelectionAlgorithm.cpp exercise the full selection pipeline via Tileset::updateViewGroup, which now delegates to selectTiles(). These cover: replace/additive refinement, SSE thresholds, frustum/fog culling, multi-frustum, forbidHoles, LOD transition fade-out, unconditionallyRefine.

New isolation test (TestTilesetSelection.cpp):

selectTiles is callable as a free function — constructs a minimal EllipsoidTilesetLoader-backed tileset, calls selectTiles() directly (bypassing Tileset::updateViewGroup), and asserts at least one tile is selected with no tiles kicked.
selectTiles result matches updateViewGroup result — runs both updateViewGroup and selectTiles() on the same warmed-up tileset and asserts tile render count and visit count are identical between the two paths.

Build verification: cmake --build build --target Cesium3DTilesSelection succeeds with zero errors.

Remaining Tasks

Things that could be done:

TilesetViewGroup::startNewFrame/finishFrame still take const Tileset&, which prevents fully async-free unit tests of selectTiles. Decoupling that is a natural follow-up once this PR lands.

…error

kring

Thanks for the PR @calebbuffa!

I love the way you've broken the tileset selection algorithm into a separate, testable function.

Some of the other changes seem mostly unrelated to this important core, though, and I have some quibbles with them. See below.

kring · 2026-04-13T07:21:28Z

+ *
+ * Tiles at the head are least-recently-used; tiles at the tail are
+ * most-recently-used. All methods must be called from the main thread.
+ * [main-thread-only]


I think this class plays by our thread safety rules just fine. There's no need to further restrict it to "main thread only".
https://cesium.com/learn/cesium-native/ref-doc/thread-safety-rules.html

… into ownership

calebbuffa · 2026-04-13T14:27:29Z

@kring thank you for the feedback! I got a little too carried away. I stripped the changes just to selectTiles and the TilesUnloadQueue. Thanks for refocusing!

kring · 2026-04-20T02:01:37Z

I asked Copilot to review this PR, specifically looking for any changes that were inadvertently introduced when
moving code from Tileset.cpp to TilesetSelection.cpp. Both observations look correct to me. However, I suggest moving the orphaned comment into the new file instead of deleting it.

I reviewed the code moved from Tileset.cpp to TilesetSelection.cpp for inadvertent changes. The core traversal logic appears to be a faithful move — no inadvertent behavioral differences found.

Two issues:

1. Orphaned doc comment in Tileset.h

When _loadAndRenderAdditiveRefinedTile was removed, its full /** ... */ doc comment was accidentally left behind. It now sits immediately before _unloadCachedTiles, making it appear to document that function:

private:
  /**
   * @brief When called on an additive-refined tile, queues it for load and adds
   * it to the render list.
   * ...
   * @return false The non-additive-refined tile was ignored.
   */
  // ← _loadAndRenderAdditiveRefinedTile declaration was deleted, but this comment wasn't

  void _unloadCachedTiles(double timeBudget) noexcept;

This happened because the deletion straddled a diff hunk boundary — the /** and @brief lines fell in the first hunk, the closing @param/@return/*/ lines in the second. The function declaration was correctly deleted but the entire comment body survived as context lines in both hunks.

Fix: delete the orphaned /** ... */ block from Tileset.h.

2. Unnecessary const_cast introduced in checkOcclusion

In TilesetSelection.cpp, the new checkOcclusion accesses ctx.childOcclusionProxies via a spurious const_cast:

auto& childOcclusionProxies =
    const_cast<std::vector<const TileOcclusionRendererProxy*>&>(
        ctx.childOcclusionProxies);

childOcclusionProxies is declared as std::vector<const TileOcclusionRendererProxy*>& (a non-const reference member) in TraversalContext. Reference members retain their mutability when accessed through a const struct reference in C++, so the cast is unnecessary. The original _checkOcclusion in Tileset.cpp modified this->_childOcclusionProxies directly without any cast.

Fix: remove the const_cast and use ctx.childOcclusionProxies directly.

kring

A few more comments. Other than this, this PR looks good to me! I'll let @azrogers or @j9liu merge it, though.

j9liu · 2026-04-22T20:28:41Z

Hi @calebbuffa! We're interested in getting this into the next release, are you able to address Kevin's comments above within the next few days?

calebbuffa · 2026-04-22T20:37:45Z

@j9liu yes! Sorry got a little sidetracked the last few days. I'll fix the PR by end of week!

j9liu · 2026-04-27T15:11:17Z

Thanks for the updates @calebbuffa! Just for transparency, I think I'll actually defer merging this to after our May 1 release if that's alright. We'll still review this soon, but it would help to have additional time to stress-test the changes, just in case there are any surprises in our runtimes.

Copilot

Pull request overview

This PR refactors the tileset LOD / tile-selection traversal by extracting it from Tileset private methods into a standalone selectTiles() free function, aiming to reduce Tileset surface area and make selection logic more directly testable.

Changes:

Introduces selectTiles() and TileSelectionContext as a public entry point for the selection traversal.
Adds TilesetFrameState::tileStateUpdater to decouple traversal from TilesetContentManager.
Refactors tile content eviction tracking behind a new TileUnloadQueue wrapper and adds new unit tests for the free-function selection path.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
Cesium3DTilesSelection/test/TestTilesetSelection.cpp	Adds unit tests that call `selectTiles()` directly and compare results to `Tileset::updateViewGroup`.
Cesium3DTilesSelection/src/TilesetSelection.cpp	New implementation of the extracted tile-selection / traversal algorithm.
Cesium3DTilesSelection/src/Tileset.cpp	Makes `updateViewGroup` a coordinator that builds frame state/context and calls `selectTiles()`.
Cesium3DTilesSelection/include/Cesium3DTilesSelection/TilesetSelection.h	New public API header defining `TileSelectionContext` and `selectTiles()`.
Cesium3DTilesSelection/include/Cesium3DTilesSelection/TilesetFrameState.h	Adds `tileStateUpdater` callback for per-visited-tile state advancement.
Cesium3DTilesSelection/include/Cesium3DTilesSelection/Tileset.h	Removes private traversal helpers and includes the new selection header.
Cesium3DTilesSelection/src/TilesetContentManager.h	Replaces raw unused-tile linked list member with `TileUnloadQueue`.
Cesium3DTilesSelection/src/TilesetContentManager.cpp	Switches eviction bookkeeping to `TileUnloadQueue` and adjusts member initialization.
Cesium3DTilesSelection/include/Cesium3DTilesSelection/TileUnloadQueue.h	Adds a small wrapper around `Tile::UnusedLinkedList` for LRU eviction tracking.
Cesium3DTilesSelection/include/Cesium3DTilesSelection/Tile.h	Minor doc wording tweak for `getParent()` comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-27T15:13:21Z

+TraversalDetails visitTileIfNeeded(
+    const TraversalContext& ctx,
+    uint32_t depth,
+    bool ancestorMeetsSse,
+    Tile& tile,
+    ViewUpdateResult& result);
+


visitTileIfNeeded is forward-declared here and then forward-declared again later in the file. Keeping only one forward declaration block (or reordering definitions) would reduce duplication and make the file easier to follow.

Not the end of the world, but I guess just remove the duplicate one that appears later 🤷

Copilot · 2026-04-27T15:13:21Z

+CESIUM3DTILESSELECTION_API void selectTiles(
+    const TileSelectionContext& ctx,
+    const TilesetFrameState& frameState,
+    Tile& rootTile,
+    ViewUpdateResult& result);


PR description shows selectTiles returning a ViewUpdateResult (and taking no result out-parameter), but the implemented public API is void selectTiles(..., ViewUpdateResult& result). Please update the PR description (or the API) so they match, to avoid confusing reviewers and downstream users reading the change log / docs.

Probably enough to leave an "OUTDATED" comment on the PR description, not the end of the world 🤷

Copilot · 2026-04-27T15:13:22Z

+ * @brief Runs the tile selection / LOD traversal algorithm.
+ *
+ * Populates `result` with the tiles to render this frame. The view group in
+ * `frameState` also receives load queue updates.
+ *
+ * @param ctx Configuration, external dependencies, and scratch buffers.
+ * @param frameState Per-frame view parameters and the view group to update.
+ * @param rootTile Root of the tile hierarchy to traverse.
+ * @param result Filled with the selected tiles and statistics for this frame.


Because selectTiles() mutates frameState.viewGroup traversal/load-queue state, the API doc should state the required call pattern/preconditions for external callers (e.g., TilesetViewGroup::startNewFrame must be called before and finishFrame after; and frameState.fogDensities must correspond 1:1 with frameState.frustums). Without this, it’s easy to call selectTiles() directly and hit assertions or UB.

Copilot · 2026-04-27T15:13:22Z

+  const auto& frustums = ctx.frameState.frustums;
+  const auto& fogDensities = ctx.frameState.fogDensities;
+  const auto& distances = ctx.distances;
+
+  bool isFogCulled = true;
+  for (size_t i = 0; i < frustums.size(); ++i) {
+    if (isVisibleInFog(distances[i], fogDensities[i])) {
+      isFogCulled = false;


fogCull indexes fogDensities[i] assuming frameState.fogDensities.size() == frameState.frustums.size(). Since selectTiles() is a public free function, callers can accidentally provide mismatched vectors, leading to out-of-bounds access in release builds. Add a CESIUM_ASSERT (or iterate up to min(frustums.size(), fogDensities.size(), distances.size())) to make the precondition explicit and prevent UB.

CESIUM_ASSERT isn't a bad idea here, since that internal assumption is no longer guaranteed outside of Tileset.cpp

… ownership

j9liu

Thank you for the work on this PR @calebbuffa! It might look like there's a lot of comments but I promise they're all minor.

The biggest note is that we lost a lot of helpful internal comments that we should preserve for future knowledge. Otherwise, it's just a matter of style / minor tweaks.

j9liu · 2026-04-27T15:17:10Z

+struct TraversalDetails {
+  bool allAreRenderable = true;
+  bool anyWereRenderedLastFrame = false;
+  uint32_t notYetRenderableCount = 0;
+};


I think it would still be valuable to preserve comments that were here when this was defined in Tileset.h (minus allusions to outdated implementation details).

j9liu · 2026-04-27T15:27:48Z

+ * @param ctx Configuration, external dependencies, and scratch buffers.
+ * @param frameState Per-frame view parameters and the view group to update.
+ * @param rootTile Root of the tile hierarchy to traverse.
+ * @param result Filled with the selected tiles and statistics for this frame.
+ */
+CESIUM3DTILESSELECTION_API void selectTiles(
+    const TileSelectionContext& ctx,


Hm, I can't find where we mention it in the style guide, but in general we avoid abbreviations for better code readability. Since this is public API, it's better to be overly explicit and write out everything unless truly cumbersome.

Suggested change

* @param ctx Configuration, external dependencies, and scratch buffers.

* @param frameState Per-frame view parameters and the view group to update.

* @param rootTile Root of the tile hierarchy to traverse.

* @param result Filled with the selected tiles and statistics for this frame.

*/

CESIUM3DTILESSELECTION_API void selectTiles(

const TileSelectionContext& ctx,

* @param context Configuration, external dependencies, and scratch buffers.

* @param frameState Per-frame view parameters and the view group to update.

* @param rootTile Root of the tile hierarchy to traverse.

* @param result Filled with the selected tiles and statistics for this frame.

*/

CESIUM3DTILESSELECTION_API void selectTiles(

const TileSelectionContext& context,

j9liu · 2026-04-27T17:20:47Z

+  void markEligible(Tile& tile) noexcept {
+    if (!_queue.contains(tile)) {
+      _queue.insertAtTail(tile);
+    }
+  }
+
+  /**
+   * @brief Removes `tile` from the queue. No-op if not present. [main-thread]
+   */
+  void markIneligible(Tile& tile) noexcept { _queue.remove(tile); }
+
+  /** @brief Returns true when `tile` is currently in the candidate list.
+   * [main-thread] */
+  bool contains(const Tile& tile) const noexcept {
+    return _queue.contains(tile);
+  }
+
+  /**
+   * @brief Directly removes `tile` from the list.
+   * Prefer markIneligible; this exists for sites that call remove()
+   * unconditionally. [main-thread]
+   */
+  void remove(Tile& tile) noexcept { _queue.remove(tile); }
+
+  /** @brief Returns the least-recently-used tile, or nullptr. [main-thread] */
+  Tile* head() noexcept { return _queue.head(); }
+
+  /** @brief Returns the tile following `tile` in LRU order. [main-thread] */
+  Tile* next(Tile& tile) noexcept { return _queue.next(tile); }


Per our style guide:

Use this-> when accessing member variables and functions. This helps distinguish member variables from local variables and member functions from anonymous namespace functions.

If we could this->_queue throughout the file that'd be great!

j9liu · 2026-04-27T17:24:37Z

  if (!frustums.empty()) {
    viewGroup.startNewFrame(*this, frameState);
-    this->_visitTileIfNeeded(frameState, 0, false, *pRootTile, result);
+    TileSelectionContext ctx{


Avoid abbreviations unless the full word(s) are truly cumbersome.

Suggested change

TileSelectionContext ctx{

TileSelectionContext context{

j9liu · 2026-04-27T17:26:30Z

+      result);
+}
+
+bool isVisibleFromCamera(


IMO it's valuable to keep the previous comments around, even for the more straightforward private functions.

Suggested change

bool isVisibleFromCamera(

/**

* @brief Returns whether a tile with the given bounding volume is visible for

* the camera.

*

* @param viewState The {@link ViewState}

* @param boundingVolume The bounding volume of the tile

* @param forceRenderTilesUnderCamera Whether tiles under the camera should

* always be considered visible and rendered (see

* {@link Cesium3DTilesSelection::TilesetOptions}).

* @return Whether the tile is visible according to the current camera

* configuration

*/

bool isVisibleFromCamera(

j9liu · 2026-04-27T18:53:00Z

+    return TileOcclusionState::NotOccluded;
+  }
+
+  const TileOcclusionRendererProxy* pOcclusion =


The internal comments in this function got lost in the move; could you restore them?

j9liu · 2026-04-27T18:54:27Z

+  const auto& frustums = ctx.frameState.frustums;
+  const auto& fogDensities = ctx.frameState.fogDensities;
+  const auto& distances = ctx.distances;
+
+  bool isFogCulled = true;
+  for (size_t i = 0; i < frustums.size(); ++i) {
+    if (isVisibleInFog(distances[i], fogDensities[i])) {
+      isFogCulled = false;


CESIUM_ASSERT isn't a bad idea here, since that internal assumption is no longer guaranteed outside of Tileset.cpp

j9liu · 2026-04-27T18:54:58Z

+
+// Frustum and fog culling
+
+void frustumCull(


This function has several internal comments that got lost in the move; could you restore them?

j9liu · 2026-04-27T18:59:28Z

+ * @param frameState Per-frame view parameters and the view group to update.
+ * @param rootTile Root of the tile hierarchy to traverse.
+ * @param result Filled with the selected tiles and statistics for this frame.
+ */


Maybe worth marking as @private API for now; not sure if access to this function might confuse users

Suggested change

*/

* @private

*/

j9liu · 2026-04-27T19:00:01Z

+ * Head is least-recently-used, tail is most-recently-used.
+ */


Maybe worth marking as @private API for now, might not have public API value for users.

Suggested change

* Head is least-recently-used, tail is most-recently-used.

*/

* Head is least-recently-used, tail is most-recently-used.

*

* @private

*/

calebbuffa added 9 commits April 9, 2026 13:59

call .get() to fix test

1b8fc8f

create MainThreadOnly and TileObserver

5eece01

create TileHiearchy and TileOverlaySystem

89bc16d

create TileUnloadQueue

c282c89

make tile state update a function callback

8b64f4b

create TileSelection

fa17459

tile selection delegates to free functions

aafcd22

update content calls to use .get()

f4c06d9

create free selectTiles test

836f709

calebbuffa marked this pull request as ready for review April 9, 2026 21:18

calebbuffa changed the title ~~Ownership~~ Improve Ownership Semantics Apr 9, 2026

calebbuffa added 2 commits April 10, 2026 06:37

remov unused makeCloseViewstate function to fix -Wunused-function ci …

32abaaa

…error

document undocumented public members

acb9a7d

calebbuffa mentioned this pull request Apr 10, 2026

Bug: TilesetOptions::enableFrustumCulling and enableFogCulling are irreversibly cleared when enableLodTransitionPeriod is set #1343

Closed

satisfy clang-tidy includes

9abf209

calebbuffa changed the title ~~Improve Ownership Semantics~~ Improve Ownership Semantics in Cesium3DTilesSelection Apr 10, 2026

Update comment TileOverlaySystem.h

cb0aac9

kring reviewed Apr 13, 2026

View reviewed changes

calebbuffa added 5 commits April 13, 2026 06:57

move selectTiles to top

1a576f8

revert unnecessary changes based on feedback

051f3ae

Merge branch 'ownership' of https://github.com/calebbuffa/cesium-native…

47205f4

… into ownership

remove [main-thread-only] comment

d51f930

revert _overlayCollection rename

1a9ddf5

calebbuffa changed the title ~~Improve Ownership Semantics in Cesium3DTilesSelection~~ Extract Tile selection algorithms from Tileset into free functions Apr 13, 2026

j9liu requested a review from kring April 16, 2026 19:23

j9liu added this to the May 2026 Release milestone Apr 17, 2026

kring reviewed Apr 20, 2026

View reviewed changes

Comment thread Cesium3DTilesSelection/src/Tileset.cpp Outdated

Comment thread Cesium3DTilesSelection/src/Tileset.cpp Outdated

Comment thread Cesium3DTilesSelection/src/Tileset.cpp Outdated

calebbuffa added 7 commits April 24, 2026 14:50

fix comments

792878b

update markIneligble call

e33a898

pass ViewUpdateResult instead of recreating in selectTiles

9eaca5c

format

d7a8b89

fix const cast

7d1de8e

fix orphaned comment

3e6c4eb

fix remaining issues

488eb2d

j9liu removed this from the May 2026 Release milestone Apr 27, 2026

j9liu requested review from Copilot and j9liu April 27, 2026 15:07

Copilot started reviewing on behalf of j9liu April 27, 2026 15:08 View session

Copilot AI reviewed Apr 27, 2026

View reviewed changes

Merge branch 'main' of https://github.com/CesiumGS/cesium-native into…

abded91

… ownership

j9liu requested changes Apr 27, 2026

View reviewed changes

j9liu added this to the June 2026 Release milestone Apr 28, 2026

-bool isVisibleFromCamera(
+/**
+ * @brief Returns whether a tile with the given bounding volume is visible for
+ * the camera.
+ *
+ * @param viewState The {@link ViewState}
+ * @param boundingVolume The bounding volume of the tile
+ * @param forceRenderTilesUnderCamera Whether tiles under the camera should
+ * always be considered visible and rendered (see
+ * {@link Cesium3DTilesSelection::TilesetOptions}).
+ * @return Whether the tile is visible according to the current camera
+ * configuration
+ */
+bool isVisibleFromCamera(

		* Head is least-recently-used, tail is most-recently-used.
		*/

Conversation

calebbuffa commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

1) — tileStateUpdater callback: decouple traversal from loading

2) — selectTiles(): pure free function for tile LOD selection

Issue number or link

Author checklist

Testing plan

Remaining Tasks

Uh oh!

kring left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

calebbuffa commented Apr 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kring commented Apr 20, 2026

Uh oh!

kring left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

j9liu commented Apr 22, 2026

Uh oh!

calebbuffa commented Apr 22, 2026

Uh oh!

j9liu commented Apr 27, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

j9liu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

calebbuffa commented Apr 9, 2026 •

edited

Loading

1) — `tileStateUpdater` callback: decouple traversal from loading

2) — `selectTiles()`: pure free function for tile LOD selection

calebbuffa commented Apr 13, 2026 •

edited

Loading