Some enhancement to new animation system

[Ali-RS]

Including:

• Option to enable/disable animation mask propagation to child actions.
• Option to control max transition weight. For example useful for controlling smooth animation transition when an animation is removed from an upper layer.
• Added animation loop support in AnimLayer.
• AnimLayer can now also keep the action name, so one can easily look up the currently playing action name in a specific layer.

[capdevon]

Hi Ali,
a little tip: could you remove the currentActionName field from the AnimLayer class and add the name field to the Action class? After all when we create an Action we give it a name, so it's an attribute that should be in the Action class.
In this way, redundancies are avoided. It is more natural to write the code like this:

String actionName = animComposer.getCurrentAction().getName();

//or
String actionName = animComposer.getCurrentAction(layerName).getName();

//Most common use case:
...
public void playAnimation(String actionName) {
    if ( !actionName.equals(animComposer.getCurrentAction().getName()) ) {
        animComposer.setCurrentAction(actionName);
    }
}

What do you think?

[pspeed42]

I don't have time to look at the code at the moment but my vague recollection is that the same action instance can be under multiple names... or that actions are not always associated with names or something.

I just remember looking at this before and somehow coming to the same conclusion as Ali.

[Ali-RS]

Hmm, I do not remember why I didn't put the name on Action. 🤔 I made yet another look and it sounds fine to give a name to the action. We are already associating a name for each action we create with AnimComposer anyway.

is that the same action instance can be under multiple names...

My brain can not think of a use case for this right now. Can you probably give an example of this case?

[pspeed42]

I'd have to look at the code or go through my own code again. I just know I once looked at making the change on action and decided against it.

Is there a way to add already built actions and give them a name? How do we add blendable actions and stuff? If no one knows, I will look again and it's definitely possible I'm wrong.

[Ali-RS]

Is there a way to add already built actions and give them a name?

Yes,

jmonkeyengine/jme3-core/src/main/java/com/jme3/anim/AnimComposer.java

Line 269 in 591eb36

