New hierarchy find functions

[NewboO]

This PR add various functions to find entities, components and script instances either through parents or children. It completes the already existing functions to find through children and can be used the same way.

List of new functions:

• GraphNode.findDescendants
• GraphNode.findDescendant
• GraphNode.forEachDescendant
• GraphNode.findAncestors
• GraphNode.findAncestor
• GraphNode.forEachAncestor
• Entity.findScript
• Entity.findScripts
• Entity.findComponentInDescendants
• Entity.findComponentsInDescendants
• Entity.findScriptInDescendants
• Entity.findScriptsInDescendants
• Entity.findComponentInAncestors
• Entity.findComponentsInAncestors
• Entity.findScriptInAncestors
• Entity.findScriptsInAncestors

It also simplifies some already existing functions in GraphNode.

I hope I documentated them correctly. I also added unit tests inspired by the existing ones.

I confirm I have read the contributing guidelines and signed the Contributor License Agreement.

[yaustar]

Personal take as an end user. I think Entity.findScript and Entity.findScripts could be very useful but the * in parent don't seem as useful given that you can get the parent and then call find * functions on that?

[NewboO]

@yaustar What if you don't know if the script or component you want is the direct parent or higher in the hierarchy? What if the parent doesn't have the script or component you want but a child do?

For instance, as I love having generic scripts, I have some to tint, scale, etc. elements based on a parent button state (the button component can only tint OR change sprite of a single element) thus my scripts can simply find the closest button in its ancestors (which can be itself) and I don't need to write specific scripts each time a button doesn't have the same hierarchy.

[yaustar]

Does Entity.findComponentsInParent search all the parents? Or just the immediate parent? The name sounds like it only checks the immediate parent.

[NewboO]

All the parents, same as Unity though "*InParents" would be more accurate indeed.

[kungfooman]

Suggestion for GraphNode#findInParent:

    /**
     * Search the graph node and all of its ascendants for the nodes that satisfy some search
     * criteria.
     *
     * @param {FindNodeCallback|string} attr - This can either be a function or a string. If it's a
     * function, it is executed for each ascendant node to test if node satisfies the search
     * logic. Returning true from the function will include the node into the results. If it's a
     * string then it represents the name of a field or a method of the node. If this is the name
     * of a field then the value passed as the second argument will be checked for equality. If
     * this is the name of a function then the return value of the function will be checked for
     * equality against the valued passed as the second argument to this function.
     * @param {object} [value] - If the first argument (attr) is a property name then this value
     * will be checked against the value of the property.
     * @param {this[]} [results] - Array where the results are appended to.
     * @returns {this[]} The array of graph nodes that match the search criteria.
     * @example
     * // Finds all nodes that have a group element component
     * var groups = element.findInParent(function (node) {
     *     return node.element && node.element.type === pc.ELEMENTTYPE_GROUP;
     * });
     * @example
     * // Finds all nodes that have the name property set to 'Test'
     * var entities = entity.findInParent('name', 'Test');
     */
    findInParent(attr, value, results = []) {
        if (attr instanceof Function) {
            if (attr(this))
                results.push(this);
        } else if (this[attr]) {
            let testValue;

            if (this[attr] instanceof Function) {
                testValue = this[attr]();
            } else {
                testValue = this[attr];
            }
            if (testValue === value)
                results.push(this);
        }

        if (this._parent) {
            this._parent.findInParent(attr, value, results);
        }

        return results;
    }

Motivation is that right now empty arrays are created and merged all the time, which is basically waste. You need exactly one results array, without any form of merging.

That turns: results.push(...this._parent.findInParent(attr, value));
Into: this._parent.findInParent(attr, value, results);

The this[] type is used for proper OOP and reasoning is rather simple with an example:

class TestNode extends GraphNode {
    // Nothing yet.
};
const testNode = new TestNode("hai");
const ret = testNode.findInParent("name", "hai");

