Three.js: clone issue: SpotLight.target which is placed in scene graph is cloned in two roles

[EliasHasle]

Description of the problem

Case: Make many similar lamps with geometry and attached SpotLight.

````
let scene = new THREE.Scene();
let lampMesh = new THREE.Mesh(fooPars);
let spotLight = new THREE.SpotLight(barPars);
spotLight.target.position.set(1,2,3);
lampMesh.add(spotLight, spotLight.target);

for (let i = 0; i < numLights; i++) {
let lightClone = lampMesh.clone();
lightClone.position.set(0,0,i);
scene.add(lightClone);
}
````

The cloning will copy the target independently in the roles of lampMesh.children[1] and of lampMesh.target. This will cause the lamps to interpret the target position as a global position, rather than a local one, and moving the targets in the scene will have no effect on the lights, because their actual target properties are independent clones with no parent. And the error is/was really hard to locate.

Proposed solution: I am not sure. The only one I can think of is adding an optional parameter to _all_ clone methods, that contains a map (literal object) from original ID/UUID to already cloned objects, such that whenever an already cloned object is encountered, the existing clone will be reused in the new role. The map is passed down the recursion, and must also be passed to the copy methods. This seems like a drastic solution which will require a substantial amount of work, but it is also a general solution which will solve similar problems that _may_ exist elsewhere. I am not sure it is without unintended consequences, though.

User solution with current implementation: Don't use clone without knowing how it will behave. For the case above, consider either adding targets to scene after cloning, or building the copies without using clone, e.g. by using instances of a class which has static geometry and material.

Please also include a live example if possible (maybe later). You can start from these templates:

• jsfiddle (latest release branch)
• jsfiddle (dev branch)
• codepen (latest release branch)
• codepen (dev branch)

[Mugen87]

Many issues with clone()/copy() methods were exposed in the past like #16076 or #16224. This is obviously another one.

TBH, I'm not a fan of trying to make the clone more fancy like described in your proposed solution. What you have demonstrated seems to me a conceptual issue of the engine which is now hard to change. Removing all clone() and copy() methods from complex classes like Object3D might be the easiest and clean approach.

[WestLangley]

What you have demonstrated seems to me a conceptual issue of the engine

@Mugen87 Can you please explain what you see as the problem with the library in this use case? If you are not sure, it is OK to say so. I am interested in specifics. (The OP's description is not clear to me...)

EDIT: In other words, I agree clone() has problems, but why is this an example of a problem with the library and not a user error?

[EliasHasle]

@Mugen87 I don't think my proposed solution is _too_ fancy, really. It is easy to implement and understand, but I don't have overview of all consequences. It must be clear what cloning means, i.e. what we expect a user to expect when cloning.

For instance: What is the intended behavior if the object to be cloned is not an ancestor to the target, but the target has a parent elsewhere in the scene? Should the target then not be cloned, but linked? Should the target be cloned and detached (keeping global transform)? Should it be cloned and moved to null-parent space (keeping local transform)?

@WestLangley I tried to be clear. I will gladly clarify if you can be specific on what you find unclear. :) Responding to your edit: It may be a user error in the sense that there exist more robust ways to create similar objects. But clone _almost_ works, and the error is hard to locate. The mere existence of clone will tempt people/beginners to use it, and if the behavior is unpredictable/inconsistent/ill-defined, then maybe clone should be discouraged or even deprecated.

[WestLangley]

Let's see what @Mugen87 has to say in response to my two questions...

[EliasHasle]

A sketch of the "fancy" proposal:

