General discussion about AnimationMask

[capdevon]

Hi everyone,
I’d like to share some ideas with you. I thought about the discussion about the ‘ArmatureMask’ class based on feedback from @sgold and @pspeed.

Class suggestion for new AnimSystem: BoneChannel

The thing that especially bugged me about this part is that if you pass a totally foreign armature to these methods then it will return nonsense results. On the other hand, I understand the desire not to have a generally redundant reference inside of ArmatureMask… especially since there really are cases where the same AnimationMask instance can be used for many different Armature instances. (ie: shared masks across multiple instances of the same model).

Here are two proposals:

---

1. Version A (implements AnimationMask)

AnimMaskBuilder.java

• Method signature is consistent.
• You can chain all methods.
• The ‘Armature’ is passed only once in the constructor.
• ‘Joints’ are passed to methods as strings instead of objects.
• Redundant reference of Armature inside of AnimationMask.

import java.util.ArrayList;
import java.util.BitSet;
import java.util.List;
import java.util.logging.Level;
import java.util.logging.Logger;

import com.jme3.anim.AnimationMask;
import com.jme3.anim.Armature;
import com.jme3.anim.Joint;

/**
 * An AnimationMask to select joints from a single Armature.
 * @author capdevon
 */
public class AnimMaskBuilder implements AnimationMask {

    private static final Logger logger = Logger.getLogger(AnimMaskBuilder.class.getName());

    private final BitSet affectedJoints;
    private final Armature armature;

    /**
     * Instantiate a mask that affects no joints.
     *
     * @param armature
     */
    public AnimMaskBuilder(Armature armature) {
        this.armature = armature;
        this.affectedJoints = new BitSet(armature.getJointCount());
        logger.log(Level.INFO, "Joint count: {0}", armature.getJointCount());
    }

    /**
     * Add all the bones of the model's armature to be influenced by this
     * animation mask.
     *
     * @return AnimMaskBuilder
     */
    public AnimMaskBuilder addAllJoints() {
        int numJoints = armature.getJointCount();
        affectedJoints.set(0, numJoints);
        return this;
    }

    /**
     * Add joints to be influenced by this animation mask.
     *
     * @param jointNames
     * @return AnimMaskBuilder
     */
    public AnimMaskBuilder addJoints(String...jointNames) {
        for (String jointName: jointNames) {
            Joint joint = findJoint(jointName);
            affectedJoints.set(joint.getId());
        }
        return this;
    }

    private Joint findJoint(String jointName) {
        Joint joint = armature.getJoint(jointName);
        if (joint == null) {
            throw new IllegalArgumentException("Cannot find joint " + jointName);
        }
        return joint;
    }

    /**
     * Add a joint and all its sub armature joints to be influenced by this
     * animation mask.
     *
     * @param jointName the starting point (may be null, unaffected)
     * @return AnimMaskBuilder
     */
    public AnimMaskBuilder addFromJoint(String jointName) {
        Joint joint = findJoint(jointName);
        addFromJoint(joint);
        return this;
    }

    private void addFromJoint(Joint joint) {
        affectedJoints.set(joint.getId());
        for (Joint j: joint.getChildren()) {
            addFromJoint(j);
        }
    }

    /**
     * Remove a joint and all its sub armature joints to be influenced by this
     * animation mask.
     *
     * @param jointName the starting point (may be null, unaffected)
     * @return AnimMaskBuilder
     */
    public AnimMaskBuilder removeFromJoint(String jointName) {
        Joint joint = findJoint(jointName);
        removeFromJoint(joint);
        return this;
    }

    private void removeFromJoint(Joint joint) {
        affectedJoints.clear(joint.getId());
        for (Joint j: joint.getChildren()) {
            removeFromJoint(j);
        }
    }

    /**
     * Add the specified Joint and all its ancestors.
     *
     * @param jointName the starting point (may be null, unaffected)
     * @return AnimMaskBuilder
     */
    public AnimMaskBuilder addAncestors(String jointName) {
        Joint joint = findJoint(jointName);
        addAncestors(joint);
        return this;
    }

    private void addAncestors(Joint start) {
        for (Joint joint = start; joint != null; joint = joint.getParent()) {
            affectedJoints.set(joint.getId());
        }
    }

    /**
     * Remove the specified Joint and all its ancestors.
     *
     * @param jointName the starting point (may be null, unaffected)
     * @return AnimMaskBuilder
     */
    public AnimMaskBuilder removeAncestors(String jointName) {
        Joint joint = findJoint(jointName);
        removeAncestors(joint);
        return this;
    }

