Three.js: Request for Object3D method to assign (replace) children by array index (updated first post)

Created on 9 Mar 2016 · 39Comments · Source: mrdoob/three.js

Manipulation of Object3D's _children_ array is currenty achieved using the _add_ and _remove_ methods. They are basically set methods, in that the indices of the children are not relevant. I believe the indices currently serve no important purpose for traversal or anything else using standard methods. But _children_ is documented as an array, and it is possible to access children by indices.

What lacks is the possibility to cleanly assign values by index. That is what this issue and my pull request are all about. I do not request an introduction of a lot of high level stuff, only a safe means of assigning children by index. For various reasons, assignment cannot be made exactly analogous to _children[index]=newChild_.

I suggest making replaceAt(index, newChild) return the replaced child (aka oldChild), and making a (set-oriented) method replace(oldChild, newChild) that looks up oldChild, then calls replaceAt(indexOf(oldChild), newChild) (return value up for debate). Implementing replace in-place instead of as remove+add saves a splice, and if the application developer knows the index of the element to replace, then using that information with replaceAt will save a search too.

In addition to the (at least theoretical) performance advantages, using array indices is a natural, low-level way to replace unknown objects in known positions, e.g. replacing the current hat with a given one. As discussed below, this can be solved in other ways, such as having the same _name_ "hat" for all the hats, and looking up the current hat by searching for an object with the name "hat" (one search), removing it (another search and a splice), then adding the new hat (phew!). It can also be solved by creating a hat container object with only one child, avoiding the excessive searching operations, but building a less compact object hierarchy that (AFAICT) requires more computations and memory accesses later.

See my suggested implementation in PR #8414 . Some notes:

The implementation is largely based on code from _add_ and _remove_.
It is assumed that _children_ and _oldChild_ (if provided) have no errors, and the new methods are designed to not introduce any new errors, and to give warnings whenever there are indications of user errors.
Index shifting is avoided by disallowing movement within _children_. (Reordering _children_ is, as far as I know, entirely safe for the user to do directly on the array, but is not something the user of _replaceAt_ typically wants to happen, and the user of replace typically does not care, because _replace_ is not index-oriented (even though it internally uses _replaceAt_).)
If _newChild_ has a parent already, it will be "stolen" from the old parent, and a warning will be printed, because this is likely to be unintended.
_index_=_children.length_ is currently not allowed, because it is not a proper replacement. Use _add_ instead.

I can understand if you don't want to close doors for the future evolution of _children_ into another type of object. If, however, you plan to keep it as an array, having some simple array methods, namely replaceAt, will IMO be very useful. There has been some dicussion around other, somewhat higher-order, index-oriented methods, more or less analogous to _splice_ with various inputs. IMO replaceAt should be added regardless of what happens to the other candidates. Because it is low-level, efficient, conservative and useful.

Anyhow, I really think it should be mentioned in the docs that the children array is only for hackers... I spent a lot of hours scratching my head because nothing happened whenever I changed positions and rotations etc.

Enhancement

Source

EliasHasle

All 39 comments

Edit: See #8414 .

EliasHasle on 9 Mar 2016

You mean adding replaceChildAt( index, newChild ) to Object3D?

mrdoob on 9 Mar 2016

👍1