IMO ret should be a TestNode[] and not a GraphNode[] here. But I also see that its impossible to judge the parent types aswell, a bit tricky/unsolvable?

[NewboO]

I included the suggestion to avoid empty arrays, and also applied it to find function.

However, using a this[] type feels really off. In your example, it would indeed return a TestNode[] but nothing prevents you from having a hierarchy mixing different subtypes of GraphNode, most of them being Entity in real use cases, so it would be wrong to say it returns a TestNode[] in this other example:

class TestNode extends GraphNode {
    // Nothing yet.
};
const rootNode = new GraphNode("hai");
const testNode = new TestNode("hai");
rootNode.addChild(testNode);
const ret = testNode.findInParent("name", "hai");

Moreover, that would be way beyond the initial objective of this PR because every properties and functions to get, add and remove nodes are documented as returning or expecting GraphNode thus they would all need to be changed accordingly.

[NewboO]

Function names are now pluralized as *InParents for clarity and are also non-recursive.
The results parameter has been removed again by using another way to have a single const array where matching are pushed when necessary.
I also added findByNameInParents to have the findByName equivalent.

By the way, about whether current node can be returned or not, I noticed that findByTag and findByPath are excluding it while find, findOne and findByName are including it. Shouldn't all those functions behave the same way? Though changing any of those might break some projects...

[kungfooman]

Your GraphNode#find is duplicating quite some lines of code now, but using a closure is of course a good idea to not add results = [] as default value argument.

You can just reuse GraphNode#forEach:

    find(attr, value) {
        const results = [];
        if (attr instanceof Function) {
            this.forEach((node) => {
                if (attr(node))
                    results.push(node);
            });
        } else {
            this.forEach((node) => {
                let testValue;

                if (node[attr] instanceof Function) {
                    testValue = node[attr]();
                } else {
                    testValue = node[attr];
                }
                if (testValue === value)
                    results.push(node);
            });
        }
        return results;
    }

I am also contemplating more helper functions for code like this:

        while (current) {
            if (current.name === name)
                return current;

            current = current._parent;
        }

GraphNode#forEachParent (inspired by GraphNode#forEach)
GraphNode#findOneParent (inspired by GraphNode#findOne)

Then there is also more code duplication regarding:

            // ...
            if (this[attr] instanceof Function) {
                testValue = this[attr]();
            } else {
                testValue = this[attr];
            }

Usually its called resolve. But yea, I guess you didn't plan to refactor all of these other methods.

[NewboO]

I added forEachParent and used it in findParents the same way find is now using forEach.

I already did findOneParent but it was named findOneInParents, I changed its name for clarity as well as findInParents is now findParents and findByNameInParents is now findParentByName. I'm wondering about doing findParentsByTag but I can't think of relevant examples where it could be useful.

I'm not sure I understood correctly what you meant with the duplicated code to get the test value, but I added a const function at the beginning of the file to directly check if an attribute matches a value and all four functions are now using it. Is this what you had in mind?

[kungfooman]

I'm not sure I understood correctly what you meant with the duplicated code to get the test value, but I added a const function at the beginning of the file to directly check if an attribute matches a value and all four functions are now using it. Is this what you had in mind?

Thank you for the quick implementation, yes, that is what I had in mind and you can even go a step further:

/**
 * 
 * @param {Function | string} attr 
 * @param {*} value 
 * @returns {Function}
 */
function createTest(attr, value) {
    if (attr instanceof Function) {
        return attr;
    }
    return (node) => {
        let x = node[attr];
        if (x instanceof Function) {
            x = x();
        }
        return x === value;
    };
}

If you do not "hardcode" node, but return a function which takes said node, other code just "melts" away, because its unified into one test function:

For example, GraphNode#find you can rewrite into:

    find(attr, value) {
        const results = [];
        const test = createTest(attr, value);
        this.forEach((node) => {
            if (test(node))
                results.push(node);
        });
        return results;
    }

That boils this method from 37 lines down to these 9 lines and makes it more reusable aswell. You can simplify/shorten the other methods with the same technique.

