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

Improve the performance of certain GraphNode methods and avoid recalculating the AABB multiple times during rendering. #7245

Closed
wants to merge 13 commits into from
242 changes: 166 additions & 76 deletions src/scene/graph-node.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,31 +43,6 @@ function createTest(attr, value) {
};
}

/**
* Helper function to recurse findOne without calling createTest constantly.
*
* @param {GraphNode} node - Current node.
* @param {FindNodeCallback} test - Test function.
* @returns {GraphNode|null} A graph node that matches the search criteria. Returns null if no
* node is found.
*/
function findNode(node, test) {
if (test(node)) {
return node;
}

const children = node._children;
const len = children.length;
for (let i = 0; i < len; ++i) {
const result = findNode(children[i], test);
if (result) {
return result;
}
}

return null;
}

/**
* Callback used by {@link GraphNode#find} and {@link GraphNode#findOne} to search through a graph
* node and all of its descendants.
Expand Down Expand Up @@ -98,6 +73,12 @@ function findNode(node, test) {
* a powerful set of features that are leveraged by the `Entity` class.
*/
class GraphNode extends EventHandler {
/**
* @type {GraphNode[]}
* @ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be @private instead?

*/
static _tmpGraphNodeStack = [];

/**
* The non-unique name of a graph node. Defaults to 'Untitled'.
*
Expand Down Expand Up @@ -448,12 +429,30 @@ class GraphNode extends EventHandler {
* @protected
*/
_notifyHierarchyStateChanged(node, enabled) {
node._onHierarchyStateChanged(enabled);

const c = node._children;
for (let i = 0, len = c.length; i < len; i++) {
if (c[i]._enabled) {
this._notifyHierarchyStateChanged(c[i], enabled);
/**
* NOTE: GraphNode._tmpGraphNodeStack is not used here because _notifyHierarchyStateChanged can be called
* while other _notifyHierarchyStateChanged is still running because of _onHierarchyStateChanged
*
* @type {GraphNode[]}
*/
const stack = [node];

while (stack.length) {
const current = stack.pop();
current._onHierarchyStateChanged(enabled);

const c = current._children;

for (let i = 0; i < c.length; i++) {
const child = c[i];

if (enabled) {
if (child._enabled) {
stack.push(child);
}
} else {
stack.push(child);
}
}
}
}
Expand Down Expand Up @@ -594,11 +593,22 @@ class GraphNode extends EventHandler {
const results = [];
const test = createTest(attr, value);

this.forEach((node) => {
const stack = GraphNode._tmpGraphNodeStack;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since forEach is now optimized to execute iteratively, why duplicate the iterative loop again here?

stack.push(this);

while (stack.length) {
const node = stack.pop();

if (test(node)) {
results.push(node);
}
});

const children = node._children;

for (let i = 0; i < children.length; i++) {
stack.push(children[i]);
}
}

return results;
}
Expand Down Expand Up @@ -629,7 +639,23 @@ class GraphNode extends EventHandler {
*/
findOne(attr, value) {
const test = createTest(attr, value);
return findNode(this, test);

const stack = GraphNode._tmpGraphNodeStack;
stack.push(this);

while (stack.length) {
const current = stack.pop();
if (test(current)) {
return current;
}

const children = current._children;
for (let i = 0; i < children.length; i++) {
stack.push(children[i]);
}
}

return null;
}

/**
Expand All @@ -653,21 +679,27 @@ class GraphNode extends EventHandler {
* // Return all assets that tagged by (`carnivore` AND `mammal`) OR (`carnivore` AND `reptile`)
* const meatEatingMammalsAndReptiles = node.findByTag(["carnivore", "mammal"], ["carnivore", "reptile"]);
*/
findByTag() {
const query = arguments;
findByTag(...query) {
const results = [];
const stack = GraphNode._tmpGraphNodeStack;

const children = this._children;
for (let i = 0; i < children.length; i++) {
stack.push(children[i]);
}

const queryNode = (node, checkNode) => {
if (checkNode && node.tags.has(...query)) {
while (stack.length) {
const node = stack.pop();

if (node.tags.has(...query)) {
results.push(node);
}

for (let i = 0; i < node._children.length; i++) {
queryNode(node._children[i], true);
const children = node._children;
for (let i = 0; i < children.length; i++) {
stack.push(children[i]);
}
};

queryNode(this, false);
}

return results;
}
Expand Down Expand Up @@ -715,23 +747,36 @@ class GraphNode extends EventHandler {

/**
* Executes a provided function once on this graph node and all of its descendants.
* The method executes the provided function for each node in the graph in a breadth-first manner.
*
* @param {ForEachNodeCallback} callback - The function to execute on the graph node and each
* descendant.
* @param {object} [thisArg] - Optional value to use as this when executing callback function.
* @param {object} [thisArg] - Optional value to use as this when executing the callback function.
* @example
* // Log the path and name of each node in descendant tree starting with "parent"
* // Log the path and name of each node in the descendant tree starting with "parent"
* parent.forEach(function (node) {
* console.log(node.path + "/" + node.name);
* });
*/
forEach(callback, thisArg) {
callback.call(thisArg, this);
/**
* NOTE: GraphNode._tmpGraphNodeStack is not used here because forEach can be called within the callback
*
* @type {GraphNode[]}
*/
const queue = [this];

const children = this._children;
const len = children.length;
for (let i = 0; i < len; ++i) {
children[i].forEach(callback, thisArg);
while (queue.length > 0) {
// shift() is used for breadth-first approach, use pop() for depth-first
const current = queue.shift();

callback.call(thisArg, current);

const children = current._children;

for (let i = 0; i < children.length; i++) {
queue.push(children[i]);
}
}
}

Expand Down Expand Up @@ -1115,18 +1160,28 @@ class GraphNode extends EventHandler {

/** @private */
_dirtifyWorldInternal() {
if (!this._dirtyWorld) {
this._frozen = false;
this._dirtyWorld = true;
for (let i = 0; i < this._children.length; i++) {
if (!this._children[i]._dirtyWorld) {
this._children[i]._dirtifyWorldInternal();
const stack = GraphNode._tmpGraphNodeStack;
stack.push(this);

while (stack.length > 0) {
const node = stack.pop();

if (!node._dirtyWorld) {
node._frozen = false;
node._dirtyWorld = true;
}

node._dirtyNormal = true;
node._worldScaleSign = 0;
node._aabbVer++;

const children = node._children;
for (let i = 0; i < children.length; i++) {
if (!children[i]._dirtyWorld) {
stack.push(children[i]);
}
}
}
this._dirtyNormal = true;
this._worldScaleSign = 0; // world matrix is dirty, mark this flag dirty too
this._aabbVer++;
}

/**
Expand Down Expand Up @@ -1345,8 +1400,28 @@ class GraphNode extends EventHandler {
*/
_fireOnHierarchy(name, nameHierarchy, parent) {
this.fire(name, parent);
for (let i = 0; i < this._children.length; i++) {
this._children[i]._fireOnHierarchy(nameHierarchy, nameHierarchy, parent);

/**
* NOTE: GraphNode._tmpGraphNodeStack is not used here because _fireOnHierarchy can be called
* while other _fireOnHierarchy is still running
*
* @type {GraphNode[]}
*/
const stack = [this];

const child = this._children;

for (let i = 0; i < child.length; i++) {
stack.push(child[i]);
}

while (stack.length) {
const curr = stack.pop();
curr.fire(nameHierarchy, parent);

for (let j = 0; j < curr._children.length; j++) {
stack.push(curr._children[j]);
}
}
}

Expand Down Expand Up @@ -1390,15 +1465,21 @@ class GraphNode extends EventHandler {
}

/**
* Recurse the hierarchy and update the graph depth at each node.
* Iterates through the hierarchy and update the graph depth at each node.
*
* @private
*/
_updateGraphDepth() {
this._graphDepth = this._parent ? this._parent._graphDepth + 1 : 0;
const stack = GraphNode._tmpGraphNodeStack;
stack.push(this);

for (let i = 0, len = this._children.length; i < len; i++) {
this._children[i]._updateGraphDepth();
while (stack.length) {
const n = stack.pop();
n._graphDepth = n._parent ? n._parent._graphDepth + 1 : 0;

for (let i = 0; i < n._children.length; i++) {
stack.push(n._children[i]);
}
}
}

Expand Down Expand Up @@ -1501,22 +1582,31 @@ class GraphNode extends EventHandler {
* @ignore
*/
syncHierarchy() {
if (!this._enabled) {
if (!this._enabled || this._frozen) {
return;
}

if (this._frozen) {
return;
}
this._frozen = true;
const stack = GraphNode._tmpGraphNodeStack;
willeastcott marked this conversation as resolved.
Show resolved Hide resolved
stack.push(this);

if (this._dirtyLocal || this._dirtyWorld) {
this._sync();
}
while (stack.length) {
const node = stack.pop();

const children = this._children;
for (let i = 0, len = children.length; i < len; i++) {
children[i].syncHierarchy();
if (!node._enabled || node._frozen) {
continue;
}

node._frozen = true;

if (node._dirtyLocal || node._dirtyWorld) {
node._sync();
}

const children = node._children;

for (let i = children.length - 1; i >= 0; i--) {
stack.push(children[i]);
}
}
}

Expand Down
13 changes: 10 additions & 3 deletions src/scene/mesh-instance.js
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,8 @@
*/
pick = true;

_aabbUpdateIndex = NaN;

/**
* The stencil parameters for front faces or null if no stencil is enabled.
*
Expand Down Expand Up @@ -945,7 +947,7 @@
* @returns {boolean} - True if the mesh instance is visible by the camera, false otherwise.
* @ignore
*/
_isVisible(camera) {
_isVisible(camera, aabbUpdateIndex) {

if (this.visible) {

Expand All @@ -954,8 +956,13 @@
return this.isVisibleFunc(camera);
}

_tempSphere.center = this.aabb.center; // this line evaluates aabb
_tempSphere.radius = this._aabb.halfExtents.length();
const aabb = this._aabbUpdateIndex === aabbUpdateIndex
? this._aabb

Check failure on line 960 in src/scene/mesh-instance.js

View workflow job for this annotation

GitHub Actions / Lint

'?' should be placed at the end of the line
: this.aabb; // this line evaluates aabb

Check failure on line 961 in src/scene/mesh-instance.js

View workflow job for this annotation

GitHub Actions / Lint

':' should be placed at the end of the line
this._aabbUpdateIndex = aabbUpdateIndex;

_tempSphere.center = aabb.center;
_tempSphere.radius = aabb.halfExtents.length();

return camera.frustum.containsSphere(_tempSphere) > 0;
}
Expand Down
Loading
Loading