Layers: Add recursive property #32619
Conversation
📦 Bundle sizeFull ESM build, minified and gzipped.
🌳 Bundle size after tree-shakingMinimal build including a renderer, camera, empty scene, and dependencies.
|
wrangelvid
force-pushed
the
recursive_layers
branch
from
f3a46e6 to
f013785
Compare
|
I converted this to draft as I realized there are many more places where we need to take account for layer inheritance. Alternatively, we could store an |
wrangelvid
force-pushed
the
recursive_layers
branch
2 times, most recently
from
9b9b1e2 to
e45ddc3
Compare
Changes SummaryThis PR adds a Type: feature Components Affected: Layers system, Object3D traversal, Renderer (common, WebGL, WebGPU), Raycaster, RenderList, CSS2D/CSS3D renderers, Shadow mapping Files Changed
Architecture Impact
Risk Areas: Renderer traversal logic: Complex parameter threading through Suggestions
Full review in progress... | Powered by diffray |
|
|
||
| if ( layerVisible ) callback( this ); | ||
|
|
||
| const childInheritedLayers = this.layers.recursive ? this.layers : inheritedLayers; |
There was a problem hiding this comment.
🟠 HIGH - Incorrect layer inheritance - using object.layers instead of effectiveLayers
Agent: Delegated (architecture, bugs, documentation, performance)
Category: bug
Description:
When an object has recursive=true, children should inherit the effectiveLayers (which may have been inherited from an ancestor), not object.layers. The code checks object.layers.recursive instead of effectiveLayers.recursive, causing children to inherit the wrong layer mask when their parent inherited layers from an ancestor with recursive=true.
Suggestion:
Change line 1104 from 'const childInheritedLayers = this.layers.recursive ? this.layers : inheritedLayers;' to 'const childInheritedLayers = effectiveLayers.recursive ? effectiveLayers : inheritedLayers;'
Confidence: 85%
Review ID: 18426889-7dd5-4a37-bc33-d2c4c0357982
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
|
|
||
| if ( propagate === true && recursive === true ) { | ||
|
|
||
| const childInheritedLayers = object.layers.recursive === true ? object.layers : inheritedLayers; |
There was a problem hiding this comment.
🟠 HIGH - Incorrect layer inheritance in raycasting - using object.layers instead of effectiveLayers
Agent: Delegated (architecture, bugs, documentation, performance)
Category: bug
Description:
When an object has recursive=true, children should inherit the effectiveLayers (which may have been inherited from an ancestor), not object.layers. The code checks object.layers.recursive instead of effectiveLayers.recursive, causing incorrect raycasting behavior with inherited layers.
Suggestion:
Change line 255 from 'const childInheritedLayers = object.layers.recursive === true ? object.layers : inheritedLayers;' to 'const childInheritedLayers = effectiveLayers.recursive === true ? effectiveLayers : inheritedLayers;'
Confidence: 85%
Review ID: 18426889-7dd5-4a37-bc33-d2c4c0357982
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
|
|
||
| } | ||
|
|
||
| const childInheritedLayers = object.layers.recursive === true ? object.layers : inheritedLayers; |
There was a problem hiding this comment.
🟠 HIGH - Incorrect layer inheritance in rendering - using object.layers instead of effectiveLayers
Agent: Delegated (architecture, bugs, documentation, performance)
Category: bug
Description:
When an object has recursive=true, children should inherit the effectiveLayers (which may have been inherited from an ancestor), not object.layers. The code checks object.layers.recursive instead of effectiveLayers.recursive, causing incorrect rendering of objects with inherited recursive layers.
Suggestion:
Change line 2925 from 'const childInheritedLayers = object.layers.recursive === true ? object.layers : inheritedLayers;' to 'const childInheritedLayers = effectiveLayers.recursive === true ? effectiveLayers : inheritedLayers;'
Confidence: 85%
Review ID: 18426889-7dd5-4a37-bc33-d2c4c0357982
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
|
|
||
| if ( material.transparent === true || material.transmission > 0 || | ||
| ( material.transmissionNode && material.transmissionNode.isNode ) || | ||
| ( material.backdropNode && material.backdropNode.isNode ) ) { | ||
|
|
||
| if ( needsDoublePass( material ) ) this.transparentDoublePass.push( renderItem ); | ||
|
|
||
| this.transparent.push( renderItem ); | ||
|
|
||
| } else { | ||
|
|
||
| this.opaque.push( renderItem ); | ||
|
|
||
| } | ||
|
|
||
| } | ||
|
|
||
| /** | ||
| * Inserts the given object as a render item at the start of the internal render lists. | ||
| * The selected lists depend on the object properties. | ||
| * | ||
| * @param {Object3D} object - The 3D object. | ||
| * @param {BufferGeometry} geometry - The 3D object's geometry. | ||
| * @param {Material} material - The 3D object's material. | ||
| * @param {number} groupOrder - The current group order. | ||
| * @param {number} z - Th 3D object's depth value (z value in clip space). | ||
| * @param {?number} group - {?Object} group - Only relevant for objects using multiple materials. This represents a group entry from the respective `BufferGeometry`. | ||
| * @param {ClippingContext} clippingContext - The current clipping context. | ||
| * @param {Layers} effectiveLayers - The effective layers for layer visibility testing. | ||
| */ | ||
| unshift( object, geometry, material, groupOrder, z, group, clippingContext ) { | ||
| unshift( object, geometry, material, groupOrder, z, group, clippingContext, effectiveLayers ) { | ||
|
|
||
| const renderItem = this.getNextRenderItem( object, geometry, material, groupOrder, z, group, clippingContext ); | ||
| const renderItem = this.getNextRenderItem( object, geometry, material, groupOrder, z, group, clippingContext, effectiveLayers ); | ||
|
|
||
| if ( material.transparent === true || material.transmission > 0 || | ||
| ( material.transmissionNode && material.transmissionNode.isNode ) || |
There was a problem hiding this comment.
🟡 MEDIUM - Repeated material transparency checks
Agent: react
Category: quality
Description:
Material transparency logic is duplicated in push() and unshift() methods, checking material.transmission and related properties in identical patterns.
Suggestion:
Extract material transparency check into a shared helper function to eliminate duplication and reduce maintenance risk when logic changes.
Why this matters: Reduces duplicate work and logic drift.
Confidence: 75%
Rule: ts_use_computed_derived_properties_for_repe
Review ID: 18426889-7dd5-4a37-bc33-d2c4c0357982
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| /** | ||
| * Like {@link Object3D#traverseVisible}, but the callback will only be executed | ||
| * for visible 3D objects that pass the layer test. Descendants of invisible | ||
| * 3D objects and objects with recursive layers that fail the test are not traversed. | ||
| * | ||
| * Note: Modifying the scene graph inside the callback is discouraged. | ||
| * | ||
| * @param {Layers} cameraLayers - The camera layers to test against. | ||
| * @param {Function} callback - A callback function that allows to process the current 3D object. | ||
| */ | ||
| traverseVisibleWithLayers( cameraLayers, callback, inheritedLayers = null ) { | ||
|
|
||
| if ( this.visible === false ) return; | ||
|
|
||
| const effectiveLayers = inheritedLayers !== null ? inheritedLayers : this.layers; | ||
| const layerVisible = effectiveLayers.test( cameraLayers ); | ||
|
|
||
| if ( ! layerVisible && effectiveLayers.recursive ) return; | ||
|
|
||
| if ( layerVisible ) callback( this ); | ||
|
|
||
| const childInheritedLayers = this.layers.recursive ? this.layers : inheritedLayers; | ||
|
|
||
| for ( let i = 0, l = this.children.length; i < l; i ++ ) { | ||
|
|
||
| this.children[ i ].traverseVisibleWithLayers( cameraLayers, callback, childInheritedLayers ); | ||
|
|
||
| } | ||
|
|
||
| } | ||
|
|
There was a problem hiding this comment.
🟠 HIGH - New traverseVisibleWithLayers method lacks test coverage
Agent: testing
Category: quality
Description:
A new public method traverseVisibleWithLayers() was added to Object3D with parameters (cameraLayers, callback, inheritedLayers = null) but there are no corresponding unit tests. The existing traverse/traverseVisible/traverseAncestors tests follow a pattern in Object3D.tests.js but the new method combining layer visibility testing with inheritance logic has zero test coverage.
Suggestion:
Add comprehensive test case to test/unit/src/core/Object3D.tests.js following the existing pattern, testing: (1) Basic traversal with matching camera layers, (2) Traversal when child layers don't match camera layers, (3) Recursive layer inheritance - when parent has recursive=true and child has different layers, (4) Handling of inheritedLayers parameter (5) Edge case where recursive=true but test fails, (6) Multiple levels of nesting with mixed recursive values
Confidence: 92%
Rule: test_new_parameter_coverage
Review ID: 18426889-7dd5-4a37-bc33-d2c4c0357982
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| function intersect( object, raycaster, intersects, recursive, inheritedLayers = null ) { | ||
|
|
||
| const effectiveLayers = inheritedLayers !== null ? inheritedLayers : object.layers; | ||
| const visible = effectiveLayers.test( raycaster.layers ); | ||
|
|
||
| if ( visible === false && effectiveLayers.recursive === true ) return; | ||
|
|
||
| let propagate = true; | ||
|
|
||
| if ( object.layers.test( raycaster.layers ) ) { | ||
| if ( visible ) { | ||
|
|
||
| const result = object.raycast( raycaster, intersects ); | ||
|
|
||
| if ( result === false ) propagate = false; | ||
|
|
||
| } | ||
|
|
||
| if ( propagate === true && recursive === true ) { | ||
|
|
||
| const childInheritedLayers = object.layers.recursive === true ? object.layers : inheritedLayers; | ||
|
|
||
| const children = object.children; | ||
|
|
||
| for ( let i = 0, l = children.length; i < l; i ++ ) { | ||
|
|
||
| intersect( children[ i ], raycaster, intersects, true ); | ||
| intersect( children[ i ], raycaster, intersects, true, childInheritedLayers ); |
There was a problem hiding this comment.
🟠 HIGH - Raycaster.intersect() new parameter inheritedLayers lacks test coverage
Agent: testing
Category: quality
Description:
The intersect() helper function was modified to add a new parameter inheritedLayers = null and uses it to determine effective layers for visibility testing. The test file test/unit/src/core/Raycaster.tests.js tests intersectObject() and intersectObjects() with recursive and non-recursive searches, but does not test layer-based filtering or the new recursive layer inheritance behavior.
Suggestion:
Add test cases to test/unit/src/core/Raycaster.tests.js following the existing 'intersectObject' and 'intersectObjects' test pattern. Test: (1) Raycasting with layer filtering enabled (2) Child objects hidden by parent's recursive layer setting (3) Child objects with different layers when parent has recursive=true (4) Inheritance of parent layers through recursive property
Confidence: 90%
Rule: test_new_parameter_coverage
Review ID: 18426889-7dd5-4a37-bc33-d2c4c0357982
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| * @param {Layers|null} inheritedLayers - Layers inherited from a recursive ancestor. | ||
| */ | ||
| _projectObject( object, camera, groupOrder, renderList, clippingContext ) { | ||
| _projectObject( object, camera, groupOrder, renderList, clippingContext, inheritedLayers = null ) { | ||
|
|
||
| if ( object.visible === false ) return; | ||
|
|
||
| const visible = object.layers.test( camera.layers ); | ||
| const effectiveLayers = inheritedLayers !== null ? inheritedLayers : object.layers; | ||
| const visible = effectiveLayers.test( camera.layers ); | ||
|
|
||
| if ( visible === false && effectiveLayers.recursive === true ) return; | ||
|
|
||
| if ( visible ) { | ||
|
|
||
| if ( object.isGroup ) { | ||
|
|
||
| groupOrder = object.renderOrder; | ||
|
|
||
| if ( object.isClippingGroup && object.enabled ) clippingContext = clippingContext.getGroupContext( object ); | ||
|
|
||
| } else if ( object.isLOD ) { | ||
|
|
||
| if ( object.autoUpdate === true ) object.update( camera ); | ||
|
|
||
| } else if ( object.isLight ) { | ||
|
|
||
| renderList.pushLight( object ); | ||
|
|
||
| } else if ( object.isSprite ) { | ||
|
|
||
| const frustum = camera.isArrayCamera ? _frustumArray : _frustum; | ||
|
|
||
| if ( ! object.frustumCulled || frustum.intersectsSprite( object, camera ) ) { | ||
|
|
||
| if ( this.sortObjects === true ) { | ||
|
|
||
| _vector4.setFromMatrixPosition( object.matrixWorld ).applyMatrix4( _projScreenMatrix ); | ||
|
|
||
| } | ||
|
|
||
| const { geometry, material } = object; | ||
|
|
||
| if ( material.visible ) { | ||
|
|
||
| renderList.push( object, geometry, material, groupOrder, _vector4.z, null, clippingContext ); | ||
| renderList.push( object, geometry, material, groupOrder, _vector4.z, null, clippingContext, effectiveLayers ); | ||
|
|
||
| } | ||
|
|
||
| } | ||
|
|
||
| } else if ( object.isLineLoop ) { | ||
|
|
||
| error( 'Renderer: Objects of type THREE.LineLoop are not supported. Please use THREE.Line or THREE.LineSegments.' ); | ||
|
|
||
| } else if ( object.isMesh || object.isLine || object.isPoints ) { | ||
|
|
||
| const frustum = camera.isArrayCamera ? _frustumArray : _frustum; | ||
|
|
||
| if ( ! object.frustumCulled || frustum.intersectsObject( object, camera ) ) { | ||
|
|
||
| const { geometry, material } = object; | ||
|
|
||
| if ( this.sortObjects === true ) { | ||
|
|
||
| if ( geometry.boundingSphere === null ) geometry.computeBoundingSphere(); | ||
|
|
||
| _vector4 | ||
| .copy( geometry.boundingSphere.center ) | ||
| .applyMatrix4( object.matrixWorld ) | ||
| .applyMatrix4( _projScreenMatrix ); | ||
|
|
||
| } | ||
|
|
||
| if ( Array.isArray( material ) ) { | ||
|
|
||
| const groups = geometry.groups; | ||
|
|
||
| for ( let i = 0, l = groups.length; i < l; i ++ ) { | ||
|
|
||
| const group = groups[ i ]; | ||
| const groupMaterial = material[ group.materialIndex ]; | ||
|
|
||
| if ( groupMaterial && groupMaterial.visible ) { | ||
|
|
||
| renderList.push( object, geometry, groupMaterial, groupOrder, _vector4.z, group, clippingContext ); | ||
| renderList.push( object, geometry, groupMaterial, groupOrder, _vector4.z, group, clippingContext, effectiveLayers ); | ||
|
|
||
| } | ||
|
|
||
| } | ||
|
|
||
| } else if ( material.visible ) { | ||
|
|
||
| renderList.push( object, geometry, material, groupOrder, _vector4.z, null, clippingContext ); | ||
| renderList.push( object, geometry, material, groupOrder, _vector4.z, null, clippingContext, effectiveLayers ); | ||
|
|
||
| } | ||
|
|
||
| } | ||
|
|
||
| } | ||
|
|
||
| } | ||
|
|
||
| if ( object.isBundleGroup === true && this.backend.beginBundle !== undefined ) { | ||
|
|
||
| const baseRenderList = renderList; | ||
|
|
||
| // replace render list | ||
| renderList = this._renderLists.get( object, camera ); | ||
|
|
||
| renderList.begin(); | ||
|
|
||
| baseRenderList.pushBundle( { | ||
| bundleGroup: object, | ||
| camera, | ||
| renderList, | ||
| } ); | ||
|
|
||
| renderList.finish(); | ||
|
|
||
| } | ||
|
|
||
| const childInheritedLayers = object.layers.recursive === true ? object.layers : inheritedLayers; | ||
|
|
||
| const children = object.children; | ||
|
|
||
| for ( let i = 0, l = children.length; i < l; i ++ ) { | ||
|
|
||
| this._projectObject( children[ i ], camera, groupOrder, renderList, clippingContext ); | ||
| this._projectObject( children[ i ], camera, groupOrder, renderList, clippingContext, childInheritedLayers ); |
There was a problem hiding this comment.
🟠 HIGH - Renderer._projectObject() new parameter inheritedLayers lacks test coverage
Agent: testing
Category: quality
Description:
The _projectObject() method signature was modified to add inheritedLayers = null parameter, implementing layer inheritance with effectiveLayers computation for visibility calculations. WebGLRenderer.tests.js is an empty stub with no tests.
Suggestion:
Add integration tests or create tests in WebGLRenderer.tests.js that verify: (1) Objects with recursive layers=true are rendered correctly (2) Child objects inherit parent's layers when parent has recursive=true (3) effectiveLayers is passed correctly through the rendering pipeline (4) Layer filtering works correctly with the new inheritance mechanism
Confidence: 85%
Rule: test_new_parameter_coverage
Review ID: 18426889-7dd5-4a37-bc33-d2c4c0357982
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| getNextRenderItem( object, geometry, material, groupOrder, z, group, clippingContext, effectiveLayers ) { | ||
|
|
||
| let renderItem = this.renderItems[ this.renderItemsIndex ]; | ||
|
|
||
| if ( renderItem === undefined ) { | ||
|
|
||
| renderItem = { | ||
| id: object.id, | ||
| object: object, | ||
| geometry: geometry, | ||
| material: material, | ||
| groupOrder: groupOrder, | ||
| renderOrder: object.renderOrder, | ||
| z: z, | ||
| group: group, | ||
| clippingContext: clippingContext | ||
| clippingContext: clippingContext, | ||
| effectiveLayers: effectiveLayers | ||
| }; | ||
|
|
||
| this.renderItems[ this.renderItemsIndex ] = renderItem; | ||
|
|
||
| } else { | ||
|
|
||
| renderItem.id = object.id; | ||
| renderItem.object = object; | ||
| renderItem.geometry = geometry; | ||
| renderItem.material = material; | ||
| renderItem.groupOrder = groupOrder; | ||
| renderItem.renderOrder = object.renderOrder; | ||
| renderItem.z = z; | ||
| renderItem.group = group; | ||
| renderItem.clippingContext = clippingContext; | ||
| renderItem.effectiveLayers = effectiveLayers; | ||
|
|
||
| } | ||
|
|
||
| this.renderItemsIndex ++; | ||
|
|
||
| return renderItem; | ||
|
|
||
| } | ||
|
|
||
| /** | ||
| * Pushes the given object as a render item to the internal render lists. | ||
| * The selected lists depend on the object properties. | ||
| * | ||
| * @param {Object3D} object - The 3D object. | ||
| * @param {BufferGeometry} geometry - The 3D object's geometry. | ||
| * @param {Material} material - The 3D object's material. | ||
| * @param {number} groupOrder - The current group order. | ||
| * @param {number} z - Th 3D object's depth value (z value in clip space). | ||
| * @param {?number} group - {?Object} group - Only relevant for objects using multiple materials. This represents a group entry from the respective `BufferGeometry`. | ||
| * @param {ClippingContext} clippingContext - The current clipping context. | ||
| * @param {Layers} effectiveLayers - The effective layers for layer visibility testing. | ||
| */ | ||
| push( object, geometry, material, groupOrder, z, group, clippingContext ) { | ||
| push( object, geometry, material, groupOrder, z, group, clippingContext, effectiveLayers ) { | ||
|
|
||
| const renderItem = this.getNextRenderItem( object, geometry, material, groupOrder, z, group, clippingContext ); | ||
| const renderItem = this.getNextRenderItem( object, geometry, material, groupOrder, z, group, clippingContext, effectiveLayers ); | ||
|
|
||
| if ( object.occlusionTest === true ) this.occlusionQueryCount ++; | ||
|
|
||
| if ( material.transparent === true || material.transmission > 0 || | ||
| ( material.transmissionNode && material.transmissionNode.isNode ) || | ||
| ( material.backdropNode && material.backdropNode.isNode ) ) { | ||
|
|
||
| if ( needsDoublePass( material ) ) this.transparentDoublePass.push( renderItem ); | ||
|
|
||
| this.transparent.push( renderItem ); | ||
|
|
||
| } else { | ||
|
|
||
| this.opaque.push( renderItem ); | ||
|
|
||
| } | ||
|
|
||
| } | ||
|
|
||
| /** | ||
| * Inserts the given object as a render item at the start of the internal render lists. | ||
| * The selected lists depend on the object properties. | ||
| * | ||
| * @param {Object3D} object - The 3D object. | ||
| * @param {BufferGeometry} geometry - The 3D object's geometry. | ||
| * @param {Material} material - The 3D object's material. | ||
| * @param {number} groupOrder - The current group order. | ||
| * @param {number} z - Th 3D object's depth value (z value in clip space). | ||
| * @param {?number} group - {?Object} group - Only relevant for objects using multiple materials. This represents a group entry from the respective `BufferGeometry`. | ||
| * @param {ClippingContext} clippingContext - The current clipping context. | ||
| * @param {Layers} effectiveLayers - The effective layers for layer visibility testing. | ||
| */ | ||
| unshift( object, geometry, material, groupOrder, z, group, clippingContext ) { | ||
| unshift( object, geometry, material, groupOrder, z, group, clippingContext, effectiveLayers ) { | ||
|
|
||
| const renderItem = this.getNextRenderItem( object, geometry, material, groupOrder, z, group, clippingContext ); | ||
| const renderItem = this.getNextRenderItem( object, geometry, material, groupOrder, z, group, clippingContext, effectiveLayers ); |
There was a problem hiding this comment.
🟡 MEDIUM - RenderList push/unshift/getNextRenderItem effectiveLayers lacks test coverage
Agent: testing
Category: quality
Description:
Three RenderList methods (push, unshift, getNextRenderItem) were modified to accept and store a new effectiveLayers parameter. These are internal rendering pipeline methods for the WebGPU/common renderer path. WebGLRenderLists.tests.js tests the WebGL-specific render list, but the common/RenderList.js class has no tests.
Suggestion:
Create tests for RenderList class to verify: (1) push() correctly stores effectiveLayers in the render item (2) unshift() correctly stores effectiveLayers in the render item (3) getNextRenderItem() properly initializes effectiveLayers property on new items (4) Re-used render items have effectiveLayers updated correctly
Confidence: 72%
Rule: test_new_parameter_coverage
Review ID: 18426889-7dd5-4a37-bc33-d2c4c0357982
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
Review Summary
Validated 28 issues: 17 kept, 11 filtered Issues Found: 17💬 See 8 individual line comment(s) for details. 📊 8 unique issue type(s) across 17 location(s) 📋 Full issue list (click to expand)🟠 HIGH - Incorrect layer inheritance - using object.layers instead of effectiveLayersAgent: Delegated (architecture, bugs, documentation, performance) Category: bug File: Description: When an object has recursive=true, children should inherit the effectiveLayers (which may have been inherited from an ancestor), not object.layers. The code checks object.layers.recursive instead of effectiveLayers.recursive, causing children to inherit the wrong layer mask when their parent inherited layers from an ancestor with recursive=true. Suggestion: Change line 1104 from 'const childInheritedLayers = this.layers.recursive ? this.layers : inheritedLayers;' to 'const childInheritedLayers = effectiveLayers.recursive ? effectiveLayers : inheritedLayers;' Confidence: 85% 🟠 HIGH - Incorrect layer inheritance in raycasting - using object.layers instead of effectiveLayersAgent: Delegated (architecture, bugs, documentation, performance) Category: bug File: Description: When an object has recursive=true, children should inherit the effectiveLayers (which may have been inherited from an ancestor), not object.layers. The code checks object.layers.recursive instead of effectiveLayers.recursive, causing incorrect raycasting behavior with inherited layers. Suggestion: Change line 255 from 'const childInheritedLayers = object.layers.recursive === true ? object.layers : inheritedLayers;' to 'const childInheritedLayers = effectiveLayers.recursive === true ? effectiveLayers : inheritedLayers;' Confidence: 85% 🟠 HIGH - Incorrect layer inheritance in rendering - using object.layers instead of effectiveLayersAgent: Delegated (architecture, bugs, documentation, performance) Category: bug File: Description: When an object has recursive=true, children should inherit the effectiveLayers (which may have been inherited from an ancestor), not object.layers. The code checks object.layers.recursive instead of effectiveLayers.recursive, causing incorrect rendering of objects with inherited recursive layers. Suggestion: Change line 2925 from 'const childInheritedLayers = object.layers.recursive === true ? object.layers : inheritedLayers;' to 'const childInheritedLayers = effectiveLayers.recursive === true ? effectiveLayers : inheritedLayers;' Confidence: 85% 🟠 HIGH - JSDoc parameter description mismatch in disable() method (4 occurrences)Agent: documentation Category: docs 📍 View all locations
Rule: 🟠 HIGH - New traverseVisibleWithLayers method lacks test coverage (4 occurrences)Agent: testing Category: quality 📍 View all locations
Rule: 🟠 HIGH - Object3D.toJSON() missing layers.recursive serialization (2 occurrences)Agent: typescript Category: bug 📍 View all locations
Rule: 🟡 MEDIUM - JSDoc default value mismatch for CSS2DObject element property (3 occurrences)Agent: documentation Category: docs Why this matters: Wrong return docs cause callers to mishandle return values. 📍 View all locations
Rule: 🟡 MEDIUM - Repeated material transparency checksAgent: react Category: quality Why this matters: Reduces duplicate work and logic drift. File: Description: Material transparency logic is duplicated in push() and unshift() methods, checking material.transmission and related properties in identical patterns. Suggestion: Extract material transparency check into a shared helper function to eliminate duplication and reduce maintenance risk when logic changes. Confidence: 75% Rule: ℹ️ 9 issue(s) outside PR diff (click to expand)
🟠 HIGH - JSDoc parameter description mismatch in disable() method (4 occurrences)Agent: documentation Category: docs 📍 View all locations
Rule: 🟠 HIGH - Object3D.toJSON() missing layers.recursive serialization (2 occurrences)Agent: typescript Category: bug 📍 View all locations
Rule: 🟡 MEDIUM - JSDoc default value mismatch for CSS2DObject element property (3 occurrences)Agent: documentation Category: docs Why this matters: Wrong return docs cause callers to mishandle return values. 📍 View all locations
Rule: Review ID: |
|
Who invited diffray-bot ? |
Co-Authored-By: Cody Bennett <[email protected]>
wrangelvid
force-pushed
the
recursive_layers
branch
from
e45ddc3 to
c80433f
Compare
2 participants
Description
Adds a
recursiveproperty to the Layers class. When set totrue, if an object's layer test fails during traversal, the entire subtree is skipped. If the layer test passes, children inherit the parent's layers instead of using their own.This is a revival of #28427 by @CodyJasonBennett.
Changes from original PR
DEFAULT_RECURSIVEstatic property (main source of debate in the original PR)inheritedLayersparameter passed through traversalUse case
When using multiple cameras (e.g. secondary cameras for minimaps, portals, etc.), you often want to exclude entire UI hierarchies from certain cameras. Currently, you must manually set layers on every object in the hierarchy. With
recursive, you set it once on the parent.We have been using three.js with react-three-fiber and @pmndrs/uikit. Internally, we built a few different solutions, but it was quite tedious without affecting the renderer directly.
Looking forward to feedback!