Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mesh/Line/Points: Fix copy(). #19471

Merged
merged 2 commits into from
May 29, 2020
Merged

Mesh/Line/Points: Fix copy(). #19471

merged 2 commits into from
May 29, 2020

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented May 26, 2020

Fixed #16224.

This PR implements the approach I have outlined here: #16224 (comment)

The idea is to remove all clone() methods in mesh, points and line classes and always use the version of Object3D. The copy() methods are now responsible for copying all properties.

@Mugen87 Mugen87 requested a review from WestLangley May 26, 2020 19:16
@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 26, 2020

The difference in the new implementation is the invocation of updateMorphTargets(). Previously, the method processed the geometry of the source mesh. Now, it is processing the default geometry and thus producing no result. However, morphTargetInfluences and morphTargetDictionary are copied in copy() so you still get the same object status.

In any event, there might be use cases where code could break. E.g.

var mesh1 = new THREE.Mesh();
mesh1.geometry = geometry; // assign geometry with morph attributes
var mesh2 = mesh1.clone();

Previously, mesh2 would have valid morphTargetInfluences and morphTargetDictionary properties. Now, these properties are missing since copy() replicates the status of mesh1(which has no morphTargetInfluences and morphTargetDictionary). However, I think the new behavior is the result you would normally expect.

@WestLangley
Copy link
Collaborator

This is getting better. Thank you for working on it.

Even so, I have long-advocated removing clone() for a host of reasons -- except for the math classes, maybe. ***


*** But I do not like it there either because it violates our so-called policy of requiring users to allocate memory at the application level. Plus, inexperienced users are known to call clone() unnecessarily in tight loops.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 26, 2020

How about discussing a potential removal of Object3D.clone() in a new issue? AFAICS, the knowledge about this topic is spread over multiple issues and PRs. Maybe it's easier to draw attention to the downsides by having a single issue.

Copy link
Collaborator

@WestLangley WestLangley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this PR an improvement, so let's merge it.

You can wait until r.118 if you want to be safe.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 26, 2020

You can wait until r.118 if you want to be safe.

I would prefer that so we have more time for testing 😇 . Although I think this change should be safe.

@Mugen87 Mugen87 added this to the r118 milestone May 26, 2020
@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 29, 2020

@mrdoob Since we are at the beginning of a new dev cycle, can we merge this PR? 😇

@WestLangley
Copy link
Collaborator

WestLangley commented May 29, 2020

I think this is worth a try. But we will see...

I think @mrdoob prefers to wait several days after a release to ensure there are no hot-fixes required.

@mrdoob
Copy link
Owner

mrdoob commented May 29, 2020

Yeah, I think we're good. Only GLTFLoader had a minor issue but I updated npm only with the fixed GLTFLoader.

@mrdoob mrdoob merged commit d687097 into mrdoob:dev May 29, 2020
@mrdoob
Copy link
Owner

mrdoob commented May 29, 2020

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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