Yes, exactly! With no bugs or unintended consequences, of course! ;-) (I don't know all the implications of the events I have triggered in the above code.) BTW: Thanks for fast response, mrdoob. :-)

EliasHasle on 9 Mar 2016

Maybe this API is prettier:

parent.replace( oldChild, newChild );

mrdoob on 9 Mar 2016

Hm. I think both can be useful. But parent.replace(oldChild, newChild) will require a search operation, which is something I would like to avoid.

Here is an example with my suggested interface:

//User clicks "red hat", which runs changeHat("red").
function changeHat(style) {
newHat = hat[style]; //loads specified model from an object in RAM
head.replaceChildAt(1, newHat); //1 is the known index of the hat child.
}

Very simple example, but in my particular case I also use the input ("red") for updating GUI and other stuff. And say I have ten or a hundred or thousand different hats, and don't store anywhere else what hat is currently on.

With parent.replace( oldChild, newChild ); I would (absent major restructuring of the code) need to know the index anyway, but the index won't be utilized well, because the replace method must search for the child when the index isn't passed.

(Maybe there is a very clever way to achieve what I am trying to do, without resorting to complicated or comparatively slow procedures. I haven't really started using names and IDs for Object3Ds, neither have I tested the manipulation of visibility, so maybe I am missing out on something important.)

EliasHasle on 9 Mar 2016

Maybe this API is prettier:

parent.replace( oldChild, newChild );

Agreed.

WestLangley on 9 Mar 2016

parent.replace(old, new, index);

if (!index) index = searchFor (old);

MasterJames on 10 Mar 2016

@mrdoob @MasterJames What happens if there are multiple occurrences of oldChild? Will all be replaced?

I believe both methods can be useful, and that replaceChild(oldChild, newChild) can be implemented with replaceChildAt(i, newChild) as a helper. I just want direct access to the helper function replaceChildAt, as an alternative to either hacking the children array myself, or sticking with a less efficient and more ambiguous replaceChild method. (Less efficient because it must search for the oldChild, and more ambiguous because it could or couldn't replace multiple occurrences at once.)

EliasHasle on 11 Mar 2016

@mrdoob @MasterJames What happens if there are multiple occurrences of oldChild? Will all be replaced?

That's not possible, you can't add oldChild multiple times.

(Maybe there is a very clever way to achieve what I am trying to do, without resorting to complicated or comparatively slow procedures. I haven't really started using names and IDs for Object3Ds, neither have I tested the manipulation of visibility, so maybe I am missing out on something important.)

You could just create a container:

var container = new THREE.Group();
body.add( container );

function changeHat( style ) {

    if ( container.children.length > 0 ) {

        container.remove(  container.children[ 0 ] );

    }

    container.add( hat[ style ] );

}

mrdoob on 11 Mar 2016

Maybe he wants to pass arrays of both index and newObjs.
Really with a helper of any kind you can just inject after THREE is loaded to insert a function of any kind into the protoclass for your custom needs. Maybe you need to call some cleanup functions while you'really at it too.
I think you need to explain "multiple occurrences" of old? like you should search for an array of indices that match old or something?

MasterJames on 12 Mar 2016

That's not possible, you can't add oldChild multiple times.

@mrdoob Oh, OK. I see. I understand why not to do that (because they can't be manipulated independently, not even their position), and technically an object can have only one parent.

You could just create a container: ...

OK, that is a kind of solution, but feels a bit hackish to me. Having an extra logical level just for using only the zeroth array index. That at least avoids errors of undefined behavior, but the code gets a bit bloated (and the extra level in the object tree incurs an extra random memory access, right?):

Maybe he wants to pass arrays of both index and newObjs.

@MasterJames I didn't have that in mind. I was just wrong about the "multiple occurrences" thing.

Really with a helper of any kind you can just inject after THREE is loaded to insert a function of any kind into the protoclass for your custom needs. Maybe you need to call some cleanup functions while you'really at it too.

Wow, that's interesting! So I would do something like:
Object3D.prototype.replaceChildAt = function (index, newChild) { ... };
and that will always be possible, even with future releases where the children array is made read-only from the outside? That is definitely better than nothing. But my hope is that you also see such a helper method as useful enough to want it included in the library.

EliasHasle on 15 Mar 2016

I was just trying to interpret and make suggests. I have little clout here.
I would say if you did a PR with your suggestion it's up to others here to decide that have an understanding of average users needs.
I think it's just as easy to inject it, as it is to include it, so it would not be hard to convert an initial PR as an injectable helper, to a core item and either or could be integrated and supported.
Anyway I liked the idea of adding an index as a third argument and searching if not provided.
As im sure you're aware one can even rename the original, overwrite where it is while calling the renamed original and still benifit from future updates etc.
Anyway it seemed it's a limited use case and not suggested for core level at least initially unless others liked and used the initial helper version. Then again I'm still trying to learn what's acceptible design patterns here since my thinking is usually not in line with easiest to use API i think is worked great here. I like extra options like this idea, but as a 3rd argument, and thought a good compromise to consider.
All input is appreciated and respected. I'm sure decisions are tough and I suspect never truly final.

MasterJames on 15 Mar 2016

So, I guess we can add both:

object.replace( oldChild, newChild );
object.replaceAt( index, newChild );

object.replace would use object.replaceAt internally.

Sounds good @WestLangley?

mrdoob on 15 Mar 2016

👍1

You could just create a container: ...

OK, that is a kind of solution, but feels a bit hackish to me.

I think it is elegant.

Regarding object.replaceAt( index, newChild ), the user is responsible for disposing of objects properly. How would the old child and its children be disposed of? And how would the user know to do that?

WestLangley on 15 Mar 2016

@WestLangley I imagined that the object would not be disposed of, but returned by the method for easy handling afterwards. In my use case I wouldn't dispose of it afterwards, but keep it in memory for later.

EliasHasle on 15 Mar 2016

Is it here with "remove" that both a replace and replaceAt are desired?
https://github.com/mrdoob/three.js/blob/dev/src/core/Object3D.js#L322
and expected to come after remove?

MasterJames on 17 Mar 2016

@MasterJames Sounds OK to me. It matters only for library code readability, right?

EliasHasle on 17 Mar 2016

So this would do it? but I put it above "Remove" as logically it should fit between add and remove.
Speaking of Add should this call an addAt or they share some functions too?

    replace: function( oldChild, newChild ) {

        var index = this.children.indexOf(oldChild);

        if (index !== -1) {

            return this.replaceAt( index, newChild );

        }

    },

    replaceAt: function ( index, newChild ) {

        var oldChild = this.children[index];

        if (oldChild !== undefined) {

            this.children[index] = newChild;

            return oldChild;

        }
    },

Edit: added sanity check

MasterJames on 17 Mar 2016

We need to add a sanity check in the code above and then some docs...

        <h3>[method:null replace]( [page:Object3D oldObject], [page:Object3D newObject] )</h3>
        <div>
            oldObject - An object.<br />
        </div>
        <div>
            newObject - An object.<br />
        </div>
        <div>
            Replaces the old Object with the new, only if old is found.
        </div>
        <div>
            Returns the old Object that has now been replaced, but nothing if not found.
        </div>

        <h3>[method:null replaceAt]( [page:Integer index], [page:Object3D object] )</h3>
        <div>
            index - index of object.<br />
        </div>
        <div>
            newObject - An object.<br />
        </div>
        <div>
            If object is found at index, it is replaced with newObject.
        </div>
        <div>
            Returns the Old Object that has now been replaced at the index.
        </div>

MasterJames on 17 Mar 2016

I put that in through the github web page editor but I tested it only briefly.
https://github.com/mrdoob/three.js/pull/8388
Seems the docs didn't join the first one, ohps.
https://github.com/mrdoob/three.js/pull/8389

MasterJames on 17 Mar 2016

@MasterJames The code must change the parents of oldChild and newChild, and maybe it also should dispatch some events, like the add and remove methods do. Edit: See #8414. It is directly based upon the add and remove methods.

EliasHasle on 17 Mar 2016

Indeed...

Maybe we should also add addAt( index, child ) and removeAt() and turn .add into .addAt( children.length, child ).

mrdoob on 17 Mar 2016

👍1

@mrdoob What is the use case for inserting objects at specific indices in the children array?

WestLangley on 17 Mar 2016

@WestLangley When I recently re-created the outliner for http://threejs.org/editor/ I found that something like this could be handy when moving children around in the hierarchy. This is how I ended up doing it:

https://github.com/mrdoob/three.js/blob/master/editor/js/libs/ui.three.js#L272-L322
https://github.com/mrdoob/three.js/blob/master/editor/js/commands/MoveObjectCommand.js

mrdoob on 17 Mar 2016

@WestLangley (in particular:) My example with the hats is a bit silly. In my current application I am actually using my home-hacked replaceAt function to switch out an entire environment with waving ocean and everything almost instantly on the fly, in addition to changing ship parts (keeping the unused parts/environment ready in memory). Maybe this can be achieved in a more elegant way, but I have yet to find that. :-)

EliasHasle on 17 Mar 2016

I've attempted to add this with the addAt to cover the need for events.
I noted that while removing with the splice command, it was best to also insert at the same time so 'remove' also takes the items to insert, making replace and replaceAt just aliases to the quasi-'remove' function. Sorry couldn't help myself.
https://github.com/mrdoob/three.js/pull/8406

I also battled grievously with my reformatting settings and other git-glitches.
Sadly it still is a TAB off somehow?
My testing has been limited.

MasterJames on 18 Mar 2016

This PR is bogus. I'm redoing it sorry for rushing it.

MasterJames on 19 Mar 2016

Okay Please pardon my early submission.
I hope you can excuse me while I learn the process of submission PRs via GitHub etc.
The same PR different changes.

https://github.com/mrdoob/three.js/pull/8406/files

In order to add indexing I've taken this approach.

This one adds addAt and 'replace', and a lookUp that's not used internally, but it will return either the index of the object or the object with the index. This is similar to how the replace functionality also works to find the target to remove/replace and possible insert/splice in a replacement.

There's also a tweak I did to EventDispatcher that's worthy of a separate PR I later realized, please review that too, thanks.

MasterJames on 19 Mar 2016

If th object exists it moves index position and emits a 'moved' Event. Also events get an added index with type and they return removed objects for cleanup. Except remove which returns 'this' (the parent as before).

MasterJames on 19 Mar 2016

@mrdoob Is it acceptable to have undefined values in the chileren array? I know it wouldn't work well now, because some parts of the code elsewere assumes that the chileren array contains Object3Ds. My question arises when considering what should happen when a user calls replaceAt with a newChild that is already a child at another index. Because I don't want to move the other children to "wrong" indexes by removing and splicing at the old index. The least radical and most consistent solution, I think, is to disallow newChild if it exists at another index, and ignore it if it exists at the given index. That corresponds well with how I would use the method. Would that be OK? Update after two years: Hmhmhmhm, that incurs a search operation too...

EliasHasle on 21 Mar 2016

The new lookUp function allows one to double check if they don't want to do it cause it exists in the right place but I would also add a flag preferable just looking to avoid extra stuff as lookUp is a useful shortcut/tool/function to provide for checking without checking the the children directly yourself.
Anyway if they try to put it in the same place I don't think it moves it.