Three.js: Remove "stealing" behavior from Object3D.add

[dantoye]

Description of the problem

I was recently bitten by some very unintuitive and difficult to trace behavior.

When adding a child Object3D to a parent Object3D, such as in scene.add(mesh), there is understandably the invariant that a child can only have one parent, so it is set to the parent. However, I believe it is overly aggressive in maintaining that invariant.

What is not clear, from the method name (add), the documentation, or even intuition, is that if the child already has a parent, it will be forcibly evicted from it's current parents "children" array.

This caused a huge problem for me, as I imported a mesh hierarchy from a Three.JS official 3D model loader, which created an object graph internally. I appreciate the library taking this initiative for me. I was iterating over the children in that imported file and adding them to my scene in different places, but I was randomly missing a few of the children, roughly half of them.

It took far too long to discover that "scene.add(item)" will perform an "item.parent.remove(item)" beforehand. I feel cheated by the method being named "add", when in fact it is more akin to "steal". It is against the intuition of that "add" means.

I still don't even know the best way to do this - should I copy the Object3D's as I iterate over them and add the copy to the scene?

What I think would be more intuitive is for the "add" method to throw an exception if adding the item would violate the single-parent invariant, and provide a new method, perhaps "transfer", "assign", or "steal", that will steal it from it's current parent if necessary. At the very least, please document this behaviour, as kidnapping is simply not okay.

• jsfiddle

Three.js version

• [ ] Dev
• [X] r92
• [ ] ...

Browser

• [x] All of them
• [ ] Chrome
• [ ] Firefox
• [ ] Internet Explorer

OS

• [x] All of them
• [ ] Windows
• [ ] macOS
• [ ] Linux
• [ ] Android
• [ ] iOS

Hardware Requirements (graphics card, VR Device, ...)

[Mugen87]

A scene graph is a tree structure. When you add an element, you change its parent.

Sry, this behavior is so obvious that I don't think we need an explicit note in the docs.

[looeee]

I don't think there would be any harm in adding a note though - even something as simple as this would be fine:

Object3D.parent:

Object's parent in the scene graph. An object cannot have more than one parent.

[Mugen87]

Okay, if you like :wink:

[dantoye]

Mugen, maybe I wasn't clear. I understand the invariant - a child can only have one parent. However that does not imply the other invariant - a parent must not have a child who does not have it as it's parent.

I am arguing that, adopting a node that already has a parent, should raise an exception for breaking the invariant, not forcefully repair the invariant.

My main argument is - who is the current behaviour it benefitting? In what workflow is this the behavior you actually want? In my opinion, the most obvious workflow was to load in my model once, and add it to the scene as necessary, and if I needed multiple copies in a single scene I would clone it. To me that flow sounds reasonable and it's only because of this small subtlety that it wouldn't work.

Throwing an exception will quickly alert the user they are doing something they probably did not intend to do, unless I am wrong and most users and workflows expect scene.add() to remove the child from it's existing spot, in which case my expectation was atypical.

[donmccurdy]

Sorry that this behavior bit you like it did — I think an update to the docs for .add() like Adds object as child of this object, and removes from the current parent if any. would be reasonable. But I don't think renaming the method or throwing an error would be warranted — our use of .add is consistent with DOM, jQuery, other tree implementations. Others would be surprised by the opposite behavior, just as you were surprised here.

[dantoye]

That's an interesting point, it does behave like the DOM. I had not considered that parallel. My bug would have happened with the DOM too, were I iterating over a DOM node's children and adding them to another element it would "skip" every second child, and there would be no exceptions in that case, either.

That behavior, however, is well-documented, and perhaps that's my real issue?

W3Schools: "You can also use this method to move an element from one element to another."

Mozilla: "If the given child is a reference to an existing node in the document, appendChild() moves it from its current position to the new position"

It seems the behavior is unintuitive enough in the DOM that it is noted frequently and rather early in documentations, which I believe is argument enough that it should be decently noted in the documentation for three. Do you agree, Mugen? You said it's so obvious it doesn't warrant documentation, does this convince you otherwise?

[mrdoob]

I still don't even know the best way to do this - should I copy the Object3D's as I iterate over them and add the copy to the scene?

You can do this...

var arm = model. getObjectByName( 'arm' );
scene.add( arm.clone() );

Mozilla: "If the given child is a reference to an existing node in the document, appendChild() moves it from its current position to the new position"

Adding something like that to the documentation of add() sounds good to me.

[looeee]

Added notes in #13932