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

Bug: deserialized scene should have node transforms invalidated at initial #16353

Merged
merged 3 commits into from
Oct 9, 2023
Merged

Bug: deserialized scene should have node transforms invalidated at initial #16353

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
48ca27a
Bug: deserialized scene should have node transforms invalidated
shrinktofit Oct 7, 2023
18815bf
Merge remote-tracking branch 'upstream/v3.8.2' into FixSceneDirty
shrinktofit Oct 7, 2023
abb7ea2
Remove transformFlags modification in _onBatchCreated
shrinktofit Oct 9, 2023
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
1 change: 0 additions & 1 deletion cocos/scene-graph/node.jsb.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1258,7 +1258,6 @@ nodeProto._onActiveNode = function (shouldActiveNow: boolean) {

nodeProto._onBatchCreated = function (dontSyncChildPrefab: boolean) {
this.hasChangedFlags = TRANSFORMBIT_TRS;
this._transformFlags |= TRANSFORMBIT_TRS;
const children = this._children;
const len = children.length;
let child;
Expand Down
3 changes: 1 addition & 2 deletions cocos/scene-graph/node.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@

const idGenerator = new js.IDGenerator('Node');

function getConstructor<T> (typeOrClassName: string | Constructor<T> | AbstractedConstructor<T>): Constructor<T> | AbstractedConstructor<T> | null | undefined {

Check warning on line 53 in cocos/scene-graph/node.ts

View workflow job for this annotation

GitHub Actions / Run ESLint 

This line has a length of 160. Maximum allowed is 150
if (!typeOrClassName) {
errorID(3804);
return null;
Expand Down Expand Up @@ -291,7 +291,7 @@
return null;
}

protected static _findComponents<T extends Component> (node: Node, constructor: Constructor<T> | AbstractedConstructor<T>, components: Component[]): void {

Check warning on line 294 in cocos/scene-graph/node.ts

View workflow job for this annotation

GitHub Actions / Run ESLint 

This line has a length of 159. Maximum allowed is 150
const cls = constructor;
const comps = node._components;
// NOTE: internal rtti property
Expand Down Expand Up @@ -333,7 +333,7 @@
protected static _findChildComponents (children: Node[], constructor, components): void {
for (let i = 0; i < children.length; ++i) {
const node = children[i];
Node._findComponents(node, constructor, components);

Check failure on line 336 in cocos/scene-graph/node.ts

View workflow job for this annotation

GitHub Actions / Run ESLint 

Unsafe argument of type `any` assigned to a parameter of type `Constructor<Component> | AbstractedConstructor<Component>`

Check failure on line 336 in cocos/scene-graph/node.ts

View workflow job for this annotation

GitHub Actions / Run ESLint 

Unsafe argument of type `any` assigned to a parameter of type `Component[]`
if (node._children.length > 0) {
Node._findChildComponents(node._children, constructor, components);
}
Expand Down Expand Up @@ -1004,8 +1004,8 @@
if (Array.isArray(reqComps)) {
for (let i = 0; i < reqComps.length; i++) {
const reqComp = reqComps[i];
if (!this.getComponent(reqComp)) {

Check failure on line 1007 in cocos/scene-graph/node.ts

View workflow job for this annotation

GitHub Actions / Run ESLint 

Unsafe argument of type `any` assigned to a parameter of type `Constructor<Component> | AbstractedConstructor<Component>`
this.addComponent(reqComp);

Check failure on line 1008 in cocos/scene-graph/node.ts

View workflow job for this annotation

GitHub Actions / Run ESLint 

Unsafe argument of type `any` assigned to a parameter of type `Constructor<Component>`
}
}
} else {
Expand Down Expand Up @@ -1090,7 +1090,7 @@
if (component instanceof Component) {
componentInstance = component;
} else {
componentInstance = this.getComponent(component);

Check failure on line 1093 in cocos/scene-graph/node.ts

View workflow job for this annotation

GitHub Actions / Run ESLint 

Unsafe argument of type `any` assigned to a parameter of type `Constructor<Component> | AbstractedConstructor<Component>`
}
if (componentInstance) {
componentInstance.destroy();
Expand Down Expand Up @@ -1144,7 +1144,7 @@
default:
break;
}
this._eventProcessor.on(type as NodeEventType, callback, target, useCapture);

Check failure on line 1147 in cocos/scene-graph/node.ts

View workflow job for this annotation

GitHub Actions / Run ESLint 

Unsafe argument of type `any` assigned to a parameter of type `boolean | undefined`
}

/**
Expand All @@ -1164,7 +1164,7 @@
* ```
*/
public off (type: string, callback?: AnyFunction, target?: unknown, useCapture: any = false): void {
this._eventProcessor.off(type as NodeEventType, callback, target, useCapture);

Check failure on line 1167 in cocos/scene-graph/node.ts

View workflow job for this annotation

GitHub Actions / Run ESLint 

Unsafe argument of type `any` assigned to a parameter of type `boolean | undefined`

const hasListeners = this._eventProcessor.hasEventListener(type);
// All listener removed
Expand Down Expand Up @@ -1192,7 +1192,7 @@
* @param target - The target (this object) to invoke the callback, can be null
*/
public once (type: string, callback: AnyFunction, target?: unknown, useCapture?: any): void {
this._eventProcessor.once(type as NodeEventType, callback, target, useCapture);

Check failure on line 1195 in cocos/scene-graph/node.ts

View workflow job for this annotation

GitHub Actions / Run ESLint 

Unsafe argument of type `any` assigned to a parameter of type `boolean | undefined`
}

/**
Expand Down Expand Up @@ -1245,7 +1245,7 @@
* @zh 移除目标上的所有注册事件。
* @param target - The target to be searched for all related callbacks
*/
public targetOff (target: string | unknown): void {

Check failure on line 1248 in cocos/scene-graph/node.ts

View workflow job for this annotation

GitHub Actions / Run ESLint 

'unknown' overrides all other types in this union type
this._eventProcessor.targetOff(target);
// Check for event mask reset
if ((this._eventMask & TRANSFORM_ON) && !this._eventProcessor.hasEventListener(NodeEventType.TRANSFORM_CHANGED)) {
Expand Down Expand Up @@ -1321,7 +1321,7 @@
if (EDITOR && newPrefabInfo) {
if (cloned === newPrefabInfo.root) {
// when instantiate prefab in Editor,should add prefab instance info for root node
EditorExtends.PrefabUtils.addPrefabInstance?.(cloned);

Check failure on line 1324 in cocos/scene-graph/node.ts

View workflow job for this annotation

GitHub Actions / Run ESLint 

Unsafe argument of type `any` assigned to a parameter of type `Node`
// newPrefabInfo.fileId = '';
} else {
// var PrefabUtils = Editor.require('scene://utils/prefab');
Expand Down Expand Up @@ -1525,7 +1525,7 @@
@serializable
protected _euler = new Vec3();

protected _transformFlags = TransformBit.NONE; // does the world transform need to update?
protected _transformFlags = TransformBit.TRS; // does the world transform need to update?
protected _eulerDirty = false;

protected _flagChangeVersion = 0;
Expand Down Expand Up @@ -1883,7 +1883,6 @@
*/
public _onBatchCreated (dontSyncChildPrefab: boolean): void {
this.hasChangedFlags = TransformBit.TRS;
this._transformFlags |= TransformBit.TRS;
const len = this._children.length;
for (let i = 0; i < len; ++i) {
this._children[i]._siblingIndex = i;
Expand Down
2 changes: 1 addition & 1 deletion native/cocos/core/scene-graph/Node.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -657,7 +657,7 @@ class Node : public CCObject {
// NOTE: TypeArray created in node.jsb.ts _ctor should have the same memory layout
uint32_t _eventMask{0}; // Uint32: 0
uint32_t _layer{static_cast<uint32_t>(Layers::LayerList::DEFAULT)}; // Uint32: 1
uint32_t _transformFlags{0}; // Uint32: 2
uint32_t _transformFlags{static_cast<uint32_t>(TransformBit::TRS)}; // Uint32: 2
Copy link
Contributor

Choose a reason for hiding this comment

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

this._transformFlags |= TransformBit.TRS; was set in _onBatchCreated.

If we modify the initialized value to TRS, will it mark it as dirty twice and call updateWorldTransform twice?

For instance,

const scene = deserialize(archive) as Scene;
const node = scene.getChildByPath('Node')!;
const worldScale = node.worldScale;  // call updateWorldTransform internally, and reset child._transformFlags = TransformBit.NONE;

// Use worldScale 

director.runScene(scene); // call scene._load -> scene._onBatchCreated -> node._onBatchCreated -> this._transformFlags |= TransformBit.TRS; 
//  this._transformFlags is set to TransformBit.TRS again, so updateWorldTransform will be invoked twice, but in fact, the world transform was not changed at all.

Copy link
Contributor Author

@shrinktofit shrinktofit Oct 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'd recommend deleting transform flags modification in Node._onBatchCreated.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's ok for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

abb7ea2

index_t _siblingIndex{0}; // Int32: 0
uint8_t _activeInHierarchy{0}; // Uint8: 0
uint8_t _active{1}; // Uint8: 1
Expand Down
26 changes: 26 additions & 0 deletions tests/scene-graph/scene.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import { deserialize } from "../../cocos/serialization";
import '../utils/matchers/value-type-asymmetric-matchers';
import { v3 } from "../../cocos/core";
import { Scene } from "../../cocos/scene-graph";

test(`Bugfix cocos/cocos-engine#16352: deserialized scene should have node transforms invalidated`, () => {
const archive = [{
__type__: 'cc.Scene',
_children: [{
__type__: 'cc.Node',
_name: 'Node',
_lscale: {
__type__: 'cc.Vec3',
x: 2,
y: 1,
z: 1,
},
}],
}];

const scene = deserialize(archive) as Scene;

const node = scene.getChildByPath('Node')!;
expect(node).not.toBeNull();
expect(node.worldScale).toBeCloseToVec3(v3(2, 1, 1));
});
Loading