Repository navigation
Keep cloned Geometry morph weights independent - #3003
Open
toaster0123 wants to merge 2 commits into
Open
toaster0123 wants to merge 2 commits into
toaster0123 wants to merge 2 commits into
Conversation
riccardobl
marked this pull request as ready for review
October 2, 2026 17:47
jaime-jmebot
approved these changes
Oct 2, 2026
jaime-jmebot
left a comment
Contributor
There was a problem hiding this comment.
Nice and tight fix — exactly the right size for the problem.
- Cloning
morphStatethrough the existingCloner(next tocachedWorldMat) is the minimal, idiomatic fix: each clone gets its own weight array with the same values, uninitialized/null state still stays lazy, and array identity stays consistent inside the cloned graph. Mesh/material sharing policy is untouched. - The regression file covers the real surface well: both setter directions across
clone(),clone(false),deepClone()andNode.clone(true), clone-of-clone, uninitialized state, and the dirty-flag behavior. - Tiny optional nit:
geometryWithoutMorphTargetsStillClonesclones first and only then callssetMorphState(new float[0])on both objects, so it passes with or without this change. Setting the (empty) morph state before cloning would make it a real control for the zero-length-array path. Not blocking.
Contributor
Author
|
Thanks for checking this. The no-target case is intentionally a passing compatibility control; the initialized-weight cases supply the before-failing regressions. Moving setMorphState(new float[0]) before the clone would still leave morphState uninitialized, because the setter returns immediately when the mesh has no morph targets. An explicitly allocated zero-length-array case would need getMorphState() first. I have kept the existing control and left this approved change unchanged. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Changing a cloned geometry's morph weights can also change the original geometry.
For example, set the original's
smileweight to 0.2, clone it, then set the clone'ssmileto 0.8. The original now reports 0.8 too, even though its dirty-morph flag stays false. The same problem occurs with the array setter,clone(false),deepClone(), and cloning the containing Node.This happens when the original's weight array was initialized before cloning. The array is shallow-copied, so both geometries write to the same storage. Cloning before either weight array is initialized already works.
Spatial's cloning contract allows independent scene changes, with documented mesh/material-sharing choices. Morph weights belong to each Geometry; they are not shared mesh data.
Fix
Clone
morphStatethrough the existingClonerinGeometry.cloneFields.This copies existing weight values, preserves null/lazy initialization, and keeps references within the cloned object graph consistent. It leaves the mesh/material-sharing policy and dirty flag behavior unchanged.
The production change is one line, based directly on current upstream
master. It does not depend on #2997 or #3002.Validation
Limits
These checks establish per-geometry weight-state independence, not pixel output or complete morph-rendering independence. Material parameter and mutable fallback-buffer cache behavior are outside this change.
The skips are the existing FastMath counterclockwise test and a desktop test requiring a writable noexec filesystem. The local whole-repository Android build remains unverified because the Android SDK is unavailable.