[FIXED] AnimComposer cloning bug

Hi all,
recently I’ve been playing with the new animation system and found a problem (more to come, I’m sure).
I think I already found the root cause so I just need you to confirm that I actually did.

The issue
AnimComposer has something called globalSpeed. When I export animated model to j3o and then load it again, this global speed stops working.

Some background
The composer uses layers internally to manage the progress of animation clips being played. These layers are instantiated from internal classes so they have access to their parent object (the composer). Some of you probably already know where this is heading.

Now the assumption (correct me if I’m wrong):
When the j3o file is loaded, all controls are cloned as well but the layer object keeps reference to the old AnimComposer. This is in line with my findings. When I debugged the app it turned out that when AnimComposer calls layer.advance(), the layer internally uses different AnimComposer object with different global speed.

Any thoughts?

3 Likes

Potential fix:
Recreate the layers while cloning AnimComposer?

If an old reference is hanging around then it means cloning is broken… that reference wasn’t cloned.

I’m not very familiar with the whole jme cloning magic but this reference is not directly maintained. It is a default java reference from inner class instance to it’s parent object. So as far as I know it can’t be manually updated.

EDIT:
That’s why the proposed fix just recreates the layers.

So, to me it sounds like the children aren’t being cloned and they should be. Or they are relying on state that they shouldn’t be.

If the layers need to be recreated then they weren’t cloned, eh? Else how are they still refering to the wrong “internal to Java parent”?

I’m just going from your description and not looking at the code. So some specific references might save the rest of us time.

Then maybe look at the code :slight_smile:
Saving your time actually means wasting mine. No offense.

But since we are all a bunch of nice guys I will try to be more specific.

The cloning issue is not a jME one it is a java issue. You can clone inner objects as much as you want but they will not have the parent reference updated. Bug ID: JDK-4206170 Cloning inner classes

That’s why I mentioned the jme cloning system which is supposed to get around these problems I guess, but in order to have correct inner objects after cloning the parent, one must recreate them. That’s actually the workaround mentioned in the bug report.

Yeah, except you were already looking at the code so presumably could save 5 seconds to avoid wasting 10 minutes of mine.

But fair. I will stop wasting my time on this. Good luck with your game.

Thanks for reporting, I made a patch to fix it:

2 Likes

I don’t think this will solve the issue. Although you probably found another bug :slight_smile:
I will try to to describe the problem with code as Paul suggested.

Simplified version:

public class AnimComposer extends AbstractControl {

    private Layer layer;
    private float globalSpeed = 1.0;

    public AnimComposer() {
        layer = new Layer()
    }

    public void setGlobalSpeed(float speed) {
        globalSpeed = speed;
    }

    public void update(float tpf) {
        layer.advance(tpf);
    }

    private class Layer {

        private float time = 0;
        private Action action;
    
        public void advance(float tpf) {
            time = tpf * action.speed * globalSpeed;
        }
    }
}

Problem
When you look at the advance() method of Layer class you can see that it uses globalSpeed which is defined in AnimComposer class. Java achieves that by storing reference to the enclosing instance of AnimComposer. This reference is stored when the Layer object is created in AnimComposer constructor and it cannot be changed.
When you load j3o file with AnimComposer, it will be cloned. The AnimComposer object will be cloned as well as the Layer object. But the reference to enclosing instance in the Layer object will not be updated.

Symptoms
Play a clip with AnimComposer restored from j3o file and change the global speed. The speed will change in the AnimComposer but it will not be reflected. Why? Because the Layer object which is stored in the correct AnimComposer object will use global speed from the original AnimComposer, not from the clone.

// simplified
Spatial model = .....
model.getControl(AnimComposer.class).setGlobalSpeed(2.0f)
export(model, "test.j30");
Spatial loaded =  assetManager.loadModel("test.j3o");
loaded.getControl(AnimComposer.class).setGlobalSpeed(0.5f)
loaded.getControl(AnimComposer.class).setCurrentAction("walk")

// Now the animation will play with global speed 2.0

// Although this will print 0.5
System.out.println(loaded.getControl(AnimComposer.class).getGlobalSpeed());

EDIT:
There are two possible solutions (maybe more, who knows?)

Solution 1
Recreate the layer objects after cloning.
This is the right solution in my opinion as it prevents similar mistakes in the future.

Solution 2
Pass the global speed as a parameter to the advance() method. This is much simpler but could result in similar issues in future.

1 Like

Solution 3: turn the inner class into a static inner class and make sure it has the state/access it needs on construction. Update cloning, etc. respectively.

Okay,

Currently Layer class looks like :

I modified it as below:

private static class Layer implements JmeCloneable {
        private AnimComposer ac;
        private Action currentAction;
        private AnimationMask mask;
        private float weight;
        private double time;

        public Layer(AnimComposer ac) {
            this.ac = ac;
        }
        
        public void advance(float tpf) {
            time += tpf * currentAction.getSpeed() * ac.globalSpeed;
            // make sure negative time is in [0, length] range
            if (time < 0) {
                double length = currentAction.getLength();
                time = (time % length + length) % length;
            }

        }

        @Override
        public Object jmeClone() {
            try {
                Layer clone = (Layer) super.clone();
                return clone;
            } catch (CloneNotSupportedException ex) {
                throw new AssertionError();
            }
        }

        @Override
        public void cloneFields(Cloner cloner, Object original) {
            ac = null;
            currentAction = null;
        }
    }

made it static and add a field for AnimComposer ac, and setting it in constructor.

and changed this line (which I am not certain if it is OK) :

to

newLayers.put(key, new Layer(this));

Any thought ? Does Layer class need to be Cloneable ?

I am not certain about AnimationMask :

only implementation is ArmatureMask which does not implement JmeCloneable.

2 Likes

If you are going to recreate them all the time then there was no reason to make the class static, eh?

One or the other solution, I think.

Probably Layers should be cloneable as they always were… in which case the change (as you have it) would have just been to make the class static, pass the parent AnimComposer and then make sure to clone that in cloneFields() (the thing Java isn’t doing automatically with a non-static inner class).

And I guess it’s only accessing the outer global speed or it could have been a static class all along.

2 Likes

Maybe I’m missing something, but if you create new layers in AnimComposer.cloneFields then Layer does not have to be JmeCloneable right?

Just clone the layer and then update the reference to AnimComposer via something like layer.setAnimComposer(ac);

1 Like

Okay guys, do you mean to do this ?

for (String key : layers.keySet()) {
            Layer layer = cloner.clone(layers.get(key));
            layer.ac = this;
            newLayers.put(key, layer);
}
1 Like

Yes. I think that should do it.

1 Like

No, I meant what I said. I mean, that’s fine… but there is already a cloneFields()… it’s for cloning the fields. if you clone the fields then they get cloned with the cloner which will clone the fields… which in this case will reuse the parent reference.

To be crystal clear, something like:

        @Override
        public void cloneFields(Cloner cloner, Object original) {
            ac = cloner.clone(ac);
            currentAction = null;
}
1 Like

Ah, I see now, thanks for providing the example.
Did not know that!

Ahaa, the cloner maintains an index of already cloned objects… Nice…

Okay, I updated my PR with the suggested changes

@pspeed would you please take a look ?

Edit:
There seems to be something going wrong with Travis openjdk11 build.

1 Like

Yep! Simplified a ton of ugly code. :slight_smile:

Quick glance it looks good to me.

3 Likes