Skip to content

Keep blended animation targets and settings with the cloned model - #3004

Open
toaster0123 wants to merge 2 commits into
jMonkeyEngine:masterfrom
toaster0123:fix/blended-action-cloning-20261002
Open

toaster0123 wants to merge 2 commits into
jMonkeyEngine:masterfrom
toaster0123:fix/blended-action-cloning-20261002

Conversation

@toaster0123

@toaster0123 toaster0123 commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Problem

A model cloned after registering an AnimComposer.actionBlended action can throw a NullPointerException when 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

  • Rebuild the target map using target mappings already established by child-action cloning
  • Copy the mutable transform accumulators and speed-factor array
  • Make LinearBlendSpace participate in the same Cloner graph, preserving its copied action owner
  • Honor explicit space mappings and registered clone functions

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 same 17 permanent public-API cases go from 12 failures and 5 passing controls on upstream to 17/17 passes with the fix
  • Coverage includes copied target identity, before/during playback, interleaved original/copy updates, two/three-clip selection, independent default-space values and speed factors, custom shared targets/spaces, and explicit Cloner policies
  • A map-only mutation still fails 10 cases, so the space-owner correction is independently exercised
  • Core build and desktop/effects/plugins checks pass: 574 reported tests, zero failures/errors, two existing/environment skips
  • New-test Checkstyle is clean; no new production diagnostics
  • Independent source review and standalone/composed fixture replay found no blocker

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.

import com.jme3.util.clone.Cloner;
import com.jme3.util.clone.JmeCloneable;

public class LinearBlendSpace implements BlendSpace, JmeCloneable {

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.

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.

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.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

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.

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 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.

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 LinearBlendSpace on the same Cloner graph 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's value — 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 steps array is still shared by reference after cloning (see my note on LinearBlendSpace). 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 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.

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.

@riccardobl
riccardobl marked this pull request as ready for review October 2, 2026 21:12
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