clone(recursive=true, cloneCache={}) { return new this.contructor().copy(this, recursive, cloneCache); } copy(source, recursive=true, cloneCache) { //Repeat this for several properties: if (recursive && cloneCache && source.someProperty.UUID in cloneCache) { this.someProperty = cloneCache[source.someProperty.UUID]; } else { this.someProperty = new source.someProperty.constructor(); cloneCache[source.someProperty.UUID] = this.someProperty; this.someProperty.copy(source.someProperty, recursive, cloneCache); } }

Edit: The sketch now shows recursive propagation of the clone cache.

[Mugen87]

Can you please explain what you see as the problem with the library in this use case?

It's probably best to explain this issue with CameraHelper. As we know, CameraHelper.clone() is currently broken. When fixing the helper via #16076, another problem in context of the clone operations was revealed. Similar to what @EliasHasle describes here.

When a 3D object has a property that refers to another 3D object in the scene e.g. helper.camera, there are two issues that might occur.

• The camera will be cloned but helper.camera still refers to the old camera.
• The camera in the scene will be cloned _and_ helper.camera which results in two new, independent camera objects.

When I understand @EliasHasle issue correctly, the latter one happens here. From my point of view, both behaviors are wrong and unexpected for the user.

[WestLangley]

Thanks.

I have in the past advocated removing clone() from all methods except the math classes. But we could just try removing clone() from certain classes and see what happens.

[Mugen87]

I have to admit that I favor your approach more than in the past. I'm just afraid this approach might break a lot of user code since I've seen the usage of clone() so many times. It's a bit an unfortunate situation...

[WestLangley]

Also, clone() calls copy(), and SpotLight.copy() does this:

this.target = source.target.clone();

Whether the target should be cloned or not depends on the use case.

This is a complicated issue. three.js is very flexible, and with flexibility comes a responsibility on the part of the user to understand how it works. Sometimes, that means reading the code or stepping through with a debugger. I think most users eventually come to understand this responsibility.

[gkjohnson]

I have in the past advocated removing clone() from all methods except the math classes. But we could just try removing clone() from certain classes and see what happens.

I use clone for caching and cloning loaded 3d assets to improve load times when loading the same geometry multiple times and to save on memory so I would be sad to see it removed:

const cache = {};
function load(path, cb) {
    if (cache[path]) {
        cb(cache[path].clone());
    } else {
        (new GLTFLoader()).load(path, model => {
            cache[path] = model.clone();
            cb(model);
        });
    }
}

Because of this I think I'm unlikely to see the clone issues like those mentioned here but I think it would be difficult and tedious to replace the functionality if it were removed. It would also be frustrating to try to clone something but have it throw because an object with no clone function was deep in the hierarchy. It seems that the function works in 90%+ of use cases but if there are known gotchas with some objects then maybe they can be added to the documentation?

[looeee]

model.clone()

I expect that will fail when the model is a SkinnedMesh, and possibly in other unexpected places too.

[donmccurdy]

The challenge — both with SkinnedMesh and light.target — is that they can't always be handled recursively, as object.clone() assumes. The SkinnedMesh's bones or the light's target might be children of that node, they might be children of an ancestor, or they might not be in the subtree being cloned at all. That last case is more or less guaranteed to fail, and we might as well throw an error.

The workaround for cloning a SkinnedMesh is SkeletonUtils.clone( ... ), which does some extra processing before and after the cloning recursion.

var object2 = THREE.SkeletonUtils.clone( object1 );

A similar method could be moved to SceneUtils, to handle both cases. As an API I very much prefer var b = a.clone(), but it would be pretty complex to make that work perfectly. The parent class, Object3D, would probably need to know a lot about its subclasses, Light and SkinnedMesh.

[EliasHasle]

@donmccurdy This (or approximately this) would conserve b = a.clone(), right?

````
Object3D.prototype.clone = function(recursive=true, cloneCache, subtreeUUIDs) {
let theClone = new this.constructor();
if (!recursive) {
theClone.copy(this, false);
return theClone;
}

if (cloneCache===undefined) {
    treeUUIDs = {};
    this.traverse(obj=>subtreeUUIDs[obj.UUID]=true);
    //subtreeUUIDs now holds all Object3D descendants
    cloneCache = {};
}

theClone.copy(this, true, cloneCache, subtreeUUIDs);
return theClone;

}

SpotLight.prototype.copy = function(source, recursive=true, cloneCache, subtreeUUIDs) {
.... /other code/
if (recursive) {
if (subtreeUUIDs[source.target] === undefined && target.parent!==null) {
throw new Error("SpotLight.prototype.copy: Recursive cloning failed, because SpotLight target is attached outside cloned tree.");
}
if (cloneCache && source.target.UUID in cloneCache) {
this.target = cloneCache[source.target.UUID];
} else {
this.target= new source.target.constructor();
if (cloneCache) {
cloneCache[source.target.UUID] = this.target;
}
this.target.copy(source.target, recursive, cloneCache, subtreeUUIDs);
}
} else {
this.target = source.target.clone(false);
}
.... /other code/
}
````

[donmccurdy]

There are a couple parts of that I'm not sure about – mainly, after creating this.target = new source.target.constructor();, how does the target get attached to the right parent in the hierarchy? Its parent may not have been cloned yet.

Maybe there is a version of this API that will work — my guess is that it requires some sort of callback for certain nodes, after the entire subtree has been cloned, to update their references to targets and bones. For example:

class Object3D {
  clone ( recursive?: boolean, state?: CloneState ): Object3D {
    if ( ! recursive ) {
      return new this.constructor().copy( this, recursive, state );
    }

    const isRoot = state === undefined;
    state = state || new CloneState( this );

    const result = new this.constructor().copy( this, recursive, state );
    state.setResult( this, result ); // Stores cloning source and result for later.

    if ( isRoot ) state.finalize(); // Calls `onAfterClone()` on each result object.

    return result;
  }

  onAfterClone ( state: CloneState ): void {

    // Override in subclasses like SpotLight or SkinnedMesh. Use a method
    // like `state.getResult( uuid )` to look up targets and bones.

  }
}

It is a bit more complex than the current implementation. 😕

[EliasHasle]

There are a couple parts of that I'm not sure about – mainly, after creating this.target = new source.target.constructor();, how does the target get attached to the right parent in the hierarchy? Its parent may not have been cloned yet.

Objects encountered in the scene graph/tree use the cache too. So if the target is encountered first as spotlight.target and then as e.g. object.children[0], the target which is in cache will be added to the cloned object, so that it gets the right parent. That is unless I have overlooked something, which I frequently do.

[donmccurdy]

That makes sense in theory, in practice I'm not sure whether new source.target.constructor(); will work for all target types, e.g. if the target were a mesh it would normally expect the geometry and material to be passed into the constructor, and might _not_ transfer them during the copy() call. Not to say it won't work, it just might be difficult.

[EliasHasle]

Ah, that is a problem.

[gkjohnson]

@looeee

I expect that will fail when the model is a SkinnedMesh, and possibly in other unexpected places too.

It looks like that's true -- admittedly my use case is pretty simple since I primarily am concerned with rigid, non deforming geometry.

I do think it's nice to have at least some kind of reasonable default for cloning objects without having to resort to external utilities, especially for building creation tools, such as the three.js editor and other cases like mine.

@donmccurdy
I feel like the cache approach could work without having to add extra callbacks or finalize functions. DirectionalLight might overly simplify the problem but here's a quick pass:

Object3D.copy = function ( source, recursive, cache = {} ) {

  // Save the newly cloned object to the cache so it can be reused
  // later if other objects reference it.
  cache[ source.UUID ] = this;

  // ...

  if ( recursive === true) {

    for ( var i = 0; i < source.children.length; i ++ ) {

      // use the already cloned object if it exists in the cache otherwise clone it, which
      // which will cause it to be added to the cache.
      var child = source.children[ i ];
      this.add( cache[ child.UUID ] || child.clone( recursive, cache ) );

    }

  }

}

DirectionaLight.copy = function ( source, recursive, cache = {} ) {

  cache[this.UUID] = this;

  // ...

  // If the target hasn't been created as a side effect of the subtree clone then create it
  // now and add it to the cache so it will be added into the correct parent later.
  if ( ! ( source.target.UUID in cache ) ) {

      cache[ source.target.UUID ] = source.target.UUID.clone( recursive, cache );

  }
  this.target = cache[ this.target.UUID ];

  // ...

}

Maybe I've missed something but I think this would work? So in this case if the directional light target is part of the subtree being cloned then it will be added to the correct corresponding parent in the newly cloned subtree and if it is outside the subtree (or not parented to anything at all) then it will not be added to anything.

_edit: updated cache[ this.UUID ] = this; to cache[ source.UUID ] = this;_

[EliasHasle]

Object3D.copy = function ( source, recursive, cache = {} ) {

  // Save the newly cloned object to the cache so it can be reused
  // later if other objects reference it.
  cache[ source.UUID ] = this;

You don't really know here that this is supposed to be a clone, but maybe that does not matter? Hm, if you are right in that assumption, you have overcome the obstacle that I bumped into. By putting everything in copy and caching this. I suppose the handling (exception) of targets outside the cloned subtree can be done along the lines that I sketched.

[gkjohnson]

cache[ this.UUID ] = this;

Oops this should actually be cache[ source.UUID ] = this; -- updated in the example above.

You don't really know here that this is supposed to be a clone, but maybe that does not matter?

I don't think it matters. If a child refers to the root source object being copied then you'll want to retain that relationship in the new tree after the copy.

I suppose the handling (exception) of targets outside the cloned subtree can be done along the lines that I sketched.

I'm not sure I follow why this should be considered an exception. If the DirectionalLight target is outside the subtree being copied then a new target will be created but it won't be in any parent. Of course the desired behavior relies on the use case of the application but at least with this approach the clone will result in a self-contained subtree. Of course this caveat should be documented.

[EliasHasle]

Oops this should actually be cache[ source.UUID ] = this; -- updated in the example above.

OK, I updated the quote too.

I'm not sure I follow why this should be considered an exception. If the DirectionalLight target is outside the subtree being copied then a new target will be created but it won't be in any parent.

Do you think it should then be detached, so that it keeps its world transform? I think perhaps I would prefer that clone.target refers to the original object (when the target is not in the subtree). But usually not if the cloned tree is not added to the scene again. Hmmm... But both of these approaches make it more complicated. Maybe a doc change is enough for that.

[donmccurdy]

I'm not sure I follow why [light.target outside the cloned subtree] should be considered an exception.

I don't think users will be too confused by an error here – creating a symmetrical hierarchy, as clone() is expected to do, is impossible. It would be possible to just log a warning and attach the target somewhere else, yes, but the results are a bit unpredictable and they just get worse for SkinnedMeshes and Bones.

@gkjohnson Your solution does sound cleaner. I can't think of a reason it shouldn't work, although it's hard to think through cases like a light's target being a parent of the light (unusual, but it happens), a SkinnedMesh being a child of a Bone, or two lights being one anothers' targets. It still works because cache[ source.UUID ] = this; is set before recursion? Sounds pretty good to me if so.

[gkjohnson]

I've submitted PR #17370 that implements the behavior I described for DirectionalLight. I'm less familiar with the skeletal animation system so I'm not sure how it would extend to enabling SkinnedMesh cloning but maybe someone more experienced there can speak on the added complexities? Or I can take a look at it when I have a bit more time.

[gkjohnson]

@donmccurdy

I don't think users will be too confused by an error here – creating a symmetrical hierarchy, as clone() is expected to do, is impossible.

I guess I prefer not throwing an error because throwing prevents it from working altogether even though I may be fine with leaving a dangling target field that I can fix up later depending on the needs of my application. All I really want is reasonable, consistent, and documented behavior for the functions that can support as many use cases as we can. Maybe the failure case of SkinnedMesh is too problematic and wouldn't afford that, though -- I'm not clear on what exactly could happen.

(unusual, but it happens)

light = new THREE.SpotLight();
light.target = scene;
scene.add(light);

Ha, oh man. I guess here calling light.clone() would result in a cloned light that is parented to its target (which is the cloned scene). Seems weird but reasonable?

It still works because cache[ source.UUID ] = this; is set before recursion?

Yeah so any cloned objects are known about and available in the cache as soon as they been created and they can be referenced as needed when recursing.

[EliasHasle]

I've submitted PR #17370 that implements the behavior I described for DirectionalLight.

I think at least SpotLight must also be handled by the same PR.

light.target = scene;
scene.add(light);

Ha, oh man. I guess here calling light.clone() would result in a cloned light that is parented to its target (which is the cloned scene). Seems weird but reasonable?

How about scene.clone()?

[gkjohnson]

How about scene.clone()?

That should result in the same structure but you'll have a handle to a scene afterward.

[EliasHasle]

A more productive discussion follows from the concrete solutions in #17370. Closing here.