Clone Proposal: Add ID cache to Object3D, DirectionalLight copy and clone functions

[gkjohnson]

Related to #17299 (comment)

This adds an id cache to the Object3D copy and clone functions to fix the clone issues described in the above issue and updates Light.copy and DirectionalLight.copy to take advantage of it.

The short of the problem is that if objects have separate references to objects that live in the scene (such as DirectionalLight.target) then clone a subtree with the objects results in losing the relationship between those objects after the clone / copy (as happens with the light and target). Here's some code demonstrating the problem:

const obj = new THREE.Object3D();
const directionalLight = new THREE.DirectionalLight();
obj.add( directionalLight, directionalLight.target );

const clone = obj.clone();

const objEqual = obj.children[ 0 ].target === obj.children[ 1 ];
const cloneEqual = clone.children[ 0 ].target === clone.children[ 1 ];

console.log( objEqual, cloneEqual );

// in dev: "true false" is logged
// this PR: "true true" is logged

The proposed pattern enables the following behavior with DirectionalLight, which should extend to other objects:

• If target is not parented to anything then the directional light is cloned with a new target object also unparented.
• If target is parented within the subtree that is being copied / cloned then target is cloned and is parented at the same position in the hierarchy in the new subtree.
• If target is parented outside the subtree that is being copied / cloned then target is cloned but is left unparented.

[donmccurdy]

Thanks! I like this solution, if it can be extended to SkinnedMesh. SkinnedMesh has a .skeleton.bones array, which points to Bones that are (probably) also in the subtree being cloned. I don't see any reason that shouldn't work, I just haven't had the chance to check it.

To mention an alternative, I would also support #13373 and #14658, which could remove light.target entirely. That wouldn't fix cloning a SkinnedMesh, though, so this PR may be worthwhile for that case alone.

[EliasHasle]

Noise

🎖 for picking up and refining my idea and going ahead with making a PR.

I think SpotLight should be fixed in the same PR, once a working pattern is found. And then we have a similar situation with CameraHelper.camera: #17299 (comment)

[gkjohnson]

🎖 for picking up and refining my idea and going ahead with making a PR.

Ha hopefully it doesn't feel like I stole your thunder! I just felt like we'd gotten to a reasonable solution and wanted to see it in context.

I think SpotLight should be fixed in the same PR, once a working pattern is found. And then we have a similar situation with CameraHelper.camera: #17299 (comment)

I just wanted to define a minimal example here with the new pattern -- if this gets merged / approved I can look into updating the other classes including the helpers, spotlight, skinned mesh, etc.

[EliasHasle]

Noise

> Ha hopefully it doesn't feel like I stole your thunder!

Likewise. 😉 No, I am honored. 😃

Looks like my review comments got sorted out well. Have you run any tests with normal and more exotic cases?

[gkjohnson]

Have you run any tests with normal and more exotic cases?

Only the one I mentioned above. Here are the cyclic cases the @donmccurdy mentioned in #17299 (comment), which might count as exotic example cases:

two lights being eachothers' targets

const dl1 = new THREE.DirectionalLight();
const dl2 = new THREE.DirectionalLight();

dl1.target = dl2;
dl2.target = dl1;

const clone = dl1.clone();
console.log(clone.target.target === clone);
// in dev: throws stackoverflow exception on "clone"
// this pr: "true" is logged

lights target is its parent

const dl = new THREE.DirectionalLight();
dl.target.add(dl);

const clone = dl.clone();
console.log(clone.target.children[0] === clone);
// in dev: throws stackoverflow exception on "clone"
// this pr: "true" is logged

Both of these cases actually fail entirely in dev because the cyclic nature of the relationships causes an infinite recursion but it works properly in this branch. Regarding "normal" use cases -- calling new DirectionalLight().clone() works just fine unless you had something else in mind? I can test any others, too.

[EliasHasle]

Noise

How about: One target attached to an object, and many lights with different relation to it pointing at it.

[gkjohnson]

One target attached to an object, and many lights with different relation to it pointing at it.

Yeah that will work, too:

const scene = new THREE.Scene();
const dl1 = new THREE.DirectionalLight();
const dl2 = new THREE.DirectionalLight();
dl2.target = dl1.target;
scene.add(dl1, dl2, dl1.target);

const clone = scene.clone();
console.log(
    clone.children[0].target === clone.children[2],
    clone.children[1].target === clone.children[2],
);

// in dev: "false false" is logged
// this pr: "true true" is logged

[EliasHasle]

Noise

> with different relation to it

I meant like one light is a descendant, one is an ancestor.? Not that I expect it to fail or anything.

But what if the target is an ancestor to the light, but the clone three does not include the target? Will there be an extra copy of the entire target tree, detached from everything else? And will this copy use cache and "steal" objects from the real scene? If so, this is becoming problematic, and one may need to keep track of the subtree set from the start, as I suggested in #17299 (comment) (but otherwise with your improved logic), and have another default behavior than having a detached clone. I think intuitively that it is better to let the cloned light refer to the original target tree when the target is outside the tree to be cloned.

