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 13 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
150 changes: 103 additions & 47 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 { assignmentClone, deepClone, ignoreClone } from "./clone/CloneManager";

/**
* Used to implement transformation related functions.
@@ -27,12 +27,16 @@
private _rotationQuaternion: Quaternion = new Quaternion();
@deepClone
private _scale: Vector3 = new Vector3(1, 1, 1);
@assignmentClone
private _localUniformScaling: boolean = true;
@deepClone
private _worldPosition: Vector3 = new Vector3();
@deepClone
private _worldRotation: Vector3 = new Vector3();
@deepClone
private _worldRotationQuaternion: Quaternion = new Quaternion();
@assignmentClone
private _worldUniformScaling: boolean = true;
@deepClone
private _lossyWorldScale: Vector3 = new Vector3(1, 1, 1);
@deepClone
@@ -258,20 +262,26 @@
if (this._localMatrix !== value) {
this._localMatrix.copyFrom(value);
}

const { _position: position, _rotationQuaternion: rotationQuaternion, _scale: scale } = this;

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

Codecov / codecov/patch

packages/core/src/Transform.ts#L265 

Added line #L265 was not covered by tests
// @ts-ignore
this._position._onValueChanged = this._rotationQuaternion._onValueChanged = this._scale._onValueChanged = null;
this._localMatrix.decompose(this._position, this._rotationQuaternion, this._scale);
position._onValueChanged = rotationQuaternion._onValueChanged = scale._onValueChanged = null;
this._localMatrix.decompose(position, rotationQuaternion, scale);

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

Codecov / codecov/patch

packages/core/src/Transform.ts#L267-L268 

Added lines #L267 - L268 were not covered by tests
// @ts-ignore
this._position._onValueChanged = this._onPositionChanged;
position._onValueChanged = this._onPositionChanged;

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

Codecov / codecov/patch

packages/core/src/Transform.ts#L270 

Added line #L270 was not covered by tests
// @ts-ignore
this._rotationQuaternion._onValueChanged = this._onRotationQuaternionChanged;
rotationQuaternion._onValueChanged = this._onRotationQuaternionChanged;

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

Codecov / codecov/patch

packages/core/src/Transform.ts#L272 

Added line #L272 was not covered by tests
// @ts-ignore
this._scale._onValueChanged = this._onScaleChanged;
scale._onValueChanged = this._onScaleChanged;

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

Codecov / codecov/patch

packages/core/src/Transform.ts#L274 

Added line #L274 was not covered by tests

this._setDirtyFlagTrue(TransformModifyFlags.LocalEuler);
this._setDirtyFlagFalse(TransformModifyFlags.LocalMatrix | TransformModifyFlags.LocalQuat);
this._updateAllWorldFlag();
const localUniformScaling = scale.x === scale.y && scale.y === scale.z;
if (this._localUniformScaling !== localUniformScaling) {
this._localUniformScaling = localUniformScaling;
this._updateAllWorldFlag(TransformModifyFlags.WmWpWeWqWsWus);

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

Codecov / codecov/patch

packages/core/src/Transform.ts#L280-L281 

Added lines #L280 - L281 were not covered by tests
} else {
this._updateAllWorldFlag(TransformModifyFlags.WmWpWeWqWs);

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

Codecov / codecov/patch

packages/core/src/Transform.ts#L283 

Added line #L283 was not covered by tests
}
}

/**
@@ -563,7 +573,7 @@
*/
_parentChange(): void {
this._isParentDirty = true;
this._updateAllWorldFlag();
this._updateAllWorldFlag(TransformModifyFlags.WmWpWeWqWsWus);
}

