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

Jump to bottom

Fix: destroy and removeChild performance #10505

Merged
merged 11 commits into from
May 14, 2024
Merged

Fix: destroy and removeChild performance #10505

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
4e53f30
- optimise remove child
GoodBoyDigital May 1, 2024
c5683d4
Merge branch 'dev' into chore/optimise-remove
GoodBoyDigital May 1, 2024
1995851
remove dead code
GoodBoyDigital May 1, 2024
b4f363b
optimise destroy remove children
GoodBoyDigital May 1, 2024
05c9223
optimise the rendergroup
GoodBoyDigital May 2, 2024
4f868db
fix test import
GoodBoyDigital May 2, 2024
a09c528
finak clean up
GoodBoyDigital May 2, 2024
8e7c013
fix parent issue
GoodBoyDigital May 3, 2024
95f74e5
final tweaks
GoodBoyDigital May 3, 2024
6a1f23f
lint
GoodBoyDigital May 3, 2024
2f10ef4
address feedback
GoodBoyDigital May 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
106 changes: 41 additions & 65 deletions src/scene/container/Container.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,13 +362,15 @@ export class Container<C extends ContainerChild = ContainerChild> extends EventE
/** @private */
public _updateFlags = 0b1111;

// is this container the root of a renderGroup?
// TODO implement this in a few more places
/** @private */
public isRenderGroupRoot = false;
// the render group this container belongs to OR owns
// the render group this container owns
/** @private */
public renderGroup: RenderGroup = null;
// the render group this container belongs to
/** @private */
public parentRenderGroup: RenderGroup = null;
// the index of the container in the render group
/** @private */
public parentRenderGroupIndex: number = 0;

// set to true if the container has changed. It is reset once the changes have been applied
// by the transform system
Expand Down Expand Up @@ -626,9 +628,9 @@ export class Container<C extends ContainerChild = ContainerChild> extends EventE
this.children.splice(this.children.indexOf(child), 1);
this.children.push(child);

if (this.renderGroup && !this.isRenderGroupRoot)
if (this.parentRenderGroup)
{
this.renderGroup.structureDidChange = true;
this.parentRenderGroup.structureDidChange = true;
}

return child;
Expand All @@ -652,9 +654,11 @@ export class Container<C extends ContainerChild = ContainerChild> extends EventE
// TODO - OPtimise this? could check what the parent has set?
child._updateFlags = 0b1111;

if (this.renderGroup)
const renderGroup = this.renderGroup || this.parentRenderGroup;

if (renderGroup)
{
this.renderGroup.addChild(child);
renderGroup.addChild(child);
}

this.emit('childAdded', child, this, this.children.length - 1);
Expand Down Expand Up @@ -703,6 +707,10 @@ export class Container<C extends ContainerChild = ContainerChild> extends EventE
{
this.renderGroup.removeChild(child);
}
else if (this.parentRenderGroup)
{
this.parentRenderGroup.removeChild(child);
}

child.parent = null;
this.emit('childRemoved', child, this, index);
Expand Down Expand Up @@ -730,25 +738,15 @@ export class Container<C extends ContainerChild = ContainerChild> extends EventE
if (this.didChange) return;
this.didChange = true;

if (this.isRenderGroupRoot)
if (this.parentRenderGroup)
{
const renderGroupParent = this.renderGroup.renderGroupParent;
// lets update its parent..

if (renderGroupParent)
{
renderGroupParent.onChildUpdate(this);
}
}
else if (this.renderGroup)
{
this.renderGroup.onChildUpdate(this);
this.parentRenderGroup.onChildUpdate(this);
}
}

set isRenderGroup(value: boolean)
Copy link

Choose a reason for hiding this comment

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

category Error Handling

The 'isRenderGroup' property has been refactored, and the setter now throws an error if trying to set it to false. This could be a breaking change for any code that relies on toggling this property. Consider providing a migration path or additional documentation to handle this change.