    private void removeAncestors(Joint start) {
        for (Joint joint = start; joint != null; joint = joint.getParent()) {
            affectedJoints.clear(joint.getId());
        }
    }

    /**
     * Remove the named joints.
     *
     * @param jointNames the names of the joints to be removed
     * @return AnimMaskBuilder
     */
    public AnimMaskBuilder removeJoints(String...jointNames) {
        for (String jointName: jointNames) {
            Joint joint = findJoint(jointName);
            affectedJoints.clear(joint.getId());
        }

        return this;
    }
    
    /**
     * Get the list of joints affected by this animation mask.
     *
     * @return
     */
    public List<Joint> getAffectedJoints() {
        List<Joint> lst = new ArrayList<>();
        for (Joint joint : armature.getJointList()) {
            if (contains(joint)) {
                lst.add(joint);
            }
        }
        return lst;
    }

    @Override
    public boolean contains(Object target) {
        Joint joint = (Joint) target;
        return affectedJoints.get(joint.getId());
    }

}

Use case:

Armature armature = skinningControl.getArmature();

// upperBody
AnimationMask upperBody = new AnimMaskBuilder(armature)
        .addFromJoint("Spine");

// lowerBody
AnimationMask lowerBody = new AnimMaskBuilder(armature)
        .addJoints("Hips")
        .addFromJoint("RightUpLeg")
        .addFromJoint("LeftUpLeg");

// allBody
AnimationMask allBody = new AnimMaskBuilder(armature)
        .addAllJoints();

// noHeadBody
AnimationMask noHeadBody = new AnimMaskBuilder(armature)
        .addAllJoints()
        .removeJoints("Head", "HeadTop_End", "RightEye", "LeftEye");

// noHeadBody2
AnimationMask noHeadBody2 = new AnimMaskBuilder(armature)
        .addAllJoints()
        .removeFromJoint("Neck");

---

2. Version B (use Builder Pattern):

AnimMaskBuilder.java:

• Responsible for the construction of the mask.
• Method signature is consistent.
• You can chain all methods.
• The ‘Armature’ is passed only once in the constructor.
• ‘Joints’ are passed to methods as strings instead of objects.

import java.util.ArrayList;
import java.util.BitSet;
import java.util.List;
import java.util.logging.Level;
import java.util.logging.Logger;

import com.jme3.anim.AnimationMask;
import com.jme3.anim.Armature;
import com.jme3.anim.Joint;

/**
 * An AnimationMask to select joints from a single Armature.
 * @author capdevon
 */
public class AnimMaskBuilder {

    private static final Logger logger = Logger.getLogger(AnimMaskBuilder.class.getName());

    private final BitSet affectedJoints;
    private final Armature armature;

    /**
     * Instantiate a mask that affects no joints.
     *
     * @param armature
     */
    public AnimMaskBuilder(Armature armature) {
        this.armature = armature;
        this.affectedJoints = new BitSet(armature.getJointCount());
        logger.log(Level.INFO, "Joint count: {0}", armature.getJointCount());
    }

    /**
     * Add all the bones of the model's armature to be influenced by this
     * animation mask.
     *
     * @return AnimMaskBuilder
     */
    public AnimMaskBuilder addAllJoints() {
        int numJoints = armature.getJointCount();
        affectedJoints.set(0, numJoints);
        return this;
    }

    /**
     * Add joints to be influenced by this animation mask.
     *
     * @param jointNames
     * @return AnimMaskBuilder
     */
    public AnimMaskBuilder addJoints(String...jointNames) {
        for (String jointName: jointNames) {
            Joint joint = findJoint(jointName);
            affectedJoints.set(joint.getId());
        }
        return this;
    }

    private Joint findJoint(String jointName) {
        Joint joint = armature.getJoint(jointName);
        if (joint == null) {
            throw new IllegalArgumentException("Cannot find joint " + jointName);
        }
        return joint;
    }

    /**
     * Add a joint and all its sub armature joints to be influenced by this
     * animation mask.
     *
     * @param jointName the starting point (may be null, unaffected)
     * @return AnimMaskBuilder
     */
    public AnimMaskBuilder addFromJoint(String jointName) {
        Joint joint = findJoint(jointName);
        addFromJoint(joint);
        return this;
    }

    private void addFromJoint(Joint joint) {
        affectedJoints.set(joint.getId());
        for (Joint j: joint.getChildren()) {
            addFromJoint(j);
        }
    }