[Maksims]

GraphNode != Array
There is no unified convention between standards/languages on such methods.
Also, including own node in search, with returning a single result - will lead to unwanted cases where in order to ignore own node - need to start from parent/each child. While not checking own node in filter/find methods - gives simpler choice to developer if need to check for self.

Again, GraphNode - is not an array. Its an individual node.
So node.filter - can be interpreted as checking that exact node against a filter.

Also, breaking projects - is not an option. Either changes are backwards compatible or they are additions in parallel. That is the rule been followed since first days of an engine open sourcing.

[NewboO]

Also, including own node in search, with returning a single result - will lead to unwanted cases where in order to ignore own node - need to start from parent/each child. While not checking own node in filter/find methods - gives simpler choice to developer if need to check for self.

Problem is find, findOne and findByName have been always including current node since their creation many years ago so how not to break anything? Though I agree it must be avoided as much as possible, breaking projects happened in the past so it's always an option.

However, as people never had the choice of ignoring current node, I'm now wondering about the actual usage of the different find functions and in how many cases people are relying on the fact these functions can return self node (or self component). Because maybe the behaviour can be changed without actually breaking many projects but just a few ones? For instance, most people using findByName are probably calling it on nodes which can't be returned because its name is never the searched one.

Edit:
Updating PR to change behaviour of find functions so they now all ignore the current node. It allows a way shorter version of findByTag which is now simply using find. Also made a shorter version of isDescendantOf using findAncestor.

All new functions to find among ascendants were renamed from *Parents to *Ancestors for consistency because GraphNode already had a isAncestorOf function.

[kungfooman]

Removing current node from GraphNode#find is going too far, as that behaviour is expected and relied upon.

But removing self from GraphNode#findAncestors makes sense to me (after all its called GraphNode#findAncestors and GraphNode#find isn't called GraphNode#findDescendants).

You also need to test your changes against non-trivial projects to find the special cases. SeeMore for example is doing this:

You can do so by e.g.:

    findByName(name) {
        if (this.name === name) {
            console.log("findByName", this, name);
            debugger;
        }
        return this.findOne('name', name);
    }

Again, GraphNode - is not an array. Its an individual node. So node.filter - can be interpreted as checking that exact node against a filter.

@Maksims Can you expand on that? Do you mean testing (attr, value) only on the direct children or only against the node itself?

To bring in even more keywords: I would also like GraphNode#some and GraphNode#every 😅

[NewboO]

So let's try another solution by adding a parameter to let the user choose to include self node in the search. It's true by default for GraphNode.forEach, GraphNode.find, GraphNode.findOne, GraphNode.findByName, Entity.findComponent[s] and Entity.findScript[s] and false for all the other so old functions keep their default behaviour.

However, I'm faking overloading so the second parameter of some functions can be skipped when irrelevant (or not used) and I'm not sure if it respects PlayCanvas standards.

[kungfooman]

However, I'm faking overloading so the second parameter of some functions can be skipped when irrelevant (or not used) and I'm not sure if it respects PlayCanvas standards.

I can't speak for others obviously so my personal objection is increased code complexity. I liked to simplify the methods, they are shorter and (at least for me) easier/clearer to read.

_getIncludeSelf and if (typeof query[query.length - 1] === 'boolean') { includeSelf = query.pop(); } etc. is doing the opposite now.

The initially reduced complexity is back and it is hard to understand/track and just harder than necessary.

You are basically trapping into flag argument code smell, when you just want e.g. GraphNode#findExcludeSelf:

• https://medium.com/@amlcurran/clean-code-the-curse-of-a-boolean-parameter-c237a830b7a3
• https://martinfowler.com/bliki/FlagArgument.html

And then the question is: how many people need that method or in which context do you require it?

[NewboO]