/**
@@ -603,9 +613,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();
const children = this._entity._children;
for (let i = 0, n = children.length; i < n; i++) {
children[i].transform?._updateWorldPositionFlag();
}
}
}
@@ -615,14 +625,23 @@
* 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 parent = this._getParentTransform();
const parentWorldUniformScaling = parent ? parent._getWorldUniformScaling() : true;
let flags = parentWorldUniformScaling ? TransformModifyFlags.WmWeWq : TransformModifyFlags.WmWeWqWs;
if (!this._isContainDirtyFlags(flags)) {
this._worldAssociatedChange(flags);
if (flags === TransformModifyFlags.WmWeWq && !this._getWorldUniformScaling()) {
flags = TransformModifyFlags.WmWpWeWq;

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

Codecov / codecov/patch

packages/core/src/Transform.ts#L638 

Added line #L638 was not covered by tests
} else {
flags = TransformModifyFlags.WmWpWeWqWs;
}
const children = this._entity._children;
for (let i = 0, n = children.length; i < n; i++) {
children[i].transform?._updateWorldPositionAndRotationFlag(flags); // Rotation update of parent entity will trigger world position, rotation and scale update of all child entity.
}
}
}
@@ -632,14 +651,19 @@
* 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.
* @param flags - Dirty flag
*/
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(flags: TransformModifyFlags): void {
if (!this._isContainDirtyFlags(flags)) {
this._worldAssociatedChange(flags);
if (flags === TransformModifyFlags.WmWpWeWq && !this._getWorldUniformScaling()) {
flags = TransformModifyFlags.WmWpWeWqWs;

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

Codecov / codecov/patch

packages/core/src/Transform.ts#L662 

Added line #L662 was not covered by tests
}
const children = this._entity._children;
for (let i = 0, n = children.length; i < n; i++) {
children[i].transform?._updateWorldPositionAndRotationFlag(flags);
}
}
}
@@ -649,13 +673,15 @@
* 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() {
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();
* @param flags - Dirty flag
*/
private _updateWorldScaleFlag(flags: TransformModifyFlags): void {
if (!this._isContainDirtyFlags(flags)) {
this._worldAssociatedChange(flags);
flags |= TransformModifyFlags.WorldPosition;
const children = this._entity._children;
for (let i = 0, n = children.length; i < n; i++) {
children[i].transform?._updateWorldPositionAndScaleFlag(flags);
}
}
}
@@ -665,26 +691,28 @@
* 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 _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();
* @param flags - Dirty flag
*/
private _updateWorldPositionAndScaleFlag(flags: TransformModifyFlags): void {
if (!this._isContainDirtyFlags(flags)) {
this._worldAssociatedChange(flags);
const children = this._entity._children;
for (let i = 0, n = children.length; i < n; i++) {

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

Codecov / codecov/patch

packages/core/src/Transform.ts#L698-L700 

Added lines #L698 - L700 were not covered by tests
children[i].transform?._updateWorldPositionAndScaleFlag(flags);
}
}
}

/**
* Update all world transform property dirty flag, the principle is the same as above.
*/
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();
* @param flags - Dirty flag
*/
private _updateAllWorldFlag(flags: TransformModifyFlags): void {
if (!this._isContainDirtyFlags(flags)) {
this._worldAssociatedChange(flags);
const children = this._entity._children;
for (let i = 0, n = children.length; i < n; i++) {
children[i].transform?._updateAllWorldFlag(flags);
}
}
}
@@ -739,7 +767,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 +856,29 @@

@ignoreClone
private _onScaleChanged(): void {
const { x, y, z } = this._scale;
this._setDirtyFlagTrue(TransformModifyFlags.LocalMatrix);
this._updateWorldScaleFlag();
const localUniformScaling = x == y && y == z;
if (this._localUniformScaling !== localUniformScaling) {
this._localUniformScaling = localUniformScaling;
this._updateWorldScaleFlag(TransformModifyFlags.WmWsWus);
} else {
this._updateWorldScaleFlag(TransformModifyFlags.WmWs);
}
}

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;
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 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

}
}

@@ -845,17 +894,24 @@
WorldScale = 0x20,
LocalMatrix = 0x40,
WorldMatrix = 0x80,
WorldUniformScaling = 0x100,

/** WorldMatrix | WorldPosition */
WmWp = 0x84,
/** WorldMatrix | WorldEuler | WorldQuat */
WmWeWq = 0x98,
/** WorldMatrix | WorldEuler | WorldQuat | WorldScale*/
WmWeWqWs = 0xb8,
/** WorldMatrix | WorldPosition | WorldEuler | WorldQuat */
WmWpWeWq = 0x9c,
/** WorldMatrix | WorldScale */
WmWs = 0xa0,
/** WorldMatrix | WorldScale | WorldUniformScaling */
WmWsWus = 0x1a0,
/** WorldMatrix | WorldPosition | WorldScale */
WmWpWs = 0xa4,
/** WorldMatrix | WorldPosition | WorldEuler | WorldQuat | WorldScale */
WmWpWeWqWs = 0xbc
WmWpWeWqWs = 0xbc,
/** WorldMatrix | WorldPosition | WorldEuler | WorldQuat | WorldScale | WorldUniformScaling */
WmWpWeWqWsWus = 0x1bc
}
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();