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.

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

Optimize RenderComponent destroy #7194

Merged
merged 8 commits into from
Dec 19, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
85 changes: 48 additions & 37 deletions src/framework/components/render/component.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@ import { AssetReference } from '../../asset/asset-reference.js';

import { Component } from '../component.js';

import { EntityReference } from '../../utils/entity-reference.js';

/**
* The RenderComponent enables an {@link Entity} to render 3D meshes. The {@link RenderComponent#type}
* property can be set to one of several predefined shape types (such as `box`, `sphere`, `cone`
Expand Down Expand Up @@ -54,9 +52,6 @@ import { EntityReference } from '../../utils/entity-reference.js';
* - [Primitive Shapes](https://playcanvas.github.io/#/graphics/shapes)
* - [Loading Render Assets](https://playcanvas.github.io/#/graphics/render-asset)
*
* @property {import('../../entity.js').Entity} rootBone A reference to the entity to be used as
* the root bone for any skinned meshes that are rendered by this component.
*
* @category Graphics
*/
class RenderComponent extends Component {
Expand Down Expand Up @@ -136,10 +131,13 @@ class RenderComponent extends Component {
_material;

/**
* @type {EntityReference}
* A reference to the entity to be used as the root bone for any skinned meshes that
* are rendered by this component.
*
* @type {import('../../entity.js').Entity|null}
willeastcott marked this conversation as resolved.
Show resolved Hide resolved
Maksims marked this conversation as resolved.
Show resolved Hide resolved
* @private
*/
_rootBone;
_rootBone = null;

/**
* @type {import('../../../core/event-handle.js').EventHandle|null}
Expand Down Expand Up @@ -177,8 +175,6 @@ class RenderComponent extends Component {
super(system, entity);

// the entity that represents the root bone if this render component has skinned meshes
this._rootBone = new EntityReference(this, 'rootBone');
this._rootBone.on('set:entity', this._onSetRootBone, this);

// render asset reference
this._assetReference = new AssetReference(
Expand Down Expand Up @@ -275,7 +271,6 @@ class RenderComponent extends Component {
* @type {string}
*/
set type(value) {

if (this._type !== value) {
this._area = null;
this._type = value;
Expand Down Expand Up @@ -312,7 +307,6 @@ class RenderComponent extends Component {
* @type {MeshInstance[]}
*/
set meshInstances(value) {

Debug.assert(Array.isArray(value), 'MeshInstances set to a Render component must be an array.');
this.destroyMeshInstances();

Expand Down Expand Up @@ -703,27 +697,46 @@ class RenderComponent extends Component {
}

/**
* @param {import('../../entity.js').Entity} entity - The entity set as the root bone.
* @private
* Sets the root bone entity (or entity guid) for the render component.
*
* @type {import('../../entity.js').Entity|string|null}
mvaligursky marked this conversation as resolved.
Show resolved Hide resolved
*/
_onSetRootBone(entity) {
if (entity) {
this._onRootBoneChanged();
set rootBone(value) {
if (this._rootBone !== value) {
const isString = typeof value === 'string';
if (this._rootBone && isString && this._rootBone.getGuid() === value) {
return;
}

if (this._rootBone) {
this._clearSkinInstances();
}

if (value instanceof GraphNode) {
this._rootBone = value;
} else if (isString) {
this._rootBone = this.system.app.getEntityFromIndex(value) || null;
Maksims marked this conversation as resolved.
Show resolved Hide resolved
} else {
this._rootBone = null;
}

if (this._rootBone) {
this._cloneSkinInstances();
}
}
}

/** @private */
_onRootBoneChanged() {
// remove existing skin instances and create new ones, connected to new root bone
this._clearSkinInstances();
if (this.enabled && this.entity.enabled) {
this._cloneSkinInstances();
}
/**
* Gets the root bone entity for the render component.
*
* @type {import('../../entity.js').Entity|null}
*/
get rootBone() {
return this._rootBone;
}

/** @private */
destroyMeshInstances() {

const meshInstances = this._meshInstances;
if (meshInstances) {
this.removeFromLayers();
Expand Down Expand Up @@ -814,9 +827,9 @@ class RenderComponent extends Component {
const scene = app.scene;
const layers = scene.layers;

this._rootBone.onParentComponentEnable();

this._cloneSkinInstances();
if (this._rootBone) {
this._cloneSkinInstances();
}

this._evtLayersChanged = scene.on('set:layers', this.onLayersChanged, this);

Expand Down Expand Up @@ -852,6 +865,10 @@ class RenderComponent extends Component {
this._evtLayersChanged?.off();
this._evtLayersChanged = null;

if (this._rootBone) {
this._clearSkinInstances();
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally this would not be here, but I see how it makes it easier / more consistent, and the impact is low.

}

if (layers) {
this._evtLayerAdded?.off();
this._evtLayerAdded = null;
Expand Down Expand Up @@ -904,7 +921,6 @@ class RenderComponent extends Component {
}

_onRenderAssetLoad() {

// remove existing instances
this.destroyMeshInstances();

Expand All @@ -923,7 +939,6 @@ class RenderComponent extends Component {
}

_clearSkinInstances() {

for (let i = 0; i < this._meshInstances.length; i++) {
const meshInstance = this._meshInstances[i];

Expand All @@ -934,23 +949,20 @@ class RenderComponent extends Component {
}

_cloneSkinInstances() {

if (this._meshInstances.length && this._rootBone.entity instanceof GraphNode) {

if (this._meshInstances.length && this._rootBone instanceof GraphNode) {
for (let i = 0; i < this._meshInstances.length; i++) {
const meshInstance = this._meshInstances[i];
const mesh = meshInstance.mesh;

// if skinned but does not have instance created yet
if (mesh.skin && !meshInstance.skinInstance) {
meshInstance.skinInstance = SkinInstanceCache.createCachedSkinInstance(mesh.skin, this._rootBone.entity, this.entity);
meshInstance.skinInstance = SkinInstanceCache.createCachedSkinInstance(mesh.skin, this._rootBone, this.entity);
}
}
}
}

_cloneMeshes(meshes) {

Maksims marked this conversation as resolved.
Show resolved Hide resolved
if (meshes && meshes.length) {

// cloned mesh instances
Expand Down Expand Up @@ -1031,10 +1043,9 @@ class RenderComponent extends Component {
}

resolveDuplicatedEntityReferenceProperties(oldRender, duplicatedIdsMap) {
if (oldRender.rootBone && duplicatedIdsMap[oldRender.rootBone]) {
this.rootBone = duplicatedIdsMap[oldRender.rootBone];
if (oldRender.rootBone) {
this.rootBone = duplicatedIdsMap[oldRender.rootBone.getGuid()];
}
this._clearSkinInstances();
}
}

Expand Down
1 change: 0 additions & 1 deletion src/framework/components/render/data.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
class RenderComponentData {
constructor() {
this.enabled = true;
this.rootBone = null;
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/framework/components/render/system.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import { RenderComponent } from './component.js';
import { RenderComponentData } from './data.js';

const _schema = [
{ name: 'rootBone', type: 'entity' },
'enabled'
];

Expand All @@ -30,7 +29,8 @@ const _properties = [
'type',
'layers',
'isStatic',
'batchGroupId'
'batchGroupId',
'rootBone'
];

/**
Expand Down
Loading