Repository navigation
Keep blended animation targets and settings with the cloned model - #3004
toaster0123 wants to merge 2 commits into
Conversation
| import com.jme3.util.clone.Cloner; | ||
| import com.jme3.util.clone.JmeCloneable; | ||
|
|
||
| public class LinearBlendSpace implements BlendSpace, JmeCloneable { |
There was a problem hiding this comment.
Heads-up on a subtle behavior change worth calling out in the description: previously Cloner returned the same LinearBlendSpace instance for every clone, so anyone deliberately sharing one blend space across several models (a common trick to sync them) keeps working — but now each clone silently gets its own copy with its own value. That's the right behavior for this fix, just worth mentioning so it isn't a surprise for existing users.
There was a problem hiding this comment.
One more thing to double-check while we're here: cloneFields only rebinds action, and the shallow jmeClone() copies the steps array by reference. So the original and the copy end up sharing the same float[]. That's fine as long as steps is never mutated after construction, but if anything can write into it, the copy would silently corrupt the original — the exact class of bug this PR is fixing. If there's any mutator, dropping final on steps and adding steps = steps.clone(); in cloneFields would keep the state genuinely independent.
There was a problem hiding this comment.
Added an explicit compatibility note to the description: the default LinearBlendSpace now has its own value and owner after cloning. Code that wants synchronized blend values should update each copied action's blend space.
For the second point, this revision has a scalar float step, not a steps array: fields at lines 8–12. setBlendAction recomputes that scalar, and the shallow clone copies it by value. There is no array storage to share or clone here, so no production change is needed.
There was a problem hiding this comment.
You're right, and thanks for the correction — my note was based on a field shape that isn't in this revision. Here it's a plain float step, setBlendAction recomputes it from the actions' length, and the shallow clone copies it by value. Nothing to share, nothing to fix. My earlier "steps array" wording was simply mistaken; the approval stands as-is.
The compatibility note in the description covers the remaining behavior change nicely. 👍
jaime-jmebot
left a comment
There was a problem hiding this comment.
Great fix — this is exactly right, and the reasoning in the description holds up.
- Rebuilding the target map from the mappings that child-action cloning already established is the clean way to do it; it avoids duplicating the target-resolution logic inside
BlendAction. - Putting
LinearBlendSpaceon the sameClonergraph is the piece that actually matters for correctness. Mapping-only would have silenced the NPE while still letting the copy pick clips off the original's action and mutate the original'svalue— the x=5 vs x=7.5 example is a good, concrete demonstration of that.
A couple of small follow-ups in the inline thread, neither blocking:
- The
stepsarray is still shared by reference after cloning (see my note onLinearBlendSpace). Fine today, but worth guarding if it ever becomes mutable. - Separately, blendspaces are no longer shared between original and copy. That's the desired outcome here, just a small behavior change worth a line in the description for anyone who was relying on sharing one space across models.
The test coverage looks thorough — especially the interleaved original/copy updates and the explicit-Cloner-policy cases, which are the ones most likely to regress this later. Approved.
jaime-jmebot
left a comment
There was a problem hiding this comment.
I checked the file and confirmed the author is correct — LinearBlendSpace stores a scalar float step, recomputed in setBlendAction, and copied by value on clone. There is no shared array to defensively clone, so my earlier follow-up note was mistaken. I replied in that inline thread acknowledging the correction and confirmed the approval stands; no code changes needed.
Problem
A model cloned after registering an
AnimComposer.actionBlendedaction can throw aNullPointerExceptionwhen that action is selected and played on the copy.The child clip actions correctly point to the copied model, but the enclosing BlendAction still looks up transforms using the original model's targets. Its default LinearBlendSpace also retains the original action as its owner. Fixing only the target map avoids the exception but can still choose the wrong clips or produce the wrong blend.
For example, halfway through two clips moving to x=10 and x=20, a 50/50 blend should produce x=7.5. The map-only correction produces x=5. Changing the copied blend-space value can also change the original's setting.
Fix
Custom actions that intentionally retain a target keep that target. Custom blend spaces without supported cloning retain their existing sharing behavior and are not rebound to the copy. No interface requirement or interpolation algorithm is changed.
Compatibility note: cloning now gives the default LinearBlendSpace its own value and action owner. Changing the original space's value no longer updates model copies. Applications that want synchronized blend values should update each copied action's blend space.
Validation
The permanent tests use zero-duration transitions to isolate this defect. Composition with exact #2992 passes 28 cases, including its eight unchanged tests and three additional zero/nonzero-transition cases. The separate transition-owner bug remains without #2992; its code is not bundled here.
Limits
This validates CPU animation targets, transforms, and clone-state ownership, not rendered pixels or GPU behavior. Unsupported custom blend spaces are not promised automatic independent state.
This branch is based directly on upstream
master. The test skips are the existing FastMath counterclockwise case and a desktop noexec-filesystem assumption. The local whole-repository Android build remains outside these checks because the SDK is unavailable.