{
if (this.isRenderGroupRoot && value === false)
if (this.renderGroup && value === false)
{
throw new Error('[Pixi] cannot undo a render group just yet');
}
Expand All @@ -765,18 +763,16 @@ export class Container<C extends ContainerChild = ContainerChild> extends EventE
*/
get isRenderGroup(): boolean
{
return this.isRenderGroupRoot;
return !!this.renderGroup;
}

/** This enables the container to be rendered as a render group. */
public enableRenderGroup()
{
// does it OWN the render group..
if (this.renderGroup && this.renderGroup.root === this) return;

this.isRenderGroupRoot = true;
if (this.renderGroup) return;

const parentRenderGroup = this.renderGroup;
const parentRenderGroup = this.parentRenderGroup;

if (parentRenderGroup)
{
Expand All @@ -785,27 +781,9 @@ export class Container<C extends ContainerChild = ContainerChild> extends EventE

this.renderGroup = new RenderGroup(this);

// find children render groups and move them out..
if (parentRenderGroup)
{
for (let i = 0; i < parentRenderGroup.renderGroupChildren.length; i++)
{
const childRenderGroup = parentRenderGroup.renderGroupChildren[i];
let parent = childRenderGroup.root;

while (parent)
{
if (parent === this)
{
this.renderGroup.addRenderGroupChild(childRenderGroup);

break;
}
parent = parent.parent;
}
}

parentRenderGroup.addRenderGroupChild(this.renderGroup);
parentRenderGroup.addChild(this);
}

this._updateIsSimple();
Expand All @@ -818,7 +796,7 @@ export class Container<C extends ContainerChild = ContainerChild> extends EventE
/** @ignore */
public _updateIsSimple()
{
this.isSimple = !(this.isRenderGroupRoot) && (this.effects.length === 0);
this.isSimple = !(this.renderGroup) && (this.effects.length === 0);
}

/**
Expand All @@ -831,14 +809,11 @@ export class Container<C extends ContainerChild = ContainerChild> extends EventE

if (this.renderGroup)
{
if (this.isRenderGroupRoot)
{
this._worldTransform.copyFrom(this.renderGroup.worldTransform);
}
else
{
this._worldTransform.appendFrom(this.relativeGroupTransform, this.renderGroup.worldTransform);
}
this._worldTransform.copyFrom(this.renderGroup.worldTransform);
}
else if (this.parentRenderGroup)
{
this._worldTransform.appendFrom(this.relativeGroupTransform, this.parentRenderGroup.worldTransform);
}

return this._worldTransform;
Expand Down Expand Up @@ -1224,9 +1199,9 @@ export class Container<C extends ContainerChild = ContainerChild> extends EventE
set blendMode(value: BLEND_MODES)
{
if (this.localBlendMode === value) return;
if (this.renderGroup && !this.isRenderGroupRoot)
if (this.parentRenderGroup)
{
this.renderGroup.structureDidChange = true;
this.parentRenderGroup.structureDidChange = true;
}

this._updateFlags |= UPDATE_BLEND;
Expand Down Expand Up @@ -1259,9 +1234,9 @@ export class Container<C extends ContainerChild = ContainerChild> extends EventE

if ((this.localDisplayStatus & 0b010) >> 1 === valueNumber) return;

if (this.renderGroup && !this.isRenderGroupRoot)
if (this.parentRenderGroup)
{
this.renderGroup.structureDidChange = true;
this.parentRenderGroup.structureDidChange = true;
}

this._updateFlags |= UPDATE_VISIBLE;
Expand All @@ -1284,9 +1259,9 @@ export class Container<C extends ContainerChild = ContainerChild> extends EventE

if ((this.localDisplayStatus & 0b100) >> 2 === valueNumber) return;

if (this.renderGroup && !this.isRenderGroupRoot)
if (this.parentRenderGroup)
{
this.renderGroup.structureDidChange = true;
this.parentRenderGroup.structureDidChange = true;
}

this._updateFlags |= UPDATE_VISIBLE;
Expand All @@ -1310,9 +1285,9 @@ export class Container<C extends ContainerChild = ContainerChild> extends EventE
this._updateFlags |= UPDATE_VISIBLE;
this.localDisplayStatus ^= 0b001;

if (this.renderGroup && !this.isRenderGroupRoot)
if (this.parentRenderGroup)
{
this.renderGroup.structureDidChange = true;
this.parentRenderGroup.structureDidChange = true;
}

this._onUpdate();
Expand Down Expand Up @@ -1343,6 +1318,9 @@ export class Container<C extends ContainerChild = ContainerChild> extends EventE
if (this.destroyed) return;
this.destroyed = true;

// remove children is faster than removeChild..
const oldChildren = this.removeChildren(0, this.children.length);

this.removeFromParent();
this.parent = null;
this._mask = null;
Expand All @@ -1359,8 +1337,6 @@ export class Container<C extends ContainerChild = ContainerChild> extends EventE

const destroyChildren = typeof options === 'boolean' ? options : options?.children;

const oldChildren = this.removeChildren(0, this.children.length);

if (destroyChildren)
{
for (let i = 0; i < oldChildren.length; ++i)
Expand Down