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

BufferAttributes and Materials are cloned by reference for Object3D instances, no matter what #30351

Closed
wants to merge 13 commits into from

Conversation

srmcgann
Copy link

Related issue: #30350

BufferAttribute arrays (for Object3D instances) are cloned / copied by reference.
this causes all predecessors of a clone to receive transformations such as rotations, which are meant to be applied only to clones.

The 'recursive' parameter for Object3D instances does not conform to new user expectations, making the method counterintuitive and user-unfriendly.

Users should expect any "recursive" clone to be a complete, new set. This update should be merged, or "recursive cloning" of Object3D instances should be removed.

bugfix:  clone/copy
Copy VALUES, not just references to the source Attribute array
This fixes e.g. 'cumulative rotations' etc., for clones which would
apply transformations to their predecessors, otherwise
bugfix:  clone/copy
Copy VALUES, not just references to the source Attribute array
This fixes e.g. 'cumulative rotations' etc., for clones which would
apply transformations to their predecessors, otherwise
Simplified change to use existing methods for deep/recursive clone/copy of Object3D instances

Recommend making recursive default to false, if recursive cloning of BufferAttributes and Materials is undesirable for the default.
src/core/Object3D.js Fixed Show fixed Hide fixed
Copy link

github-actions bot commented Jan 17, 2025

📦 Bundle size

Full ESM build, minified and gzipped.

Before After Diff
WebGL 336.25
78.28
336.25
78.28
+0 B
+0 B
WebGPU 503.18
139.76
503.18
139.76
+0 B
+0 B
WebGPU Nodes 502.65
139.65
502.65
139.65
+0 B
+0 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Before After Diff
WebGL 465.25
112.13
465.38
112.16
+126 B
+32 B
WebGPU 576.96
156.41
577.09
156.44
+126 B
+33 B
WebGPU Nodes 532.35
145.96
532.47
145.99
+126 B
+33 B

src/core/Object3D.js Fixed Show fixed Hide fixed
removed unnecessary include
added conditional testing to check for the existence of instance properties
appeasing linter...
changed clone method due to CI error
src/core/Object3D.js Fixed Show fixed Hide fixed
more lint worship
lint worship
moar lint worship
@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 18, 2025

Users should expect any "recursive" clone to be a complete, new set. This update should be merged, or "recursive cloning" of Object3D instances should be removed.

I think your interpretation of a recursive clone is too strict. It's not mandatory that a recursive clone must be a "deep" one that clones every aspect of the 3D object. This really depends on the use case. In most use cases, developers want to share the materials and geometries for performance reasons.

this causes all predecessors of a clone to receive transformations such as rotations, which are meant to be applied only to clones.

That is actually not true and it seems you misunderstand a basic aspect of 3D transformations. 3D objects with the same geometry can have different transformations without affecting each other. It's probably best if you use the forum to clarify this aspect.

Sorry, but this change can't be merged.

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.

2 participants