Mesh.copy() does not copy the geometry or material

[WestLangley]

For example:

var geometry = new THREE.BoxBufferGeometry( 1, 1, 1 );

var material = new THREE.MeshNormalMaterial();

var mesh = new THREE.Mesh( geometry, material );


var m = new THREE.Mesh().copy( mesh );

console.log( m.geometry ); // BufferGeometry with no attributes

console.log( m.material ); // MeshBasicMaterial

Likewise for Line, LineSegments, and Point.

Sprite, does not copy the material.

[Usnul]

It would be nice to have unit tests for that

[Mugen87]

My explanation for the current design is that Mesh.clone() assigns the geometry and material to the constructor call:

return new this.constructor( this.geometry, this.material ).copy( this );

Hence, Mesh.copy() does not have to copy those properties anymore. So a fix for Mesh.copy() also includes a change for Mesh.clone(). Ideally the latter one should look like so:

return new this.constructor().copy( this );

In this case, the method can completely removed since the implementation is identical to Object3D.clone(). However, changing Mesh.copy() could introduce the same problem like #15062. Instead of

this.geometry.copy( source.geometry );
this.material.copy( source.material );

it's maybe better to do:

this.geometry = source.geometry;
this.material = source.material;

Otherwise the code will fail if you try to clone a Mesh with an instance of Geometry or with a different material than MeshBasicMaterial, see #15974. Simply because a copy between different geometry and material types is not supported. The assign operation avoids this issue. The downside of this approach is that changing a material or geometry now affects multiple meshes after a copy operation. This could be unexpected since the objects have no independent properties anymore. However, this is also true for the current clone operation.

[WestLangley]

Mesh.clone() assigns the geometry and material to the constructor call ... hence, Mesh.copy() does not have to copy those properties anymore.

Do you agree that is not logical?

[Mugen87]

Do you agree that is not logical?

Yes^^. I think we need a better design. Removing Mesh.clone() and implement copy like I mentioned in my previous post could be worth a try.

[WestLangley]

@Mugen87 Would you like to try your suggestion?

[Mugen87]

I'm currently trying to find a way to avoid duplicate texture uploads so I'm not going to focus on this topic right now. Maybe at a later point unless somebody else has not started working on it.

[sciecode]

The downside of this approach is that changing a material or geometry now affects multiple meshes after a copy operation.

I don't think this is ideal. I believe in most use-cases of clone() on Mesh, Line, Points are with the intention of avoiding construction of set geometry and material and just applying unique changes afterwards. By setting a reference to both, it pretty much defeats the purpose of doing it in the first place.

Although a bit very questionable, we could default geometry.copy() to geometry.clone() if there's a type mismatch, same thing for material. Which would allow for the following:

// clone
return this.constructor().copy(this)

//copy
this.geometry.copy( source.geometry );
this.material.copy( source.material );

Given that, currently, copy() doesn't work between type mismatches either way, it might not be as bad as it sounds. But I understand if we don't want to go down this rabbit hole.

[WestLangley]

Reopening, as requested in #19455 (comment).