hi everyone, I’d like to know your point of view on some aspects of the new animation system, in particular on the AnimEvent class.
A couple of hints before we get started, useful for the latest beta testing version of the engine.
A small grammar error in the class description “An Cinematic Event …”
Checking if the action is null is redundant as the composer.action method never returns null and raises exception if the clip name does not exist.
I am upgrading my prototypes from the old to the new cinematic system.
3. Would it be possible to add other constructors with more flexibility like the old AnimationEvent class?
Something like this:
cinematic.addCinematicEvent(2f, new AnimEvent(door.getControl(AnimComposer.class), "Open", AnimComposer.DEFAULT_LAYER)); //as-is
cinematic.addCinematicEvent(2f, new AnimEvent(door, "Open", AnimComposer.DEFAULT_LAYER, LoopMode.Loop)); //new
cinematic.addCinematicEvent(2f, new AnimEvent(door, "Open", AnimComposer.DEFAULT_LAYER)); //new
cinematic.addCinematicEvent(2f, new AnimEvent(door, "Open")); // new
cinematic.addCinematicEvent(2f, new AnimEvent(door, "Open", LoopMode.Loop)); //new
Please open a new issue (or pull request) for the additional constructors.
Regarding the latest suggestion: as written, it only works if the AnimComposer is added to the model’s root spatial. In many models, it’s added to a child spatial.
what you say is true, but IMO a more compact constructor would greatly reduce the code needed to create an AnimEvent making it easier to use.
With the current constructor you need to write more code than before. This may discourage their use.
Here is an example:
before:
cinematic.activateCamera(0, cineCam.getName());
cinematic.addCinematicEvent(0, new SoundEvent("Sound/Environment/Nature.ogg", LoopMode.Loop));
cinematic.addCinematicEvent(0, new SoundEvent("Sound/Effects/kick.wav"));
cinematic.addCinematicEvent(0, new AnimationEvent(player.getChild("Armature"), "Rain_Dance", LoopMode.Loop));
after: (too much code)
AnimComposer ac = player.getChild("Armature").getControl(AnimComposer.class);
AnimEvent event = new AnimEvent(ac, "Rain_Dance", AnimComposer.DEFAULT_LAYER);
event.setLoopMode(LoopMode.Loop);
cinematic.addCinematicEvent(0, event);
cinematic.activateCamera(0, cineCam.getName());
cinematic.addCinematicEvent(0, new SoundEvent("Sound/Environment/Nature.ogg", LoopMode.Loop));
cinematic.addCinematicEvent(0, new SoundEvent("Sound/Effects/kick.wav"));
As the class has just been created, I was hoping that an addition could be made at the last moment. If it’s too late, that’s okay. It will be for the next release. You have already done an excellent job
Yes, there are many more that need to be refractored, as :
No Usages for setValue(value:Float) here :
The global instance named translations of type CompactVector3Array() must be defined in the constructor of TransformTrack & only the translations.add(translations) would take place inside the method setTranslations(), because if i haven’t settled some Translations tracks for this TransformationTrack then call getTranslations() i got a NPE, or at least make the getTranslations() returns CompactVector3Array() & not Vector3f[] which will give only null & not a crash…
Because, they aren’t used in any other code scopes, & also naming of convention of BlendSpace interface field methods seems a little bit weird, like having getters without setters or object definitions & more above that “inside an interface”, I am lost inside the code & I think anyone who will try to contribute or fix bugs, he will get lost too.
I think if we care more about the inside code design, we had better not to return null directly, is it better to just return an instance of CompactArray, & the user could use getCompactIndex(i) to fetch by index ?
Obviously it’s fine to have getters without setters. Some getters are read-only. Some are calculated. And so on.
On interfaces, it’s also fine to have setters with no getters if nothing about the API requires getting the value back. Once you add the getter to the interface then you force ALL implementations to implement that method even if it doesn’t make sense. Maybe there is some implementation that takes a value in and would find it complicated to give it back… why should it then store the value just to return it?
Missing on the interface doesn’t mean the implementation can’t implement a getter. It just means that it’s not required to.
I think all of these methods need javadoc and that would clear a lot of things up. In the mean time, the fact that one thing has only a setter and another has only a getter is a very valuable clue.
Don’t erase the clues before the API has been documented unless there is a legitimate reason.
I suspect the design intention was to hide CompactArray from the application programmer. Hiding such implementation details allows future re-implementation without affecting the API. I recommend keeping them hidden.
To provide a concrete basis for further discussion, I’ve gone ahead and submitted PRs 1531 and 1532.
Oh, i haven’t noticed that blendSpace is implemented by another class, i was implementing the BlendSpace into my state class, passing it into the BlendAction constructor & using the getWeight directly, not sure if it was the right use…
Yeh i supposed there’s no a call for setValue(value:Float) inside the BlendableAction API (ie : there’s no blendSpace.getValue() or blendSpace.value), but now i see that LinearBlendSpace is using setValue() & using var value inside getWeight() which in turn get called inside the BlendableAction,
but if you remove setValue from the interface BlendSpace, nothing would be changed, & the API still wouldn’t be affected, that is what i am trying to say…, except for the TestAnimMigration that would get broken at action.getBlendSpace().setValue(x) , but you still can use a global variable of LinearBlendSpace like linearBlendSpace.setValue(x);
At the end, it is just an opinion (not argument at all) about code design, so normal users like me won’t get lost, …that change that i have mentioned would render no change in code functionality, hopefully if there’s something else that i donot know about, but anyway if that’s documented properly, right there shouldn’t be a change…
EDIT : There may be a blueprint design, that illustrates why the author is doing this, may be having a BlendSpace & BlendSpace properties (fields of interface) for each single blendAction that controls the interpolation values & those controls get all enclosed inside an Entity named BlendSpace interface
The whole point of a blend space is to be able to set a value and have it blend. It may be a linear space, it may be a cubic space, spherical space… whatever. Application code and/or other actions will be inputting some “value” to blend.
Having a setValue() method is then a good indication of the intent of the interface. And your confusion is then deeper than “Huh, why is this method here?”
Do not confuse a badly documented API for a badly designed one. And if you don’t understand the use for some method then you may be in a poor position to decide which is which.
Yeh, you were right, i started to understand when i have seen the LinearBlendSpace, as i mentioned above, at first i was implementing the BlendSpace inside my class, so i was seeing no use for setValue().
hi everyone, I am trying to complete the transition to the new animation system in some of my projects. Could you tell me how to convert these instructions? Thanks for your help.
Case A: assuming animComposer is running a looped animation
private SkeletonControl skeletonControl;
@Deprecated
private void resetBoneTransform() {
int boneCount = skeletonControl.getSkeleton().getBoneCount();
for (int i = 0; i < boneCount; i++) {
Bone bone = skeletonControl.getSkeleton().getBone(i);
if (bone.hasUserControl()) {
bone.setUserControl(false);
System.out.println("Reset User Control: " + bone.getName());
}
}
}
private SkinningControl skinningControl;
private void resetBoneTransform() {
int boneCount = skinningControl.getArmature().getJointCount();
for (int i = 0; i < boneCount; i++) {
Joint bone = skinningControl.getArmature().getJoint(i);
/* ???
if (bone.hasUserControl()) {
bone.setUserControl(false);
System.out.println("Reset User Control: " + bone.getName());
}
*/
}
}
Case B: assuming animComposer is running a looped animation
private AnimComposer animComposer;
private Camera camera;
private boolean isAiming;
private Bone spineBone;
private Quaternion tempRotation = new Quaternion();
private float[] angles = new float[3];
...
@Override
protected void controlUpdate(float tpf) {
updateBoneIK(tpf);
updateWeaponAiming(tpf);
}
/**
* rotates the spine according to the angle of the camera.
* @param tpf
*/
private void updateBoneIK(float tpf) {
if (isAiming) {
camera.getRotation().toAngles(angles);
float rx = FastMath.clamp(angles[0], -0.1f, 0.75f);
tempRotation.fromAngles(0, 0, -rx);
// ???
spineBone.setUserControl(true);
spineBone.setUserTransforms(Vector3f.ZERO, tempRotation, Vector3f.UNIT_XYZ);
} else {
spineBone.setUserControl(false);
}
}
I don’t know what setUserControl(bool:Boolean) does, but I believe there are multiple ways to do it،
Since, Joint implements HasLocalTransform, then setting the local transform would work :
//stop the current Action
animComposer.removeAction("Running");
//fetch default start animation tracks or use const attributes
Vector3f defaultTranslation = transformTrack.getTranslations[0];
Vector3f defaultScale = transformTrack.getScales()[0];
Quaternion defaultQuat = transfromTrack.getRotations()[0];
Transform transform = new Transform(defaultTranslations, defaultRotations, defaultScales);
//teleport animation to start
joint.setLocalTransform(defaultTransform);
But, warning this is a teleport reset…not an animated reset.
I believe there are better ways, I will search more…& you will need to stop the currentAction detach & reattach to stop the animation, if you are planning to reset the animation during the tracks play…
, but you still can use a global variable of