I'm puzzled, how is the following (which was modified just 8 months ago)

    findByTag() {
        const query = arguments;
        const results = [];

        const queryNode = (node, checkNode) => {
            if (checkNode && node.tags.has(...query)) {
                results.push(node);
            }

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

        queryNode(this, false);

        return results;
    }

less complex and easier to read than

    findByTag(...query) {
        let includeSelf = false;
        if (typeof query[query.length - 1] === 'boolean')
            includeSelf = query.pop();

        return this.find(node => node.tags.has(...query), includeSelf);
    }

You can also notice how the current version of findByTag is using flag argument, though in an anonymous function (when it actually wasn't before that change 8 months ago). And then we can also wonder why find and findOne can take either a function or an attribute name and a value, instead of having findByAttribute and findOneByAttribute for the latter? Most of the complexity comes from that weird overloading in the first place.

Anyway, I don't see how adding tons of functions for every single variant would help reduce complexity for the user. And that's worse if adding as you're suggesting functions like map, some and every for which one can ask the same btw: how many people need these methods or in which context are they required?

So now I'm thinking of simply closing this PR and just keep the new functions for myself. The real usefulness relies in the new Entity functions I've been using in all of my projects for quite some time but it seems more important to focus on GraphNode details which weren't even the point of the PR.

[yaustar]

Speaking from an end user: I think the Entity.findScript and Entity.findScripts are the most useful out of the list and I'm thinking about pulling that out in a separate PR, keep them as consistent as possible with the out find functions (which from this thread seem to be inconsistent as it is).

The find in parents/ancestors would need an engine team call about whether they should be core engine functionality. It's not something I've seen asked about on the forums but wonder how many teams/developers have written their own variant 🤔

[LeXXik]

Our team made a similar one to .findScript, as some members prefer to access the script instance of a child entity for one reason or another. We don't have "find in parents/ancestors", as we never had a need in such.

[kungfooman]

You can also notice how the current version of findByTag is using flag argument, though in an anonymous function (when it actually wasn't before that change 8 months ago).

Good point, a helper function could be:

    findExcludeSelf(attr, value) {
        const results = [];
        const test = _createTest(attr, value);
        this._children.forEach(child => {
            child.forEach((node) => {
                if (test(node))
                    results.push(node);
            });
        });
        return results;
    }

And then it just becomes:

    findByTag(...query) {
        return this.findExcludeSelf(node => node.tags.has(...query));
    }

1. No flag arguments
2. Reusable
3. Not pushing checkNode/includeSelf into every stack frame and removing if (checkNode ...)
4. Keeps method signatures as they are

And then we can also wonder why find and findOne can take either a function or an attribute name and a value, instead of having findByAttribute and findOneByAttribute for the latter? Most of the complexity comes from that weird overloading in the first place.

I agree, so why create even more overloading?

Anyway, I don't see how adding tons of functions for every single variant would help reduce complexity for the user. And that's worse if adding as you're suggesting functions like map, some and every for which one can ask the same btw: how many people need these methods or in which context are they required?

Technically you already used GraphNode#some without calling it so:

This code:

    isDescendantOf(node) {
        return !!this.findAncestor(parent => parent === node);
    }

Is semantically this:

    isDescendantOf(node) {
        return this.someAncestor(ancestor => ancestor === node);
    }

    someAncestor(attr, value) {
        const test = _createTest(attr, value);
        let parent = this._parent;
        while (parent) {
            if (test(parent))
                return true;
            parent = parent._parent;
        }
        return false;
    }

So now I'm thinking of simply closing this PR and just keep the new functions for myself. The real usefulness relies in the new Entity functions I've been using in all of my projects for quite some time but it seems more important to focus on GraphNode details which weren't even the point of the PR.

Right, I definitely see the point for your includeSelf code and maybe that should be considered in a follow-up PR while this PR refactors and adds new methods and keeping the function signatures as they are (once the signatures are changed, its hard to go back).

We don't have "find in parents/ancestors", as we never had a need in such.

@LeXXik Just regexing a bit (via while \([a-zA-Z0-9 &&\.]+\)) and there is e.g. this code:

engine/src/framework/components/collision/system.js

Lines 102 to 113 in b17180c

	if (!component.rigidbody) {
	component._compoundParent = null;
	let parent = entity.parent;
	while (parent) {
	if (parent.collision && parent.collision.type === 'compound') {
	component._compoundParent = parent.collision;
	break;
	}
	parent = parent.parent;
	}
	}
	}

This looks like this could be expressed way shorter via GraphNode#findAncestor(untested):

                    const isTypeCompound = entity => entity.collision && entity.collision.type === 'compound';
                    component._compoundParent = entity.findAncestor(isTypeCompound);

[LeXXik]

@kungfooman ah, I meant in our team's custom code, not the engine 😅

[NewboO]

So, last try to make everyone happy. I considered find, findOne and forEach as legacy, so the only changes in them are now the refactoring.

There are now findDescendant[s], findAncestor[s], forEachDescendant and forEachAncestor for traversing tree except the current node, and most importantly all of the findComponent[s]* and findScript[s]* helpers in Entity deriving from them.

I kept findByName and findByTag refactorings but because one is including current node while the other don't, I'm not adding any descendant/ancestor variant. If anyone want to add other GraphNode functions, then it shoud be done in another PR to keep this one focused on the main GraphNode find functions and the new Entity component/script finding functions.

[kungfooman]

Thank you for the changes @NewboO !

I had no time to test it yet, just looking it over. Two notes:

• _createTest is missing type information through JSDoc, a nice example is e.g.:
  

  engine/src/framework/components/element/text-element.js

  Lines 50 to 71 in b17180c

  	/**
  	* Creates a new text mesh object from the supplied vertex information and topology.
  	*
  	* @param {object} device - The graphics device used to manage the mesh.
  	* @param {MeshInfo} [meshInfo] - An object that specifies optional inputs for the function as follows:
  	* @returns {Mesh} A new Mesh constructed from the supplied vertex and triangle data.
  	* @ignore
  	*/
  	function createTextMesh(device, meshInfo) {
  	const mesh = new Mesh(device);
  	mesh.setPositions(meshInfo.positions);
  	mesh.setNormals(meshInfo.normals);
  	mesh.setColors32(meshInfo.colors);
  	mesh.setUvs(0, meshInfo.uvs);
  	mesh.setIndices(meshInfo.indices);
  	mesh.setVertexStream(SEMANTIC_ATTR8, meshInfo.outlines, 3, undefined, TYPE_FLOAT32, false);
  	mesh.setVertexStream(SEMANTIC_ATTR9, meshInfo.shadows, 3, undefined, TYPE_FLOAT32, false);
  	mesh.update();
  	return mesh;
  	}

JSDoc that should work:

/**
 * Helper function that handles signature overloading to receive a test function.
 *
 * @param {Function | string} attr - Attribute or lambda.
 * @param {*} [value] - Optional value in case of `attr` being a `string`
 * @returns {Function} Test function that receives a GraphNode and returns a boolean.
 * @ignore
 */

I would also remove the underscore, it is private by default if you don't export it in in ES6 file scope, but I've seen it with and without underscore, so it doesn't really matter I guess.

• I don't see your Contributor License Agreement:

I confirm I have read the contributing guidelines and signed the Contributor License Agreement.

[mvaligursky]

We had a quick discussion, and agreed it's a great PR from many points of view, but it's so large, including the follow up discussion, that it's becoming hard to continue the work on this.

The suggestion is to break it down into small issues, addressing individual improvements / new APIs. The discussion should take place on those separate issues, and a relevant PR should be created when an agreement is reached there.

If there are some optimizations to existing done here for example, it'd be awesome to move those to a separate PR, as that would be valuable to have reviewed and merged.

[NewboO]

Finally took time to break down this PR.

So 2 new PR :
#5850 for the optimization
#5851 for the most consensual new feature

And a discussion #5852 for the main debate about ancestors/descendants.