    /**
     * Remove a joint and all its sub armature joints to be influenced by this
     * animation mask.
     *
     * @param jointName the starting point (may be null, unaffected)
     * @return AnimMaskBuilder
     */
    public AnimMaskBuilder removeFromJoint(String jointName) {
        Joint joint = findJoint(jointName);
        removeFromJoint(joint);
        return this;
    }

    private void removeFromJoint(Joint joint) {
        affectedJoints.clear(joint.getId());
        for (Joint j: joint.getChildren()) {
            removeFromJoint(j);
        }
    }

    /**
     * Add the specified Joint and all its ancestors.
     *
     * @param jointName the starting point (may be null, unaffected)
     * @return AnimMaskBuilder
     */
    public AnimMaskBuilder addAncestors(String jointName) {
        Joint joint = findJoint(jointName);
        addAncestors(joint);
        return this;
    }

    private void addAncestors(Joint start) {
        for (Joint joint = start; joint != null; joint = joint.getParent()) {
            affectedJoints.set(joint.getId());
        }
    }

    /**
     * Remove the specified Joint and all its ancestors.
     *
     * @param jointName the starting point (may be null, unaffected)
     * @return AnimMaskBuilder
     */
    public AnimMaskBuilder removeAncestors(String jointName) {
        Joint joint = findJoint(jointName);
        removeAncestors(joint);
        return this;
    }

    private void removeAncestors(Joint start) {
        for (Joint joint = start; joint != null; joint = joint.getParent()) {
            affectedJoints.clear(joint.getId());
        }
    }

    /**
     * Remove the named joints.
     *
     * @param jointNames the names of the joints to be removed
     * @return AnimMaskBuilder
     */
    public AnimMaskBuilder removeJoints(String...jointNames) {
        for (String jointName: jointNames) {
            Joint joint = findJoint(jointName);
            affectedJoints.clear(joint.getId());
        }

        return this;
    }
    
    /**
     * Get the list of joints affected by this animation mask.
     *
     * @return
     */
    public List<Joint> getAffectedJoints() {
        List<Joint> lst = new ArrayList<>();
        for (Joint joint : armature.getJointList()) {
        	if (affectedJoints.get(joint.getId())) {
                lst.add(joint);
            }
        }
        return lst;
    }
    
	/**
	 * (NEW)
	 * Build AnimationMask
	 * 
	 * @return
	 */
	public AnimationMask build() {
		AvatarMask mask = new AvatarMask();
		for (int i = 0; i < affectedJoints.length(); i++) {

			if (affectedJoints.get(i) == true) {
				Joint joint = armature.getJoint(i);
				mask.addJoint(joint.getId());
			}
		}
		return mask;
	}

}

AvatarMask.java:

• Simple implementation of the mask containing only the necessary data and a minimal set of functions (add/remove).
• AnimationMask instance can be used for many different Armature instances. (ie: shared masks across multiple instances of the same model).

import java.util.BitSet;

import com.jme3.anim.AnimationMask;
import com.jme3.anim.Joint;

/**
 * 
 * @author capdevon
 */
public class AvatarMask implements AnimationMask {

    private final BitSet affectedJoints = new BitSet();

    /**
     * Instantiate a mask that affects no joints.
     */
    public AvatarMask() {
        // do nothing
    }

    public void addJoint(int jointId) {
        affectedJoints.set(jointId);
    }

    public void removeJoint(int jointId) {
        affectedJoints.clear(jointId);
    }

    @Override
    public boolean contains(Object target) {
        Joint joint = (Joint) target;
        return affectedJoints.get(joint.getId());
    }

}

Use case:

Armature armature = skinningControl.getArmature();

// upperBody
AnimationMask upperBody = new AnimMaskBuilder(skinningControl.getArmature())
		.addFromJoint("Spine")
		.build(); /* build mask */

// lowerBody
AnimationMask lowerBody = new AnimMaskBuilder(skinningControl.getArmature())
		.addJoints("Hips")
		.addFromJoint("RightUpLeg")
		.addFromJoint("LeftUpLeg")
		.build(); /* build mask */

// allBody
AnimationMask allBody = new AnimMaskBuilder(skinningControl.getArmature())
		.addAllJoints()
		.build(); /* build mask */

// noHeadBody
AnimationMask noHeadBody = new AnimMaskBuilder(skinningControl.getArmature())
		.addAllJoints()
		.removeJoints("Head", "HeadTop_End", "RightEye", "LeftEye")
		.build(); /* build mask */

// noHeadBody 2
AnimationMask noHeadBody2 = new AnimMaskBuilder(skinningControl.getArmature())
		.addAllJoints()
		.removeFromJoint("Neck")
		.build(); /* build mask */