[gkjohnson]

Can you sketch out a hierarchy that you expect to be a problem along with which node should be cloned to illustrate the issue? I'm not sure if I'm following the description.

[EliasHasle]

//Hierarchy:
scene: {
	children: [
		obj: {
			children: [
				dirLight: {
					target: scene
				}
			]
		}
	]
}

obj.clone();

This is a case where I would prefer that the cloned dirLight simply gets a copy of the reference to the target (scene).

Keep in mind that Object3D.add will remove from previous parent:

three.js/src/core/Object3D.js

Line 332 in 7e0a78b

if ( object.parent !== null ) {

  		object.parent.remove( object );

[gkjohnson]

With the PR as it is now cloning obj will result in a clone of the complete tree as you've sketched it including the scene which will be the clone obj's parent and the cloned directional light's target.

I don't have any strong opinions on what should happen here mostly because I don't really feel there's a one-size-fits-all solution so I'm okay with whatever solution the project owners feel is best.

The behavior you're describing where a reference is maintained to objects outside the subtree would require something like the following changes to this PR:

Object3D.copy = function(...) {

  if ( cache === undefined ) {

    // prep by saving all the source uuids  in the subtree 
    // we know we'll want to clone ahead of time
    cache = {};
    source.traverse(c => cache[ c.uuid ] = null );    

  }

  // ...

}

DirectionalLight.copy = function(...) {

  // ...

  // If the id is within the cache then it should be cloned otherwise
  // just use the existing object that's outside the subtree.
  if ( source.target.uuid in cache ) {

    if ( cache[ source.target.uuid ] === null ) {

      source.target.clone( recursive, cache );

    }
    this.target = cache[ source.target.uuid ];

  } else {
    
    this.target = source.target;

  }

  // ...    

}

[EliasHasle]

Noise

With the PR as it is now cloning obj will result in a clone of the complete tree as you've sketched it including the scene which will be the clone obj's parent and the cloned directional light's target.

Ah, I see. Because the clone root will be initially orphan and added to cache, and when it is encountered during recursive cloning of the target/scene it will be taken care of from the cache by a wonderful cloned parent.

The behavior you're describing where a reference is maintained to objects outside the subtree would require something like the following changes to this PR:

So elegantly written.

I am thinking in practical applications here. How often does an average user want the whole scene to be cloned when he clones a subtree which contains a light that targets the scene, compared to how often he wants clones with lights that target the same scene? I don't know all practical use cases of cloning, but I have only ever used it to duplicate objects for use within a scene.

Another case:

//Hierarchy:
scene: {
	children: [
                movingObj
		dirLight: {
			children: [
				lightMesh
			],
			target: movingObj
		}
	]
}

dirLight.clone();

The user's intention is to have multiple lights with attached geometry, following the moving object. But since movingObj is outside the clone tree, it will be cloned, and the clone will be an orphan. Unless you use the second version. Right?

[trusktr]

This default behavior is very reasonable. It can be described as a general concept for Object3D.clone, that applies to any object that may reference any other sort of object located somewhere else in the tree outside of the tree being cloned.

[trusktr]

As far as the implementation, I imagine doing it a little differently, encapsulating it all in Object3D, and not requiring a separate cache arg to the public methods, so that not every subclass has to repeat similar logic.

[EliasHasle]

@trusktr Are you referring to the behavior in the original post of this PR, or to the one described in #17370 (comment)?

Regarding implementation, do you have a suggestion on how to encapsulate the behavior in Object3D, a solution that does not involve passing a cache?

@gkjohnson I will hide away a lot of my noise from the thread to make it more readable.

[gkjohnson]

It turns out this is causing issues with GLTF files when you clone them, as well, since the target object is no longer added as a child of the directional light as it originally when loading a GLTF file. You can see in this fiddle that if you clone the gltf model before adding it to a scene and it has a directional light embedded the displayed result is different.

https://jsfiddle.net/8795teLf/

	w/o clone()	clone()
screenshots
computed light direction	0, 0, 1	0, 0, 0

This also effects model-viewer, as well, and can cause the lighting of the model to change depending on the model position (cc @elalish):

https://jsfiddle.net/keLcuptz/

When you clone an object it should clone with as many internal references intact, as possible instead of losing all the references. This would be really nice to have fixed in some way. This was a massive pain to track down and understand why there were lighting differences between MV and my project.

[trusktr]

Regarding implementation, do you have a suggestion on how to encapsulate the behavior in Object3D, a solution that does not involve passing a cache?

My reply is a little late, so I don't remember exactly what I thought back then, but I think that this code:

		if ( ( source.target.uuid in cache ) === false ) {

			source.target.clone( recursive, cache );

		}
		this.target = cache[ source.target.uuid ];

could just be this:

		this.target = source.target.clone( recursive, cache );

where .clone() can just return a cached object if any, then this type of conditional check does not need to be repeated in subclasses of Object3D because it would be encapsulated in Object3D.clone(). Would there be a downside to this?