Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Transform world scale dirty error #2408

Merged
merged 14 commits into from
Nov 27, 2024
+117 −50
Merged

Fix Transform world scale dirty error #2408

Changes from 7 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
25bc421
fix: transform world scale dirty error
cptbtptpbcptdtptp Oct 29, 2024
47759ec
feat: update code
cptbtptpbcptdtptp Oct 29, 2024
57572dc
feat: update code
cptbtptpbcptdtptp Oct 29, 2024
ea32723
feat: udpate code
cptbtptpbcptdtptp Oct 29, 2024
9ed1133
feat: update code
cptbtptpbcptdtptp Nov 6, 2024
e1d2d24
feat: update code
cptbtptpbcptdtptp Nov 8, 2024
c62b4de
feat: update code
cptbtptpbcptdtptp Nov 8, 2024
a135e63
feat: update code
cptbtptpbcptdtptp Nov 8, 2024
2040b50
feat: update code
cptbtptpbcptdtptp Nov 8, 2024
7f06062
feat: update code
cptbtptpbcptdtptp Nov 18, 2024
ac54dcc
feat: update code
cptbtptpbcptdtptp Nov 26, 2024
2f71d7a
feat: update code
cptbtptpbcptdtptp Nov 26, 2024
fb35241
feat: update code
cptbtptpbcptdtptp Nov 26, 2024
6c2ea9a
feat: udpate code
cptbtptpbcptdtptp Nov 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
129 changes: 96 additions & 33 deletions packages/core/src/Transform.ts
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import { MathUtil, Matrix, Matrix3x3, Quaternion, Vector3 } from "@galacean/engine-math";
import { BoolUpdateFlag } from "./BoolUpdateFlag";
import { deepClone, ignoreClone } from "./clone/CloneManager";
import { Component } from "./Component";
import { Entity } from "./Entity";
import { UpdateFlagManager } from "./UpdateFlagManager";
import { deepClone, ignoreClone } from "./clone/CloneManager";

