Fix SkinnedRenderer.boundingBox updates are not timely
#2267
Conversation
WalkthroughThe recent updates to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- packages/core/src/Renderer.ts (6 hunks)
- packages/core/src/mesh/SkinnedMeshRenderer.ts (3 hunks)
Additional comments not posted (8)
packages/core/src/mesh/SkinnedMeshRenderer.ts (3)
Line range hint
123-127: LGTM! Consistent use ofthis._transform.worldMatriximproves maintainability.The update simplifies the logic by consistently using
this._transform.worldMatrixfor shader data updates, which should enhance maintainability and performance.
222-224: LGTM! Consistent use ofthis._transform.worldMatrixfor bounding box transformations.The update ensures that the bounding box is transformed using the world matrix of the transform, which is consistent with the rest of the updates.
268-270: LGTM! Properly attaching the transform of the new root bone entity.The update ensures that the transform is correctly associated with the entity when the root bone is altered, improving the handling of skinning updates.
packages/core/src/Renderer.ts (5)
71-72: LGTM! Addition of_transformproperty enhances encapsulation.The new protected property
_transformcentralizes the management of transformation data, improving encapsulation and maintainability.
175-176: LGTM! Addition of_attachTransformmethod improves modularity.The new
_attachTransformmethod integrates the functionality of the removed_registerEntityTransformListenermethod, ensuring that the previous listener is removed before a new one is added, which improves modularity and maintainability.
369-370: LGTM! Ensuring proper cleanup by detaching transform listener.Calling
_attachTransformwithnullin the_onDestroymethod ensures that the transform listener is detached when the renderer is destroyed, which is necessary for proper cleanup.
Line range hint
395-399: LGTM! Consistent use ofthis._transform.worldMatriximproves maintainability.The update simplifies the logic by consistently using
this._transform.worldMatrixfor shader data updates, which should enhance maintainability and performance.
468-471: LGTM! Proper implementation of_attachTransformmethod.The method ensures that the previous listener is removed before a new one is added, improving modularity and maintainability.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
tests/src/core/SkinnedMeshRenderer.test.ts (1)
75-75: Initializepositionwith specific values for clarity.Currently, the
positionis initialized with default values (0, 0, 0). Initializing it with specific values can make the test case more robust and clear.- const position = new Vector3(); + const position = new Vector3(0, 0, 0);
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- packages/core/src/mesh/SkinnedMeshRenderer.ts (3 hunks)
- tests/src/core/SkinnedMeshRenderer.test.ts (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/core/src/mesh/SkinnedMeshRenderer.ts
Additional comments not posted (2)
tests/src/core/SkinnedMeshRenderer.test.ts (2)
89-90: Ensure bounds are correctly updated after setting position.The test correctly checks that the bounds are updated after setting the position. This is a good practice to ensure the bounding box reflects the current transformations.
102-105: Verify bounds update correctly after changingrootBone0position.The test correctly verifies that the bounds are updated after changing the position of
rootBone0. This ensures that the bounding box is dynamically updated based on transformations.
| const modelMesh = new ModelMesh(engine); | ||
| const entity = rootEntity.createChild("SkinEntity"); | ||
| entity.transform.position = position; |
There was a problem hiding this comment.
Set the position of entity after creating rootBone.
Setting the position of entity after creating rootBone ensures that all transformations are applied correctly.
- entity.transform.position = position;Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- packages/core/src/Renderer.ts (6 hunks)
- packages/core/src/mesh/SkinnedMeshRenderer.ts (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/core/src/mesh/SkinnedMeshRenderer.ts
Additional comments not posted (4)
packages/core/src/Renderer.ts (4)
71-72: Addition of_transformproperty looks good.The new protected property
_transformcentralizes the management of transformation data, enhancing encapsulation and potentially improving maintainability.
468-471: Addition of_setTransformmethod looks good.The new protected method
_setTransformmanages the attachment and detachment of transformation listeners, ensuring that the previous listener is removed before a new one is added. This promotes better control flow and modularity.
395-395: Update to_updateTransformShaderDatamethod looks good.The method now uses the new
_transformproperty, ensuring consistent utilization across methods that deal with shader data updates and matrix calculations. This enhances clarity and reduces direct dependencies on theentityobject for transformation data.
445-445: Update to_updateWorldViewRelatedShaderDatamethod looks good.The method now uses the new
_transformproperty, ensuring consistent utilization across methods that deal with shader data updates and matrix calculations. This enhances clarity and reduces direct dependencies on theentityobject for transformation data.
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
What is the current behavior? (You can also link to an open issue here)
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
Other information:
Summary by CodeRabbit
New Features
Bug Fixes