Do you think it might be useful to integrate one of the two proposed solutions into the core library by deprecating the current implementation of the ArmatureMask class? Otherwise no problem, it remains as an example for posterity. The thing I like most is hearing your opinion.

[oxplay2]

Hard to tell now, since i had long break from animations now.

Both versions looks fine for me, tho i would like to be able have single Mask instance that i can use in multiple armatures.

[pspeed]

@oxplay2

tho i would like to be able have single Mask instance that i can use in multiple armatures.

Indeed it may be a requirement unless someone does some extensive deep dive to make sure cloning, etc. is not broken by tying an AnimationMask directly to a specific Armature instance.

@capdevon

I’d like to share some ideas with you. I thought about the discussion about the ‘ArmatureMask’ class based on feedback from @sgold and @pspeed.

The thing that especially bugged me about this part is that if you pass a totally foreign armature to these methods then it will return nonsense results. On the other hand, I understand the desire not to have a generally redundant reference inside of ArmatureMask… especially since there really are cases where the same AnimationMask instance can be used for many different Armature instances. (ie: shared masks across multiple instances of the same model).

Note: to clarify my quoted paragraph, I was frustrated enough to look into it and then saw that there was a very valid reasons for it.

I have a few problems with Version A. Right off it rubs me wrong that it’s called AnimationMaskBuilder but is actually the mask. It’s a small thing and easily fixed. The biggest problem I have with it is that I actually like that ArmatureMask is not directly tied to a specific Armature instance. After having looked into that, I see what it is that way and agree with that original design decision.

…a big assumption is that an Armature instance is not already shared across models. If it’s not then maybe there is something lower that could/should be shared and could/should be what ArmatureMask keeps.

Perhaps a particular configuration of joints is already shared and my concerns are invalid.

Also, if one of the primary motivations for all of this is that ArmatureMask doesn’t return ArmatureMask from some of its methods then that’s ultimately fixable.

It also seems like the real builder version (approach 2) could have continued to use the regular ArmatureMask, no?

[capdevon]

@pspeed

It also seems like the real builder version (approach 2) could have continued to use the regular ArmatureMask, no?

Technically yes, but I didn’t like the idea of ​​having to invoke the only method called addBones when the whole ArmatureMask class refers to Joint.

Edit:
furthermore, the addBones() method performs the same functions as the addJoints() method of the Builder class (essentially double-checking that the joint belongs to the ‘Armature’). Also for this reason I designed the AvatarMask class, which only receives the joint id as input and immediately adds it to the BitSet.

It would look like this:

	public AnimationMask buildMask() {
		ArmatureMask mask = new ArmatureMask();
		for (int i = 0; i < affectedJoints.length(); i++) {

			if (affectedJoints.get(i) == true) {
				String jointName = armature.getJoint(i).getName();
				mask.addBones(armature, jointName);
			}
		}
		return mask;
	}

I created the AvatarMask class because I liked the idea of ​​keeping the mask creation features (AnimMaskBuilder) separate from the mask logic (AvatarMask) for better code maintenance.

	public AnimationMask build() {
		AvatarMask mask = new AvatarMask();
		for (int i = 0; i < affectedJoints.length(); i++) {

			if (affectedJoints.get(i) == true) {
				Joint joint = armature.getJoint(i);
				mask.addJoint(joint.getId());
			}
		}
		return mask;
	}

---

UML of approach B compared to the ArmatureMask class currently present in the core.

---

UML of approach A compared to the ArmatureMask class currently present in the core.

@pspeed

I have a few problems with Version A. Right off it rubs me wrong that it’s called AnimationMaskBuilder but is actually the mask. It’s a small thing and easily fixed.

I agree, it could just be called AnimMask or something else.

[Ali_RS]

@capdevon

only method called addBones when the whole ArmatureMask class refers to Joint.

Have not read the whole post, but yeah, I think it was named by mistake, IMO we should deprecate that method and add a new one with the correct naming (“addJoints”).

[pspeed]

Tangentially related but I discovered pretty HUGE state leaking issues that essentially prevent the animation code from running on different threads… and I don’t mean can’t use Model A and two different threads, I mean can’t use model A loaded from one AssetManager and model B loaded from a completely different asset manager.

…random oddly morphed animation at the best of times and actual index of out of bounds exceptions at the worst times.

I’m still trying to think of the best least impactful way to fix it but there are bound to be some motivations for breaking changes. ie: maybe we can group some breaking changes together (like adding return types to some of the methods that don’t have them, etc.)