/**
* Used to implement transformation related functions.
@@ -50,6 +50,11 @@
private _isParentDirty: boolean = true;
@ignoreClone
private _parentTransformCache: Transform = null;
@ignoreClone
private _worldUniformScaling: boolean = true;
@ignoreClone
private _localUniformScaling: boolean = true;
@ignoreClone
private _dirtyFlag: number = TransformModifyFlags.WmWpWeWqWs;

/** @internal */
@@ -563,6 +568,13 @@
*/
_parentChange(): void {
this._isParentDirty = true;
const worldUniformScaling = this._localUniformScaling;
if (worldUniformScaling !== this._worldUniformScaling) {
this._worldUniformScaling = worldUniformScaling;
this._entity._children.forEach((child) => {

Check warning on line 574 in packages/core/src/Transform.ts

Codecov / codecov/patch

packages/core/src/Transform.ts#L573-L574 

Added lines #L573 - L574 were not covered by tests
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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

child.transform?._updateWorldPositionScaleFlagAndUniformScaling(worldUniformScaling);
});
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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 _worldUniformScaling flag
  • 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

this._updateAllWorldFlag();
}

@@ -603,10 +615,9 @@
private _updateWorldPositionFlag(): void {
if (!this._isContainDirtyFlags(TransformModifyFlags.WmWp)) {
this._worldAssociatedChange(TransformModifyFlags.WmWp);
const nodeChildren = this._entity._children;
for (let i: number = 0, n: number = nodeChildren.length; i < n; i++) {
nodeChildren[i].transform?._updateWorldPositionFlag();
}
this._entity._children.forEach((child) => {
child.transform?._updateWorldPositionFlag();
});
}
}

@@ -615,15 +626,16 @@
* Get worldPosition: Will trigger the worldMatrix, local position update of itself and the worldMatrix update of all parent entities.
* Get worldRotationQuaternion: Will trigger the world rotation (in quaternion) update of itself and all parent entities.
* Get worldRotation: Will trigger the world rotation(in euler and quaternion) update of itself and world rotation(in quaternion) update of all parent entities.
* Get worldScale: Will trigger the scaling update of itself and all parent entities.
* In summary, any update of related variables will cause the dirty mark of one of the full process (worldMatrix or worldRotationQuaternion) to be false.
*/
private _updateWorldRotationFlag() {
if (!this._isContainDirtyFlags(TransformModifyFlags.WmWeWq)) {
this._worldAssociatedChange(TransformModifyFlags.WmWeWq);
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.
}
const dirtyFlag = this._worldUniformScaling ? TransformModifyFlags.WmWeWq : TransformModifyFlags.WmWeWqWs;
if (!this._isContainDirtyFlags(dirtyFlag)) {
this._worldAssociatedChange(dirtyFlag);
this._entity._children.forEach((child) => {
child.transform?._updateWorldPositionAndRotationFlag(); // Rotation update of parent entity will trigger world position, rotation and scale update of all child entity.
});
}
}

@@ -632,15 +644,16 @@
* Get worldPosition: Will trigger the worldMatrix, local position update of itself and the worldMatrix update of all parent entities.
* Get worldRotationQuaternion: Will trigger the world rotation (in quaternion) update of itself and all parent entities.
* Get worldRotation: Will trigger the world rotation(in euler and quaternion) update of itself and world rotation(in quaternion) update of all parent entities.
* Get worldScale: Will trigger the scaling update of itself and all parent entities.
* In summary, any update of related variables will cause the dirty mark of one of the full process (worldMatrix or worldRotationQuaternion) to be false.
*/
private _updateWorldPositionAndRotationFlag() {
if (!this._isContainDirtyFlags(TransformModifyFlags.WmWpWeWq)) {
this._worldAssociatedChange(TransformModifyFlags.WmWpWeWq);
const nodeChildren = this._entity._children;
for (let i: number = 0, n: number = nodeChildren.length; i < n; i++) {
nodeChildren[i].transform?._updateWorldPositionAndRotationFlag();
}
private _updateWorldPositionAndRotationFlag(): void {
const dirtyFlag = this._worldUniformScaling ? TransformModifyFlags.WmWpWeWq : TransformModifyFlags.WmWpWeWqWs;
if (!this._isContainDirtyFlags(dirtyFlag)) {
this._worldAssociatedChange(dirtyFlag);
this._entity._children.forEach((child) => {
child.transform?._updateWorldPositionAndRotationFlag();
});
}
}

@@ -649,14 +662,49 @@
* Get worldPosition: Will trigger the worldMatrix, local position update of itself and the worldMatrix update of all parent entities.
* Get worldScale: Will trigger the scaling update of itself and all parent entities.
* In summary, any update of related variables will cause the dirty mark of one of the full process (worldMatrix) to be false.
* @param worldUniformScaling - Whether the world scaling is uniform
*/
private _updateWorldScaleFlag() {
private _updateWorldScaleFlagAndUniformScaling(worldUniformScaling: boolean): void {
this._isContainDirtyFlags(TransformModifyFlags.WmWs) || this._worldAssociatedChange(TransformModifyFlags.WmWs);
this._entity._children.forEach((child) => {
child.transform?._updateWorldPositionScaleFlagAndUniformScaling(worldUniformScaling);
});
}

/**
* Get worldMatrix: Will trigger the worldMatrix update of itself and all parent entities.
* Get worldPosition: Will trigger the worldMatrix, local position update of itself and the worldMatrix update of all parent entities.
* Get worldScale: Will trigger the scaling update of itself and all parent entities.
* In summary, any update of related variables will cause the dirty mark of one of the full process (worldMatrix) to be false.
* @param parentWorldUniformScaling - Whether the parent world scaling is uniform
*/
private _updateWorldPositionScaleFlagAndUniformScaling(parentWorldUniformScaling: boolean): void {
this._isContainDirtyFlags(TransformModifyFlags.WmWpWs) || this._worldAssociatedChange(TransformModifyFlags.WmWpWs);
const worldUniformScaling = this._localUniformScaling && parentWorldUniformScaling;
if (worldUniformScaling !== this._worldUniformScaling) {
this._worldUniformScaling = worldUniformScaling;
this._entity._children.forEach((child) => {

Check warning on line 686 in packages/core/src/Transform.ts

Codecov / codecov/patch

packages/core/src/Transform.ts#L685-L686 

Added lines #L685 - L686 were not covered by tests
child.transform?._updateWorldPositionScaleFlagAndUniformScaling(worldUniformScaling);
});
} else {
this._entity._children.forEach((child) => {

Check warning on line 690 in packages/core/src/Transform.ts

Codecov / codecov/patch

packages/core/src/Transform.ts#L690 

Added line #L690 was not covered by tests
child.transform?._updateWorldPositionAndScaleFlag();
});
}
}

/**
* Get worldMatrix: Will trigger the worldMatrix update of itself and all parent entities.
* Get worldPosition: Will trigger the worldMatrix, local position update of itself and the worldMatrix update of all parent entities.
* Get worldScale: Will trigger the scaling update of itself and all parent entities.
* In summary, any update of related variables will cause the dirty mark of one of the full process (worldMatrix) to be false.
*/
private _updateWorldScaleFlag(): void {
if (!this._isContainDirtyFlags(TransformModifyFlags.WmWs)) {
this._worldAssociatedChange(TransformModifyFlags.WmWs);
const nodeChildren = this._entity._children;
for (let i: number = 0, n: number = nodeChildren.length; i < n; i++) {
nodeChildren[i].transform?._updateWorldPositionAndScaleFlag();
}
this._entity._children.forEach((child) => {
child.transform?._updateWorldPositionAndScaleFlag();
});
}
}

@@ -669,10 +717,9 @@
private _updateWorldPositionAndScaleFlag(): void {
if (!this._isContainDirtyFlags(TransformModifyFlags.WmWpWs)) {
this._worldAssociatedChange(TransformModifyFlags.WmWpWs);
const nodeChildren = this._entity._children;
for (let i: number = 0, n: number = nodeChildren.length; i < n; i++) {
nodeChildren[i].transform?._updateWorldPositionAndScaleFlag();
}
this._entity._children.forEach((child) => {

Check warning on line 720 in packages/core/src/Transform.ts

Codecov / codecov/patch

packages/core/src/Transform.ts#L720 

Added line #L720 was not covered by tests
child.transform?._updateWorldPositionAndScaleFlag();
});
}
}

@@ -682,10 +729,9 @@
private _updateAllWorldFlag(): void {
if (!this._isContainDirtyFlags(TransformModifyFlags.WmWpWeWqWs)) {
this._worldAssociatedChange(TransformModifyFlags.WmWpWeWqWs);
const nodeChildren = this._entity._children;
for (let i: number = 0, n: number = nodeChildren.length; i < n; i++) {
nodeChildren[i].transform?._updateAllWorldFlag();
}
this._entity._children.forEach((child) => {
child.transform?._updateAllWorldFlag();
});
}
}

@@ -739,7 +785,7 @@

private _worldAssociatedChange(type: number): void {
this._dirtyFlag |= type;
this._updateFlagManager.dispatch(TransformModifyFlags.WorldMatrix);
this._updateFlagManager.dispatch(type);
}

private _rotateByQuat(rotateQuat: Quaternion, relativeToLocal: boolean): void {
@@ -828,8 +874,23 @@

@ignoreClone
private _onScaleChanged(): void {
const { x, y, z } = this._scale;
this._setDirtyFlagTrue(TransformModifyFlags.LocalMatrix);
this._updateWorldScaleFlag();
const isLocalUniformScaling = x == y && y == z;
if (this._localUniformScaling !== isLocalUniformScaling) {
this._localUniformScaling = isLocalUniformScaling;
const parentTransform = this._getParentTransform();
const parentWorldUniformScaling = parentTransform ? parentTransform._worldUniformScaling : true;
const worldUniformScaling = parentWorldUniformScaling && isLocalUniformScaling;
if (this._worldUniformScaling !== worldUniformScaling) {
this._worldUniformScaling = worldUniformScaling;
this._updateWorldScaleFlagAndUniformScaling(worldUniformScaling);
} else {
this._updateWorldScaleFlag();

Check warning on line 889 in packages/core/src/Transform.ts

Codecov / codecov/patch

packages/core/src/Transform.ts#L889 

Added line #L889 was not covered by tests
}
} else {
this._updateWorldScaleFlag();
}
}
}

@@ -850,6 +911,8 @@
WmWp = 0x84,
/** WorldMatrix | WorldEuler | WorldQuat */
WmWeWq = 0x98,
/** WorldMatrix | WorldEuler | WorldQuat | WorldScale*/
WmWeWqWs = 0xb8,
/** WorldMatrix | WorldPosition | WorldEuler | WorldQuat */
WmWpWeWq = 0x9c,
/** WorldMatrix | WorldScale */
12 changes: 12 additions & 0 deletions tests/src/core/Transform.test.ts
View file
Original file line number Diff line number Diff line change
@@ -26,6 +26,18 @@ describe("Transform test", function () {
expect(transform.worldUp).to.deep.equal(new Vector3(0, 1, 0));
});

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(4, 10, 18));
transform.setRotation(90, 0, 0);
expect(transform.lossyWorldScale).to.deep.equal(new Vector3(4, 15, 12));
});

it("Parent Dirty", () => {
const root1 = scene.createRootEntity();
const root2 = scene.createRootEntity();