Fix Transform world scale dirty error
#2408
Conversation
WalkthroughThe changes involve modifications to the Changes
Suggested labels
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2408 +/- ##
==========================================
+ Coverage 69.48% 70.76% +1.28%
==========================================
Files 522 526 +4
Lines 27356 27531 +175
Branches 4096 4123 +27
==========================================
+ Hits 19009 19483 +474
+ Misses 6847 6492 -355
- Partials 1500 1556 +56
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
tests/src/core/Transform.test.ts (1)
39-39: Fix string literal consistency.
Use double quotes for string literals to maintain consistency with the codebase style.
- console.log('lossyWorldScale', JSON.stringify(transform.lossyWorldScale));
+ console.log("lossyWorldScale", JSON.stringify(transform.lossyWorldScale));Also applies to: 42-42
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/core/src/Transform.ts (4 hunks)
- tests/src/core/Transform.test.ts (1 hunks)
🧰 Additional context used
🪛 eslint
tests/src/core/Transform.test.ts
[error] 42-42: Replace 'lossyWorldScale' with "lossyWorldScale"
(prettier/prettier)
🔇 Additional comments (5)
packages/core/src/Transform.ts (5)
6-6: LGTM!
The import statement correctly imports deepClone and ignoreClone from ./clone/CloneManager.
618-618: LGTM!
The added comment accurately reflects that getting worldScale will trigger scaling updates of itself and all parent entities.
622-626: Utilizing new TransformModifyFlags in _updateWorldRotationFlag
The method _updateWorldRotationFlag now correctly uses TransformModifyFlags.WmWeWqWs to represent the combined flags for world matrix, world euler, world quaternion, and world scale updates. Calling _updateAllWorldFlag() on child transforms ensures that the world position, rotation, and scale of all child entities are updated when the parent's rotation changes.
726-726: Dispatching update flags in _worldAssociatedChange
Adding this._updateFlagManager.dispatch(type); ensures that update flags are properly dispatched whenever world-associated changes occur, allowing dependent systems to react accordingly.
835-836: Adding new combined flags to TransformModifyFlags
The addition of WmWeWqWs with the value 0xb8 correctly represents the combination of WorldMatrix, WorldEuler, WorldQuat, and WorldScale flags. The computed value matches the expected bitwise OR of the individual flags.
tests/src/core/Transform.test.ts
Outdated
| it("World Scale", () => { | ||
| const root = scene.createRootEntity(); | ||
| root.transform.setScale(1, 2, 3); | ||
|
|
||
| const entity = root.createChild(); | ||
| const transform = entity.transform; | ||
| transform.setScale(4, 5, 6); | ||
| transform.setRotation(0, 0, 0); | ||
|
|
||
| // expect(transform.lossyWorldScale).to.deep.equal(new Vector3(-0.7071067811865476, -0, -0.7071067811865476)); | ||
| console.log('lossyWorldScale', JSON.stringify(transform.lossyWorldScale)); | ||
| transform.setRotation(90, 0, 0); | ||
| // expect(transform.lossyWorldScale).to.deep.equal(new Vector3(-0.7071067811865476, -0, -0.7071067811865476)); | ||
| console.log('lossyWorldScale', JSON.stringify(transform.lossyWorldScale)); | ||
| }); |
There was a problem hiding this comment.
Test implementation is incomplete and needs proper assertions.
The test case appears to be in a development state with:
- Commented-out assertions that need to be uncommented and updated with correct expected values
- Debug console.log statements that should be removed
- Missing verification of the actual world scale behavior after rotation
Please complete the test implementation by:
- Removing the console.log statements
- Adding proper assertions to verify the world scale values
- Following the assertion pattern used in other test cases
Here's a suggested structure:
it("World Scale", () => {
const root = scene.createRootEntity();
root.transform.setScale(1, 2, 3);
const entity = root.createChild();
const transform = entity.transform;
transform.setScale(4, 5, 6);
transform.setRotation(0, 0, 0);
- // expect(transform.lossyWorldScale).to.deep.equal(new Vector3(-0.7071067811865476, -0, -0.7071067811865476));
- console.log('lossyWorldScale', JSON.stringify(transform.lossyWorldScale));
+ expect(transform.lossyWorldScale).to.deep.equal(new Vector3(4, 10, 18));
+
transform.setRotation(90, 0, 0);
- // expect(transform.lossyWorldScale).to.deep.equal(new Vector3(-0.7071067811865476, -0, -0.7071067811865476));
- console.log('lossyWorldScale', JSON.stringify(transform.lossyWorldScale));
+ expect(transform.lossyWorldScale).to.deep.equal(new Vector3(4, 10, 18));
});Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 eslint
[error] 42-42: Replace 'lossyWorldScale' with "lossyWorldScale"
(prettier/prettier)
💡 Codebase verification
Based on the Transform implementation and test file analysis, I can now provide a final response:
Test coverage for world scale calculations needs improvement
The current test is insufficient and the commented-out expectations suggest incorrect assumptions about world scale behavior. The lossyWorldScale implementation shows that:
- For entities without parent, world scale equals local scale
- For entities with parent, world scale is derived from scale matrix decomposition considering:
- Parent's world rotation
- Parent's world scale
- Entity's local rotation
- Entity's local scale
The test should verify:
- Basic scale inheritance (child scale = parent scale * local scale) when no rotations involved
- Scale behavior with parent rotation (using
_getScaleMatrix()internally) - Scale behavior with child rotation
- Scale behavior with both parent and child rotations
- Negative scale values and their impact on face winding
- Scale updates after rotation changes
🔗 Analysis chain
Verify the test coverage for world scale calculations.
The test only covers basic rotation cases (0° and 90°). Consider adding test cases for:
- Different rotation angles
- Combined rotations on multiple axes
- Scale updates after multiple transformations
- Negative scale values
Let's check if there are other world scale related tests:
Let me check the Transform implementation to understand the world scale calculations better.
Let me try a different approach to find the Transform implementation.
Let me try to find the Transform file first.
Now let's examine the Transform implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other world scale related tests
rg -i "worldscale|world.*scale" tests/
Length of output: 621
Script:
#!/bin/bash
# Search for Transform class implementation and world scale related code
ast-grep --pattern 'class Transform {
$$$
get lossyWorldScale() {
$$$
}
$$$
}'
Length of output: 92
Script:
#!/bin/bash
# Search for Transform class and world scale related code
rg -A 10 "class Transform" src/
Length of output: 78
Script:
#!/bin/bash
# Find Transform related files
fd Transform
Length of output: 196
Script:
#!/bin/bash
# Check the Transform implementation
cat packages/core/src/Transform.ts
Length of output: 29190
🧰 Tools
🪛 eslint
[error] 42-42: Replace 'lossyWorldScale' with "lossyWorldScale"
(prettier/prettier)
There was a problem hiding this comment.
@cptbtptpbcptdtptp, thank you for fixing the issue!
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
tests/src/core/Transform.test.ts (1)
37-38: Consider adding more rotation test cases
The current test correctly verifies world scale behavior with a 90-degree X-axis rotation. However, to ensure robust coverage of the world scale dirty flag fix, consider adding test cases for:
- Different rotation angles (e.g., 45°, 180°)
- Rotations on other axes (Y and Z)
- Combined rotations on multiple axes
- Scale updates after multiple transformation changes
Example additional test case:
// Test Y-axis rotation
transform.setRotation(0, 90, 0);
expect(transform.lossyWorldScale).to.deep.equal(new Vector3(4, 10, 18));
// Test combined rotation
transform.setRotation(45, 45, 45);
expect(transform.lossyWorldScale).to.deep.equal(/* calculate expected values */);📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- tests/src/core/Transform.test.ts (1 hunks)
🔇 Additional comments (2)
tests/src/core/Transform.test.ts (2)
29-35: LGTM: Well-structured test setup
The test setup effectively creates a clear parent-child hierarchy with distinct scale values that make it easy to verify the world scale calculations.
36-36: LGTM: Correct world scale verification
The assertion correctly verifies that world scale is calculated as component-wise multiplication of parent and local scales when no rotation is applied.
GuoLei1990
changed the title
Transform world scale dirty error.Transform world scale dirty error
packages/core/src/Transform.ts
Outdated
| const nodeChildren = this._entity._children; | ||
| for (let i: number = 0, n: number = nodeChildren.length; i < n; i++) { | ||
| nodeChildren[i].transform?._updateWorldPositionAndRotationFlag(); // Rotation update of parent entity will trigger world position and rotation update of all child entity. | ||
| let worldScaleDirty = !this._isParentWorldUniformScaling(); |
There was a problem hiding this comment.
Bad performance！Violates the core logic of performance optimization
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
packages/core/src/Transform.ts (1)
877-893: Enhance documentation for scale change handling.The scale change handling logic is complex and would benefit from detailed documentation explaining:
- The relationship between local and world uniform scaling
- The conditions triggering different update paths
- The performance implications of uniform vs non-uniform scaling
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 889-889: packages/core/src/Transform.ts#L889
Added line #L889 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/core/src/Transform.ts(12 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/core/src/Transform.ts
[warning] 573-574: packages/core/src/Transform.ts#L573-L574
Added lines #L573 - L574 were not covered by tests
[warning] 685-686: packages/core/src/Transform.ts#L685-L686
Added lines #L685 - L686 were not covered by tests
[warning] 690-690: packages/core/src/Transform.ts#L690
Added line #L690 was not covered by tests
[warning] 720-720: packages/core/src/Transform.ts#L720
Added line #L720 was not covered by tests
[warning] 889-889: packages/core/src/Transform.ts#L889
Added line #L889 was not covered by tests
🔇 Additional comments (2)
packages/core/src/Transform.ts (2)
53-56: LGTM: Well-structured property additions for uniform scaling tracking.
The new boolean properties _worldUniformScaling and _localUniformScaling are properly initialized and follow the existing naming conventions.
633-638: LGTM: Optimized transform flag handling.
The flag handling now efficiently considers uniform scaling state, reducing unnecessary updates when scaling is uniform. The new WmWeWqWs flag appropriately combines world transformation flags with scaling.
Also applies to: 914-915
packages/core/src/Transform.ts
Outdated
| this._worldUniformScaling = worldUniformScaling; | ||
| this._entity._children.forEach((child) => { |
There was a problem hiding this comment.
Critical: Add test coverage for scale-related code paths.
Several critical code paths lack test coverage:
- World uniform scaling propagation (lines 573-574)
- Child transform updates (lines 685-686, 690)
- World position and scale flag updates (lines 720, 889)
Would you like me to help generate comprehensive test cases for these scenarios?
Also applies to: 685-686, 690-690, 720-720, 889-889
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 573-574: packages/core/src/Transform.ts#L573-L574
Added lines #L573 - L574 were not covered by tests
packages/core/src/Transform.ts
Outdated
| const worldUniformScaling = this._localUniformScaling; | ||
| if (worldUniformScaling !== this._worldUniformScaling) { | ||
| this._worldUniformScaling = worldUniformScaling; | ||
| this._entity._children.forEach((child) => { | ||
| child.transform?._updateWorldPositionScaleFlagAndUniformScaling(worldUniformScaling); | ||
| }); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Add test coverage for uniform scaling propagation
The test file tests/src/core/Transform.test.ts lacks coverage for uniform scaling propagation between parent and child entities. While it tests basic world scaling scenarios, it doesn't verify the uniform scaling flag propagation behavior. Add test cases to cover:
- Parent uniform scaling changes affecting children's
_worldUniformScalingflag - Verification of scaling propagation through multiple levels of hierarchy
- Mixed uniform/non-uniform scaling scenarios
Example locations needing coverage:
tests/src/core/Transform.test.ts: Add new test cases under the "World Scale" or "Parent Dirty" test suites
🔗 Analysis chain
Add test coverage for uniform scaling propagation.
The parent change handling for uniform scaling propagation is not covered by tests. Consider adding test cases for:
- Parent uniform scaling changes affecting children
- Mixed uniform/non-uniform scaling scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check existing test coverage for parent-child scaling
rg -l "test.*parent.*scale" tests/
Length of output: 36
Script:
#!/bin/bash
# Let's try a broader search for transform and scale related tests
rg -l "transform.*scale|scale.*transform" tests/
# Also search for any test files related to Transform
fd "transform.*test" tests/
# Check for existing test implementations involving parent-child relationships
rg -A 5 "parent.*child.*transform|transform.*parent.*child" tests/
Length of output: 304
Script:
#!/bin/bash
# Let's examine the Transform test file content specifically
cat tests/src/core/Transform.test.ts
# Also check the implementation of _updateWorldPositionScaleFlagAndUniformScaling
ast-grep --pattern 'class Transform {
$$$
_updateWorldPositionScaleFlagAndUniformScaling($_) {
$$$
}
$$$
}'
Length of output: 2408
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 573-574: packages/core/src/Transform.ts#L573-L574
Added lines #L573 - L574 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
packages/core/src/Transform.ts (4)
266-279: Consider optimizing scale comparison in localMatrix setter.The scale comparison for uniform scaling could be optimized to avoid redundant comparisons.
- this._localUniformScaling = scale.x === scale.y && scale.y === scale.z; + const x = scale.x, y = scale.y, z = scale.z; + this._localUniformScaling = x === y && x === z;
839-848: Add documentation for uniform scaling behavior.The scale change handling logic should be documented to explain the uniform scaling check and its impact on world transform updates.
@ignoreClone private _onScaleChanged(): void { + // Check if scale components are uniform (all equal) + // Updates appropriate flags based on uniform scaling state const { x, y, z } = this._scale; this._setDirtyFlagTrue(TransformModifyFlags.LocalMatrix); const localUniformScaling = x == y && y == z;
850-857: Document world uniform scaling calculation.The world uniform scaling check is a critical part of the transform hierarchy but lacks documentation explaining its purpose and behavior.
+ /** + * Checks if world uniform scaling state has changed. + * World uniform scaling is true when both local and parent world scaling are uniform. + * @returns True if world scaling is uniform + * @private + */ private _getWorldUniformScalingChange(): boolean {
873-889: Document new transform modify flags.The new flags and their combinations should be documented to explain their purpose and usage.
/** * @internal */ export enum TransformModifyFlags { + /** Flag indicating world uniform scaling state needs update */ WorldUniformScaling = 0x100, + /** Combined flags for world matrix and position updates */ WmWp = 0x84, + /** Combined flags for world matrix, euler, and quaternion updates */ WmWeWq = 0x98, + /** Combined flags including world scale updates */ WmWeWqWs = 0xb8, + /** Combined flags including uniform scaling updates */ WmWpWeWqWsWus = 0x1bc
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/core/src/Transform.ts(11 hunks)
🔇 Additional comments (1)
packages/core/src/Transform.ts (1)
627-632: Verify transform hierarchy updates.
The world rotation flag updates now consider uniform scaling state. Ensure this change doesn't break existing transform hierarchies.
✅ Verification successful
Based on the code analysis from previous results, I can now generate a final response:
Transform hierarchy flag updates are correctly implemented
The implementation correctly handles transform hierarchy updates with the following verified aspects:
- Proper flag propagation from parent to children through
_entity._children.forEach - Correct handling of uniform scaling state via
_getWorldUniformScalingChange() - Appropriate flag combinations for different update scenarios (WmWeWq vs WmWeWqWs)
- Complete world transform update chain through position, rotation, and scale
The changes maintain the transform hierarchy integrity while adding the uniform scaling optimization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any existing transform hierarchy tests
rg -l "transform.*hierarchy|hierarchy.*transform" tests/
# Look for potential parent-child transform test cases
rg -A 5 "parent.*transform.*child|child.*transform.*parent" tests/
Length of output: 127
Script:
#!/bin/bash
# Search for transform-related test files
fd "transform.*test" tests/
# Look for transform hierarchy-related code and tests with more generic patterns
rg -i "parent.*child.*transform|transform.*parent.*child" -A 5
# Search for transform update method implementations
ast-grep --pattern 'function $_updateWorldPositionAndRotationFlag() {
$$$
}'
# Look for transform flag-related code
rg "TransformModifyFlags" -A 3
Length of output: 14312
Script:
#!/bin/bash
# Check transform test cases
rg -A 10 "describe.*Transform" tests/src/core/Transform.test.ts
# Look for specific test cases related to parent-child transform updates
rg -A 10 "test.*parent.*child.*transform|test.*transform.*hierarchy" tests/src/core/Transform.test.ts
# Search for uniform scaling test cases
rg -A 5 "uniform.*scaling|worldUniformScaling" tests/src/core/Transform.test.ts
Length of output: 592
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/core/src/Transform.ts (1)
644-652: Optimize parent transform caching.The
_getParentTransform()is called multiple times in close succession. Consider caching the parent transform result temporarily during the update cycle to reduce redundant parent traversal.private _updateWorldRotationFlag() { const parent = this._getParentTransform(); + // Cache parent transform during update const parentWorldUniformScaling = parent ? parent.worldUniformScaling : true; let flags = parentWorldUniformScaling ? TransformModifyFlags.WmWeWq : TransformModifyFlags.WmWeWqWs; if (!this._isContainDirtyFlags(flags)) { this._worldAssociatedChange(flags); flags = this.worldUniformScaling ? TransformModifyFlags.WmWpWeWq : TransformModifyFlags.WmWpWeWqWs; this._entity._children.forEach((child) => { child.transform?._updateWorldPositionAndRotationFlag(flags); }); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/core/src/Transform.ts(13 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/core/src/Transform.ts
[warning] 266-266: packages/core/src/Transform.ts#L266
Added line #L266 was not covered by tests
[warning] 268-269: packages/core/src/Transform.ts#L268-L269
Added lines #L268 - L269 were not covered by tests
[warning] 271-271: packages/core/src/Transform.ts#L271
Added line #L271 was not covered by tests
[warning] 273-273: packages/core/src/Transform.ts#L273
Added line #L273 was not covered by tests
[warning] 275-275: packages/core/src/Transform.ts#L275
Added line #L275 was not covered by tests
[warning] 281-282: packages/core/src/Transform.ts#L281-L282
Added lines #L281 - L282 were not covered by tests
[warning] 284-284: packages/core/src/Transform.ts#L284
Added line #L284 was not covered by tests
[warning] 701-702: packages/core/src/Transform.ts#L701-L702
Added lines #L701 - L702 were not covered by tests
🔇 Additional comments (5)
packages/core/src/Transform.ts (5)
53-56: LGTM: New uniform scaling properties.
The addition of _worldUniformScaling and _localUniformScaling properties helps track uniform scaling states efficiently.
320-330: LGTM: Well-implemented uniform scaling getter.
The worldUniformScaling getter correctly propagates uniform scaling state through the transform hierarchy by considering both local and parent states.
860-868: LGTM: Improved scale change handling.
The _onScaleChanged method now correctly detects uniform scaling changes and propagates appropriate flags through the transform hierarchy.
884-903: LGTM: Well-structured transform modification flags.
The new flags for uniform scaling are well-organized and follow the existing bit flag pattern. The compound flags are clearly documented with their components.
701-704:
Add test coverage for world position and scale flag updates.
The world position and scale flag update logic lacks test coverage. This is important for ensuring correct transform hierarchy updates.
#!/bin/bash
# Check existing test coverage for world position and scale updates
rg -A 5 "describe.*world.*scale|test.*world.*scale" tests/🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 701-702: packages/core/src/Transform.ts#L701-L702
Added lines #L701 - L702 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
packages/core/src/Transform.ts (2)
320-330: Consider caching worldUniformScaling computationThe worldUniformScaling getter recalculates the value by traversing up the transform hierarchy each time. Consider caching the result and invalidating only when necessary to improve performance.
private _worldUniformScaling: boolean = true; +private _worldUniformScalingVersion: number = 0; +private _cachedWorldUniformScaling: boolean = true; get worldUniformScaling(): boolean { if (this._isContainDirtyFlag(TransformModifyFlags.WorldUniformScaling)) { + if (this._worldUniformScalingVersion === this._updateFlagManager.version) { + return this._cachedWorldUniformScaling; + } const parent = this._getParentTransform(); - this._worldUniformScaling = this._localUniformScaling && (parent ? parent.worldUniformScaling : true); + this._cachedWorldUniformScaling = this._localUniformScaling && (parent ? parent.worldUniformScaling : true); + this._worldUniformScalingVersion = this._updateFlagManager.version; this._setDirtyFlagFalse(TransformModifyFlags.WorldUniformScaling); } - return this._worldUniformScaling; + return this._cachedWorldUniformScaling; }
860-868: Reduce code duplication in uniform scaling detectionThe uniform scaling detection logic is duplicated between the scale change handler and matrix setter. Consider extracting this into a shared method.
+private _updateUniformScaling(x: number, y: number, z: number): void { + const localUniformScaling = x === y && y === z; + if (this._localUniformScaling !== localUniformScaling) { + this._localUniformScaling = localUniformScaling; + this._updateWorldScaleFlag(TransformModifyFlags.WmWsWus); + } else { + this._updateWorldScaleFlag(TransformModifyFlags.WmWs); + } +} private _onScaleChanged(): void { const { x, y, z } = this._scale; this._setDirtyFlagTrue(TransformModifyFlags.LocalMatrix); - const localUniformScaling = x === y && y === z; - if (this._localUniformScaling !== localUniformScaling) { - this._localUniformScaling = localUniformScaling; - this._updateWorldScaleFlag(TransformModifyFlags.WmWsWus); - } else { - this._updateWorldScaleFlag(TransformModifyFlags.WmWs); - } + this._updateUniformScaling(x, y, z); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/core/src/Transform.ts(14 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/core/src/Transform.ts
[warning] 266-266: packages/core/src/Transform.ts#L266
Added line #L266 was not covered by tests
[warning] 268-269: packages/core/src/Transform.ts#L268-L269
Added lines #L268 - L269 were not covered by tests
[warning] 271-271: packages/core/src/Transform.ts#L271
Added line #L271 was not covered by tests
[warning] 273-273: packages/core/src/Transform.ts#L273
Added line #L273 was not covered by tests
[warning] 275-275: packages/core/src/Transform.ts#L275
Added line #L275 was not covered by tests
[warning] 281-282: packages/core/src/Transform.ts#L281-L282
Added lines #L281 - L282 were not covered by tests
[warning] 284-284: packages/core/src/Transform.ts#L284
Added line #L284 was not covered by tests
[warning] 701-702: packages/core/src/Transform.ts#L701-L702
Added lines #L701 - L702 were not covered by tests
🔇 Additional comments (2)
packages/core/src/Transform.ts (2)
6-6: LGTM: New properties are well-structured
The new uniform scaling flags are properly initialized and decorated with @assignmentClone for efficient cloning.
Also applies to: 30-31, 38-39
884-884: LGTM: Transform flags are well-organized
The new flags for uniform scaling and their combinations are logically structured and follow the existing pattern.
Also applies to: 890-891, 896-897, 901-903
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
packages/core/src/Transform.ts (1)
Line range hint
629-722: Consider optimizing recursive flag updatesThe current implementation recursively updates flags through the entity hierarchy, which could impact performance with deep hierarchies. Consider:
- Using a queue-based approach to avoid deep recursion
- Batching flag updates to reduce the number of traversals
Example optimization:
private _updateWorldPositionFlag(): void { if (!this._isContainDirtyFlags(TransformModifyFlags.WmWp)) { this._worldAssociatedChange(TransformModifyFlags.WmWp); - const children = this._entity._children; - for (let i = 0, n = children.length; i < n; i++) { - children[i].transform?._updateWorldPositionFlag(); - } + this._queueFlagUpdate(TransformModifyFlags.WmWp); } } +private _queueFlagUpdate(flags: TransformModifyFlags): void { + const queue: Transform[] = [this]; + while (queue.length > 0) { + const transform = queue.shift(); + transform._entity._children.forEach(child => { + if (child.transform) { + child.transform._worldAssociatedChange(flags); + queue.push(child.transform); + } + }); + } +}🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 705-707: packages/core/src/Transform.ts#L705-L707
Added lines #L705 - L707 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/core/src/Transform.ts(14 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/core/src/Transform.ts
[warning] 266-266: packages/core/src/Transform.ts#L266
Added line #L266 was not covered by tests
[warning] 268-269: packages/core/src/Transform.ts#L268-L269
Added lines #L268 - L269 were not covered by tests
[warning] 271-271: packages/core/src/Transform.ts#L271
Added line #L271 was not covered by tests
[warning] 273-273: packages/core/src/Transform.ts#L273
Added line #L273 was not covered by tests
[warning] 275-275: packages/core/src/Transform.ts#L275
Added line #L275 was not covered by tests
[warning] 281-282: packages/core/src/Transform.ts#L281-L282
Added lines #L281 - L282 were not covered by tests
[warning] 284-284: packages/core/src/Transform.ts#L284
Added line #L284 was not covered by tests
[warning] 705-707: packages/core/src/Transform.ts#L705-L707
Added lines #L705 - L707 were not covered by tests
🔇 Additional comments (1)
packages/core/src/Transform.ts (1)
266-285: Document uniform scaling detection logic and add tests
The matrix decomposition logic handles event handlers well, but:
- Add a comment explaining the uniform scaling detection logic (x === y && y === z)
- Add test coverage for matrix decomposition with both uniform and non-uniform scaling scenarios
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 266-266: packages/core/src/Transform.ts#L266
Added line #L266 was not covered by tests
[warning] 268-269: packages/core/src/Transform.ts#L268-L269
Added lines #L268 - L269 were not covered by tests
[warning] 271-271: packages/core/src/Transform.ts#L271
Added line #L271 was not covered by tests
[warning] 273-273: packages/core/src/Transform.ts#L273
Added line #L273 was not covered by tests
[warning] 275-275: packages/core/src/Transform.ts#L275
Added line #L275 was not covered by tests
[warning] 281-282: packages/core/src/Transform.ts#L281-L282
Added lines #L281 - L282 were not covered by tests
[warning] 284-284: packages/core/src/Transform.ts#L284
Added line #L284 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
packages/core/src/Transform.ts (2)
265-284: Consider caching scale components for performanceThe scale components are accessed multiple times during uniform scaling check. Consider caching them in local variables:
- const localUniformScaling = scale.x === scale.y && scale.y === scale.z; + const { x, y, z } = scale; + const localUniformScaling = x === y && y === z;
593-593: Consider batching flag updates for better performanceThe current implementation updates flags and traverses the entity hierarchy separately for each type of update. Consider batching flag updates to reduce redundant traversals.
private _updateWorldPositionAndRotationFlag(flags: TransformModifyFlags): void { if (!this._isContainDirtyFlags(flags)) { - this._worldAssociatedChange(flags); + this._batchWorldAssociatedChange(flags); if (flags === TransformModifyFlags.WmWpWeWq && !this._getWorldUniformScaling()) { flags = TransformModifyFlags.WmWpWeWqWs; } const children = this._entity._children; for (let i = 0, n = children.length; i < n; i++) { children[i].transform?._updateWorldPositionAndRotationFlag(flags); } } } +private _batchWorldAssociatedChange(flags: TransformModifyFlags): void { + if (!this._pendingFlags) { + this._pendingFlags = 0; + queueMicrotask(() => this._processPendingFlags()); + } + this._pendingFlags |= flags; +}Also applies to: 633-635, 645-663, 671-684, 693-704, 711-732
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/core/src/Transform.ts(13 hunks)
🔇 Additional comments (3)
packages/core/src/Transform.ts (3)
6-6: LGTM: Well-structured uniform scaling properties
The new properties for tracking uniform scaling are well-designed with appropriate decorators and initialization.
Also applies to: 30-31, 38-39
319-334: LGTM: Well-implemented world uniform scaling computation
The method efficiently computes and caches the world uniform scaling state, properly handling the parent-child relationship.
900-900: LGTM: Well-structured transform modification flags
The new flags for uniform scaling are well-organized and maintain the existing binary pattern. The combined flags provide efficient handling of common operations.
Also applies to: 912-913, 917-919
packages/core/src/Transform.ts
Outdated
| /** | ||
| * @internal | ||
| */ | ||
| _getWorldUniformScaling(): boolean { | ||
| if (this._isContainDirtyFlag(TransformModifyFlags.WorldUniformScaling)) { | ||
| const localUniformScaling = this._localUniformScaling; | ||
| if (localUniformScaling) { | ||
| const parent = this._getParentTransform(); | ||
| this._worldUniformScaling = localUniformScaling && (parent ? parent._getWorldUniformScaling() : true); | ||
| } else { | ||
| this._worldUniformScaling = false; | ||
| } | ||
| this._setDirtyFlagFalse(TransformModifyFlags.WorldUniformScaling); | ||
| } | ||
| return this._worldUniformScaling; | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Test coverage for uniform scaling needs improvement
The current test coverage is insufficient for the uniform scaling functionality. The existing tests only cover basic scale operations and lossy world scale calculations, but missing critical test cases for:
- Uniform scaling flag inheritance
- Local and world uniform scaling state changes
- Edge cases with mixed uniform/non-uniform scaling in hierarchy
Required test cases:
- Setting identical x/y/z scale values and verifying
_localUniformScaling - Parent-child uniform scaling inheritance scenarios
- Flag updates when switching between uniform and non-uniform scaling
- Interaction with rotation and scale decomposition
🔗 Analysis chain
Verify test coverage for uniform scaling
Please ensure comprehensive test coverage for the uniform scaling functionality:
- Setting and getting uniform scaling flags
- Inheritance through parent-child relationships
- Flag updates when uniform scaling changes
- Scale matrix decomposition with uniform scaling
Also applies to: 876-884
Let me gather more information about the test coverage by searching for specific test cases related to uniform scaling and scale modifications.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check existing test coverage for uniform scaling
rg -A 5 "uniformScaling|_localUniformScaling|_worldUniformScaling" tests/src/core/Transform.test.ts
# Check scale-related tests
rg -i "scale|scaling" tests/src/core/Transform.test.ts
Length of output: 411
Script:
#!/bin/bash
# Search for test cases related to scale modifications and transformations
ast-grep --pattern 'it("$_", () => {
$$$
transform.setScale($$$);
$$$
})'
# Search for any scale-related assertions
rg -B 2 -A 2 "expect.*scale" tests/src/core/Transform.test.ts
# Look for uniform scaling related code in Transform class
ast-grep --pattern 'class Transform {
$$$
setScale($$$) {
$$$
}
$$$
}'
Length of output: 1080
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/core/src/Transform.ts (1)
658-666: Consider optimizing transform flag updatesThe current implementation updates flags and traverses the entity hierarchy separately for each type of update. Consider:
- Batching flag updates to reduce redundant traversals
- Using a dirty list pattern to track affected entities
Example optimization:
private _updateWorldPositionAndRotationFlag(flags: TransformModifyFlags): void { if (!this._isContainDirtyFlags(flags)) { this._worldAssociatedChange(flags); - if (flags === TransformModifyFlags.WmWpWeWq && !this._getWorldUniformScaling()) { - flags = TransformModifyFlags.WmWpWeWqWs; - } - const children = this._entity._children; - for (let i = 0, n = children.length; i < n; i++) { - children[i].transform?._updateWorldPositionAndRotationFlag(flags); - } + this._queueFlagUpdate(flags); } } +private _queueFlagUpdate(flags: TransformModifyFlags): void { + if (!this._pendingUpdates) { + this._pendingUpdates = new Set(); + queueMicrotask(() => this._processPendingUpdates()); + } + this._pendingUpdates.add(flags); +}Also applies to: 678-684, 696-701, 710-715
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 662-662: packages/core/src/Transform.ts#L662
Added line #L662 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/core/src/Transform.ts(12 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/core/src/Transform.ts
[warning] 265-265: packages/core/src/Transform.ts#L265
Added line #L265 was not covered by tests
[warning] 267-268: packages/core/src/Transform.ts#L267-L268
Added lines #L267 - L268 were not covered by tests
[warning] 270-270: packages/core/src/Transform.ts#L270
Added line #L270 was not covered by tests
[warning] 272-272: packages/core/src/Transform.ts#L272
Added line #L272 was not covered by tests
[warning] 274-274: packages/core/src/Transform.ts#L274
Added line #L274 was not covered by tests
[warning] 280-281: packages/core/src/Transform.ts#L280-L281
Added lines #L280 - L281 were not covered by tests
[warning] 283-283: packages/core/src/Transform.ts#L283
Added line #L283 was not covered by tests
[warning] 638-638: packages/core/src/Transform.ts#L638
Added line #L638 was not covered by tests
[warning] 662-662: packages/core/src/Transform.ts#L662
Added line #L662 was not covered by tests
[warning] 698-700: packages/core/src/Transform.ts#L698-L700
Added lines #L698 - L700 were not covered by tests
🔇 Additional comments (3)
packages/core/src/Transform.ts (3)
6-6: LGTM: Property additions are well-structured
The new properties for tracking uniform scaling are properly initialized and decorated with @assignmentClone.
Also applies to: 30-31, 38-39
859-867:
Use strict equality for scale comparisons
The scale comparison should use strict equality for more precise floating-point comparisons.
Apply this diff:
- const localUniformScaling = x == y && y == z;
+ const localUniformScaling = x === y && y === z;Likely invalid or redundant comment.
265-284: Add test coverage for matrix decomposition and uniform scaling detection
The matrix decomposition and uniform scaling detection logic needs test coverage, particularly:
- Matrix decomposition with event handler reassignment
- Uniform scaling detection for various scale values
- World flag updates based on scaling changes
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 265-265: packages/core/src/Transform.ts#L265
Added line #L265 was not covered by tests
[warning] 267-268: packages/core/src/Transform.ts#L267-L268
Added lines #L267 - L268 were not covered by tests
[warning] 270-270: packages/core/src/Transform.ts#L270
Added line #L270 was not covered by tests
[warning] 272-272: packages/core/src/Transform.ts#L272
Added line #L272 was not covered by tests
[warning] 274-274: packages/core/src/Transform.ts#L274
Added line #L274 was not covered by tests
[warning] 280-281: packages/core/src/Transform.ts#L280-L281
Added lines #L280 - L281 were not covered by tests
[warning] 283-283: packages/core/src/Transform.ts#L283
Added line #L283 was not covered by tests
packages/core/src/Transform.ts
Outdated
| private _getWorldUniformScaling(): boolean { | ||
| if (this._isContainDirtyFlag(TransformModifyFlags.WorldUniformScaling)) { | ||
| const localUniformScaling = this._localUniformScaling; | ||
| if (localUniformScaling) { | ||
| const parent = this._getParentTransform(); | ||
| this._worldUniformScaling = localUniformScaling && (parent ? parent._getWorldUniformScaling() : true); | ||
| } else { | ||
| this._worldUniformScaling = false; | ||
| } | ||
| this._setDirtyFlagFalse(TransformModifyFlags.WorldUniformScaling); | ||
| } | ||
| return this._worldUniformScaling; |
There was a problem hiding this comment.
💡 Codebase verification
Add test coverage for world uniform scaling inheritance
The codebase currently lacks specific test coverage for the _getWorldUniformScaling method and uniform scaling inheritance scenarios. While there are basic scale-related tests, they don't cover:
- Uniform vs non-uniform scaling detection
- Parent-child uniform scaling inheritance chain
- Dirty flag handling for cached results
Suggested test cases to add:
- Test uniform scaling detection with equal x/y/z values
- Test non-uniform scaling detection with different x/y/z values
- Test parent-child inheritance with mixed uniform/non-uniform scaling
- Test dirty flag invalidation when parent or child scaling changes
🔗 Analysis chain
Add test coverage for world uniform scaling inheritance
The _getWorldUniformScaling method needs comprehensive test coverage for:
- Parent-child uniform scaling inheritance
- Cache invalidation scenarios
- Edge cases with mixed uniform/non-uniform scaling in hierarchy
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check test coverage for world uniform scaling
rg -A 5 "_getWorldUniformScaling|_worldUniformScaling" tests/src/core/Transform.test.ts
# Look for parent-child transform tests
rg -A 5 "parent.*transform.*scale|scale.*parent.*transform" tests/src/core/Transform.test.ts
Length of output: 184
Script:
#!/bin/bash
# Let's first check if the test file exists
fd "Transform.test" tests/
# Check for any scale-related tests in Transform tests
rg -i "scale|scaling" tests/src/core/Transform.test.ts
# Look for test files in the entire repository that might contain transform scaling tests
fd -e ts -e js | rg -i "test|spec" | xargs rg -l "transform.*scale|scaling.*transform"
# Check implementation details to understand test requirements
ast-grep --pattern 'class Transform {
$$$
_getWorldUniformScaling() {
$$$
}
$$$
}'
Length of output: 643
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/core/src/Transform.ts (2)
266-285: Consider optimizing scale comparison.The scale comparison could be optimized to avoid redundant comparisons:
- const localUniformScaling = scale.x === scale.y && scale.y === scale.z; + // Compare x with y first, if they're different we know it's not uniform + const localUniformScaling = scale.x === scale.y && (scale.y === scale.z);This change short-circuits the comparison when x and y are different, potentially saving one comparison operation.
655-661: Add test coverage for flag propagation.The flag propagation logic needs comprehensive test coverage, especially for:
- Uniform scaling state changes
- Parent-child flag propagation
- Mixed uniform/non-uniform scaling scenarios
Would you like me to help generate test cases for these scenarios?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/core/src/Transform.ts(13 hunks)
🔇 Additional comments (4)
packages/core/src/Transform.ts (4)
6-6: LGTM: Property declarations are well-structured.
The new uniform scaling properties are properly initialized and decorated with @assignmentClone.
Also applies to: 30-31, 38-39
893-915: LGTM: TransformModifyFlags updates are well-structured.
The new flags are properly documented and follow the existing pattern.
854-862:
Use strict equality for number comparisons.
The use of loose equality (==) for number comparisons could lead to unexpected behavior due to type coercion.
- const localUniformScaling = x == y && y == z;
+ const localUniformScaling = x === y && y === z;Likely invalid or redundant comment.
865-876: LGTM: World uniform scaling computation is well-implemented.
The caching strategy and hierarchy propagation are handled efficiently.
✅ Verification successful
LGTM: World uniform scaling computation is correctly implemented and used
The verification confirms that _getWorldUniformScaling() is:
- Used correctly in parent-child transform hierarchy propagation
- Properly integrated with the transform modification flags system
- Called appropriately in transform update scenarios
- Maintains consistent world uniform scaling state through the transform chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the usage of _getWorldUniformScaling
ast-grep --pattern 'transform._getWorldUniformScaling()'
Length of output: 58
Script:
#!/bin/bash
# Let's try a different approach to find usages of _getWorldUniformScaling
# First, let's search for method calls
rg "_getWorldUniformScaling" -A 2
# Also search for the method definition to understand its visibility and class context
ast-grep --pattern $'class $_ {
$$$
_getWorldUniformScaling() {
$$$
}
$$$
}'
Length of output: 1627
Summary by CodeRabbit
New Features
Bug Fixes
Tests