Skip to content

Keep cloned Geometry morph weights independent - #3003

Open
toaster0123 wants to merge 2 commits into
jMonkeyEngine:masterfrom
toaster0123:fix/geometry-morph-state-cloning-20261002
Open

toaster0123 wants to merge 2 commits into
jMonkeyEngine:masterfrom
toaster0123:fix/geometry-morph-state-cloning-20261002

Conversation

@toaster0123

Copy link
Copy Markdown
Contributor

Problem

Changing a cloned geometry's morph weights can also change the original geometry.

For example, set the original's smile weight to 0.2, clone it, then set the clone's smile to 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 morphState through the existing Cloner in Geometry.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

  • The same 24 permanent regression cases give 14 failures and 10 passing controls before the fix, then 24/24 passes afterward
  • Coverage includes named/array setters in both directions, multiple weights, clone-of-clone, getter-initialized and uninitialized state, dirty flags, mesh-sharing controls, and direct-Cloner graph-array identities in both traversal orders
  • An independent 82-case public-API matrix gives 44 failures plus 38 passing controls before, and 82/82 passes after. Those results also hold when composed with Keep global lights consistent when scene branches change #2997 and Initialize loaded scene transforms, lights and material overrides #3002
  • Core build plus desktop/effects/plugins checks pass: 581 reported tests, zero failures/errors, two existing/environment skips
  • The new regression file has zero Checkstyle warnings; Geometry has the same four existing warnings as baseline
  • Independent source/test review found no blocker

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.

@riccardobl
riccardobl marked this pull request as ready for review October 2, 2026 17:47

@jaime-jmebot jaime-jmebot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice and tight fix — exactly the right size for the problem.

  • Cloning morphState through the existing Cloner (next to cachedWorldMat) 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() and Node.clone(true), clone-of-clone, uninitialized state, and the dirty-flag behavior.
  • Tiny optional nit: geometryWithoutMorphTargetsStillClones clones first and only then calls setMorphState(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.

Copy link
Copy Markdown
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.

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