TilesRenderer: Move "traversal" fields into a dedicated subobject#1431
TilesRenderer: Move "traversal" fields into a dedicated subobject#1431
Conversation
# Conflicts: # src/core/renderer/tiles/TilesRendererBase.js
There was a problem hiding this comment.
Pull request overview
This pull request refactors the tile state management in the 3D tiles renderer by reorganizing tile properties into two dedicated namespaces: internal for implementation details and traversal for frame-based traversal state. This improves code organization and makes it clearer which properties are part of the internal implementation versus traversal algorithms.
Changes:
- Moved traversal-related properties from underscore-prefixed fields to a
tile.traversalobject - Moved internal implementation properties to a
tile.internalobject - Updated TypeScript definitions with new
TileInternalDataandTileTraversalDatainterfaces - Removed the old
TileInternal.d.tsfile as it's no longer needed
Reviewed changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/core/renderer/tiles/Tile.d.ts | Added new TypeScript interfaces for TileInternalData and TileTraversalData, reorganizing tile state type definitions |
| src/core/renderer/tiles/TileInternal.d.ts | Deleted as the interface is superseded by the new structure |
| src/core/renderer/tiles/TilesRendererBase.js | Updated tile initialization to create internal and traversal objects; updated priority callbacks to use new property paths |
| src/core/renderer/tiles/traverseFunctions.js | Refactored all traversal logic to access properties through tile.traversal and tile.internal |
| src/core/renderer/tiles/optimizedTraverseFunctions.js | Similar refactoring for optimized traversal path |
| src/core/renderer/utilities/TraversalUtils.js | Updated traverseAncestors to use tile.internal.depth |
| src/three/renderer/tiles/raycastTraverse.js | Updated raycasting to check for traversal object and use new property paths |
| src/three/plugins/DebugTilesPlugin.js | Updated debug visualization to access tile properties through new namespaces |
| src/three/plugins/fade/TilesFadePlugin.js | Updated fade logic to use tile.traversal properties |
| src/three/plugins/images/ImageOverlayPlugin.js | Updated to use tile.internal properties |
| src/three/plugins/QuantizedMeshPlugin.js | Updated to use tile.internal.childrenProcessed |
| src/core/plugins/ImplicitTilingPlugin.js | Updated to use tile.internal properties |
| test/core/traverseFunctions.test.js | Updated test helper to create tiles with internal object |
Comments suppressed due to low confidence (2)
src/core/renderer/tiles/TilesRendererBase.js:87
- Inconsistent null/undefined checking for the traversal object. The defaultPriorityCallback checks if traversal exists on line 24, but the optimizedPriorityCallback does not perform the same check starting at line 65. This could lead to errors if tiles without traversal objects are compared in optimizedPriorityCallback.
const optimizedPriorityCallback = ( a, b ) => {
const aPriority = a.priority || 0;
const bPriority = b.priority || 0;
if ( aPriority !== bPriority ) {
// lower priority value sorts first
return aPriority > bPriority ? 1 : - 1;
} else if ( a.traversal.used !== b.traversal.used ) {
// load tiles that have been used
return a.traversal.used ? 1 : - 1;
} else if ( a.traversal.inFrustum !== b.traversal.inFrustum ) {
// load tiles that have are in the frustum
return a.traversal.inFrustum ? 1 : - 1;
} else if ( a.internal.hasUnrenderableContent !== b.internal.hasUnrenderableContent ) {
// load internal tile sets first
return a.internal.hasUnrenderableContent ? 1 : - 1;
} else if ( a.traversal.distanceFromCamera !== b.traversal.distanceFromCamera ) {
// load closer tiles first
return a.traversal.distanceFromCamera > b.traversal.distanceFromCamera ? - 1 : 1;
}
return 0;
src/core/renderer/tiles/TilesRendererBase.js:135
- Missing null/undefined check for the internal object in priority callbacks. While defaultPriorityCallback and lruPriorityCallback check if traversal exists (lines 24 and 103), none of the priority callbacks check if internal exists before accessing properties like
internal.depthFromRenderedParentandinternal.hasUnrenderableContent. If tiles without internal objects are compared, this will throw errors. The checks should be added consistently across all three priority callbacks.
const defaultPriorityCallback = ( a, b ) => {
const aPriority = a.priority || 0;
const bPriority = b.priority || 0;
if ( aPriority !== bPriority ) {
// lower priority value sorts first
return aPriority > bPriority ? 1 : - 1;
} else if ( ! a.traversal || ! b.traversal ) {
return 0;
} else if ( a.traversal.used !== b.traversal.used ) {
// load tiles that have been used
return a.traversal.used ? 1 : - 1;
} else if ( a.traversal.error !== b.traversal.error ) {
// load the tile with the higher error
return a.traversal.error > b.traversal.error ? 1 : - 1;
} else if ( a.traversal.distanceFromCamera !== b.traversal.distanceFromCamera ) {
// and finally visible tiles which have equal error (ex: if geometricError === 0)
// should prioritize based on distance.
return a.traversal.distanceFromCamera > b.traversal.distanceFromCamera ? - 1 : 1;
} else if ( a.internal.depthFromRenderedParent !== b.internal.depthFromRenderedParent ) {
return a.internal.depthFromRenderedParent > b.internal.depthFromRenderedParent ? - 1 : 1;
}
return 0;
};
// Optimized priority callback - prioritizes distance over error for better user experience
const optimizedPriorityCallback = ( a, b ) => {
const aPriority = a.priority || 0;
const bPriority = b.priority || 0;
if ( aPriority !== bPriority ) {
// lower priority value sorts first
return aPriority > bPriority ? 1 : - 1;
} else if ( a.traversal.used !== b.traversal.used ) {
// load tiles that have been used
return a.traversal.used ? 1 : - 1;
} else if ( a.traversal.inFrustum !== b.traversal.inFrustum ) {
// load tiles that have are in the frustum
return a.traversal.inFrustum ? 1 : - 1;
} else if ( a.internal.hasUnrenderableContent !== b.internal.hasUnrenderableContent ) {
// load internal tile sets first
return a.internal.hasUnrenderableContent ? 1 : - 1;
} else if ( a.traversal.distanceFromCamera !== b.traversal.distanceFromCamera ) {
// load closer tiles first
return a.traversal.distanceFromCamera > b.traversal.distanceFromCamera ? - 1 : 1;
}
return 0;
};
// lru cache unload callback that takes two tiles to compare. Returning 1 means "tile a"
// is unloaded first.
const lruPriorityCallback = ( a, b ) => {
const aPriority = a.priority || 0;
const bPriority = b.priority || 0;
if ( aPriority !== bPriority ) {
// lower priority value sorts first
return aPriority > bPriority ? 1 : - 1;
} else if ( ! a.traversal || ! b.traversal ) {
return 0;
} else if ( a.traversal.lastFrameVisited !== b.traversal.lastFrameVisited ) {
// dispose of least recent tiles first
return a.traversal.lastFrameVisited > b.traversal.lastFrameVisited ? - 1 : 1;
} else if ( a.internal.depthFromRenderedParent !== b.internal.depthFromRenderedParent ) {
// dispose of deeper tiles first so parents are not disposed before children
return a.internal.depthFromRenderedParent > b.internal.depthFromRenderedParent ? 1 : - 1;
} else if ( a.internal.loadingState !== b.internal.loadingState ) {
// dispose of tiles that are earlier along in the loading process first
return a.internal.loadingState > b.internal.loadingState ? - 1 : 1;
} else if ( a.internal.hasUnrenderableContent !== b.internal.hasUnrenderableContent ) {
// dispose of external tilesets last
return a.internal.hasUnrenderableContent ? - 1 : 1;
} else if ( a.traversal.error !== b.traversal.error ) {
// unload the tile with lower error
return a.traversal.error > b.traversal.error ? - 1 : 1;
}
return 0;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| const isUsed = isUsedThisFrame( tile, renderer.frameCount ); | ||
| if ( isUsed || tile.__usedLastFrame ) { | ||
| if ( isUsed || tile.traversal?.usedLastFrame ) { |
There was a problem hiding this comment.
Inconsistent use of optional chaining. Line 417 uses optional chaining tile.traversal?.usedLastFrame but immediately after on lines 424, 427, 431, etc., the code accesses tile.traversal properties directly without optional chaining. Either the optional chaining is unnecessary (if traversal is always guaranteed to exist), or all subsequent accesses should also use optional chaining for safety.
| if ( isUsed || tile.traversal?.usedLastFrame ) { | |
| if ( isUsed || tile.traversal.usedLastFrame ) { |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 13 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } else if ( a.internal.depthFromRenderedParent !== b.internal.depthFromRenderedParent ) { | ||
|
|
||
| return a.__depthFromRenderedParent > b.__depthFromRenderedParent ? - 1 : 1; | ||
| return a.internal.depthFromRenderedParent > b.internal.depthFromRenderedParent ? - 1 : 1; |
There was a problem hiding this comment.
In the defaultPriorityCallback, line 44 accesses a.internal.depthFromRenderedParent without checking if a.internal exists, similar to line 46 for b.internal. While traversal is checked on lines 24, the internal object should also be checked to prevent potential errors. Consider adding: } else if ( ! a.internal || ! b.internal ) { return 0; } before line 44.
| } else if ( a.internal.hasUnrenderableContent !== b.internal.hasUnrenderableContent ) { | ||
|
|
||
| // load internal tile sets first | ||
| return a.__hasUnrenderableContent ? 1 : - 1; | ||
| return a.internal.hasUnrenderableContent ? 1 : - 1; |
There was a problem hiding this comment.
In the optimizedPriorityCallback, line 79 accesses a.internal.hasUnrenderableContent without checking if a.internal or b.internal exists. Consider adding a null check: } else if ( ! a.internal || ! b.internal ) { return 0; } before line 79.
| } else if ( a.internal.depthFromRenderedParent !== b.internal.depthFromRenderedParent ) { | ||
|
|
||
| // dispose of deeper tiles first so parents are not disposed before children | ||
| return a.__depthFromRenderedParent > b.__depthFromRenderedParent ? 1 : - 1; | ||
| return a.internal.depthFromRenderedParent > b.internal.depthFromRenderedParent ? 1 : - 1; | ||
|
|
||
| } else if ( a.__loadingState !== b.__loadingState ) { | ||
| } else if ( a.internal.loadingState !== b.internal.loadingState ) { | ||
|
|
||
| // dispose of tiles that are earlier along in the loading process first | ||
| return a.__loadingState > b.__loadingState ? - 1 : 1; | ||
| return a.internal.loadingState > b.internal.loadingState ? - 1 : 1; | ||
|
|
||
| } else if ( a.__hasUnrenderableContent !== b.__hasUnrenderableContent ) { | ||
| } else if ( a.internal.hasUnrenderableContent !== b.internal.hasUnrenderableContent ) { | ||
|
|
||
| // dispose of external tilesets last | ||
| return a.__hasUnrenderableContent ? - 1 : 1; | ||
| return a.internal.hasUnrenderableContent ? - 1 : 1; |
There was a problem hiding this comment.
In the lruPriorityCallback, multiple lines (116, 121, 126) access a.internal and b.internal properties without checking if these objects exist. Consider adding a null check: } else if ( ! a.internal || ! b.internal ) { return 0; } before line 116.
| wasSetVisible: false, | ||
| active: false, | ||
| wasSetActive: false, | ||
| allChildrenReady: false, |
There was a problem hiding this comment.
The traversal object initialization is missing the kicked and allUsedChildrenProcessed fields that are used in optimizedTraverseFunctions.js (lines 52-53, 179, 304, 312, 411). These fields should be initialized here with appropriate default values (e.g., kicked: false and allUsedChildrenProcessed: false) to ensure consistent initialization.
| allChildrenReady: false, | |
| allChildrenReady: false, | |
| kicked: false, | |
| allUsedChildrenProcessed: false, |
| tile.__inFrustum = false; | ||
| tile.__isLeaf = false; | ||
| parentTile.internal.childrenProcessed ++; | ||
| tile.internal.depth = parentTile.internal.depth + 1, |
There was a problem hiding this comment.
There's a syntax error on line 710: using a comma instead of a semicolon. The line should end with a semicolon (;) not a comma (,), as this is a statement assignment, not an object literal property.
| tile.internal.depth = parentTile.internal.depth + 1, | |
| tile.internal.depth = parentTile.internal.depth + 1; |
| if ( tile.traversal.visible ) { | ||
|
|
||
| this.invokeOnePlugin( plugin => plugin.setTileVisible && plugin.setTileVisible( tile, false ) ); | ||
| tile.__visible = false; | ||
| tile.traversal.visible = false; | ||
|
|
||
| } | ||
|
|
||
| if ( tile.__active ) { | ||
| if ( tile.traversal.active ) { | ||
|
|
||
| this.invokeOnePlugin( plugin => plugin.setTileActive && plugin.setTileActive( tile, false ) ); | ||
| tile.__active = false; | ||
| tile.traversal.active = false; | ||
|
|
||
| } |
There was a problem hiding this comment.
The disposeTile function accesses tile.traversal properties without checking if tile.traversal exists. If a tile is disposed before being fully initialized (e.g., during error scenarios), this could cause a runtime error. Consider adding a check: if ( tile.traversal?.visible ) and if ( tile.traversal?.active ).
Related to #1375, #1429
TODO