public void addAction(String name, Action action) {

How do we add blendable actions and stuff?

jmonkeyengine/jme3-core/src/main/java/com/jme3/anim/AnimComposer.java

Lines 353 to 362 in 591eb36

	public BlendAction actionBlended(String name, BlendSpace blendSpace, String... clips) {
	BlendableAction[] acts = new BlendableAction[clips.length];
	for (int i = 0; i < acts.length; i++) {
	BlendableAction ba = (BlendableAction) makeAction(clips[i]);
	acts[i] = ba;
	}
	BlendAction action = new BlendAction(blendSpace, acts);
	actions.put(name, action);
	return action;
	}

JME actions are either ClipAction which is corresponded to its clip name in AnimComposer or BlendAction which is built from multiple ClipAction for example "WalkRunBlend" which is made by blending "Walk" and "Run" actions. The rest of the things are made up of standard tweens that we finally wrap inside BaseAction (the spaghetti class;) and register it to the AnimComposer with a name using addAction() method.

[Ali-RS]

Went ahead and added a name field to Action.

[capdevon]

Hi guys,
I'm studying the new functions, probably the Action.setName() method should be protected or removed, because actions when registered in the AnimComposer class, are stored in a map.

By leaving the method public, someone could change the name of the Action after registering it, thus creating an inconsistency in the data stored in the map and causing unwanted effects.

Is the hypothesis correct or am I wrong? What do you think?

[Ali-RS]

Make sense. Maybe it should be passed via constructor only.

@pspeed42 what do you think?

[pspeed42]

I guess it's theoretically possible that some implementation wouldn't know at construction time and want to set it after being fully initialized... so a protected setter is nice.

[Ali-RS]

Hmm, If we make it protected, AnimComposer won't be able to set it when it creates one because they are in different packages, so I believe a pass-with-constructor is also required.

[Ali-RS]

Updated the code. @capdevon thanks for reviewing.

[capdevon]

Happy to help. However, seeing the result I start to convince myself that you were right and adding the name field to the Action class is not a good idea, although it is easy to think that it should be there.

For example changing the visibility of the Action.setName() method to protected is useless, having added the name field to the constructors.

The new AnimComposer.addAction(Action) method seems unnecessary and could introduce errors and confusion.
All the methods currently present for adding an action to the AnimComposer require you to always specify the name of the action. See:

• AnimComposer.action(String name)
• AnimComposer.makeAction(String name)
• AnimComposer.addAction(String name, Action action)
• AnimComposer.actionSequence(String name, Tween... tweens)
• AnimComposer.actionBlended(String name, BlendSpace blendSpace, String... clips)

Look at this example:

        String animName = "walk";

        // Get action registered with specified name. It will make a new action if there isn't any.
        Action action = animComposer.action(animName);
        Tween callback = Tweens.callMethod(this, "animCycleDone", animName);

        // I create a BaseAction without name.
        // I can't name it because the setter method is not visible.
        action = new BaseAction(Tweens.sequence(action, callback));

        //I try to add the action on the AnimComposer and I trigger the exception. This solution is confusing.
        animComposer.addAction(action); // <- new method added with this PR

Sorry to waste your time, but the initial solution with the currentActionName field in the AnimLayer class, and Action classes without constructors and getter / setter for the name field, seems better to me.

With the current code, running an animation with loop = false, the AnimLayer resets the variable currentAction = null at the end of the animation, but the animation is still persistent on screen. Querying the AnimLayer.getCurrentAction() method I will get an inaccurate information.

Making a comparison with the old AnimChannel class, perhaps you were right to add the currentActionName field to the AnimLayer class with just the getter method. In this way we would have all the information we need about the last animation performed on the Layer.

[pspeed42]

re: "However, seeing the result I start to convince myself that you were right and adding the name field to the Action class is not a good idea, although it is easy to think that it should be there."

Agreed. I couldn't remember all of the reasons I didn't like if before and I'm sorry you all had to rediscover them.

[Ali-RS]

No problem. I will revert the namig related changes.

[MeFisto94]

Actually reading that reminds me of the problem I had as well. The Animation System is a bit indecided between string keys and action instances iirc. In theory that deserves fixing one way or another in a long run I guess. That was one of my problems with it

[Ali-RS]

Ok, dropped the last commits related to the name field on Action. Also updated AnimLayer.cloneFields method to take 'currentActionName' field into account.

[Ali-RS]

With the current code, running an animation with loop = false, the AnimLayer resets the variable currentAction = null at the end of the animation, but the animation is still persistent on screen. Querying the AnimLayer.getCurrentAction() method I will get an inaccurate information.

Can you please clarify what you mean by "but the animation is still persistent on screen"?
If loop is set to false, AnimLayer will clear the current action when the action is done and will set it to null using setCurrentAction(null). Querying the AnimLayer.getCurrentAction() afterward must return null. The character will keep the pose it had when the action is removed until a new animation is added or overridden by another layer.

[capdevon]

Can you please clarify what you mean by "but the animation is still persistent on screen"?

I meant this, but you wrote it better ;)

The character will keep the pose it had when the action is removed until a new animation is added or overridden by another layer.

--

If loop is set to false, AnimLayer will clear the current action when the action is done and will set it to null using setCurrentAction(null). Querying the AnimLayer.getCurrentAction() afterward must return null.

Correct. This aspect justifies the presence of a new currentActionName field, to keep track of the last Action performed on the AnimLayer.

--

I would like to reflect a little more on some scenarios that could occur :)

[Ali-RS]

Correct. This aspect justifies the presence of a new currentActionName field, to keep track of the last Action performed on the AnimLayer.

Note that the currentActionName field is also cleared when currentAction is cleared. I do not understand why it should keep the action name while the action is not there anymore. If an app requires to know the previous action name then it should record it somewhere.

[capdevon]

Ok, it looks good to me, it's always a pleasure to cooperate with you. It seems to me that all the new features are backwards compatible, making the new animation system more intuitive. Well done.

[Ali-RS]

Thank you so much @capdevon

Unless there's substantial discussion, I plan to integrate this in about 24 hours.