API to support serializing AnimationState

[pharan]

Currently, AnimationState has some sane safeties for preventing multiple SetAnimation calls in the same frame from creating multiple TrackEntry objects which can cause unpredictable mixing.

But for instance, I want to set a specific track arrangement (maybe based on loaded game logic) where: track 1: shooting. time: 0.5 seconds. track 0: mixing between run and walk halfway

Currently, the necessary code is:

AnimationState state;

TrackEntry shootingTrackEntry = state.setAnimation(1, shooting, false);
shootingTrackEntry.trackTime = 0.5f; // 0.5 seconds

 // nextTrackLast = 0 is needed so AnimationState allows the next setAnimation to queue up another TrackEntry. Currently a private/internal field??
TrackEntry runningTrackEntry = state.setAnimation(0, run, true);
runningTrackEntry.nextTrackLast = 0;
TrackEntry walkingTrackEntry = state.setAnimation(0, walk, true);

walkingTrackEntry.mixTime = walkingTrackEntry.mixDuration * 0.5f; // halfway through mixing.

IDEA: Give TrackEntry an allowImmediateQueue method. In the current AnimationState implementation, it just sets the nextTrackLast to 0 if its value is AnimationState's sentinel value for preventing queueing.

public void allowImmediateQueue () {
    if (trackEntry.nextTrackLast < 0) trackEntry.nextTrackLast = 0;
}

[NathanSweet]

I agree some way around the replacement of an unapplied track entry is needed to setup AnimationState as you described. I've added to the setAnimation docs:

If the formerly current track entry was never applied to a skeleton, it is replaced (not mixed from).

If we use a TrackEntry method, it needs to be named carefully. allowImmediateQueue is pretty technical, my first response was: what is "immediate queue"? One way to describe it is "mark as applied", which matches the docs. A method name could be markAsApplied or setApplied (could be a setter and getter, or without args). We could just use keep, though there isn't any context and we may regret it later (eg holdPrevious is a similar concept to "keep"). Using a negative is usually not great, but not unheard of (see preventDefault in web event APIs, not that such terrible technology is exemplary in general). For us it would be something like preventDiscard (though replace is more accurate, as the previous "mixing from" entries are preserved). Of all these, I think boolean setApplied/getApplied is nicest.

Another option is to disable the discard (replace) behavior on AnimationState, with a boolean property like discardUnapplied.

@pharan @badlogic Any naming thoughts?

BTW, please leave issues without an enhancement, bug, ready, or in progress label to increase their visibility. Once reviewed/discussed, I'll move them to the appropriate label. This way it's harder to lose new issues and issues still under discussion in the sea of already accepted issues. Note applying editor, runtimes, website, or admin is still helpful.

[pharan]

The reason I wouldn't go for "applied" is that it assumes the user knows that being not applied is the reason behind not being able to arrange multiple animations on a track as mentioned in the first post.

I'm not pushing for "allowImmediateQueue". But I think the name is better if it just says what it enables, rather than describing its implementation which may change. I would just choose a different term than "immediate queue" that describes the activity of arranging the tracks and trackentries.

But if you think it's important or inconsequential to reveal the underlying implementation to the user, I guess mentioning "applied" is fine.

I'm not even sure if a series of "SetAnimation" calls is the best way to arrange a track to restore a certain state. But my recommendation above does seem to involve the least amount of change or additions in the current code.

[badlogic]

I wonder if this shouldn't be a (set of) method(s) on AnimationState instead of stuffing more fields into TrackEntry. Not sure exactly how that'd look without making the API surface of AnimationState more complex.

If we go with a field/getter/setter on TrackEntry I think markAsApplied is good from a user perspective.

[NathanSweet]

John's point makes some sense, that marking it as "applied" (especially when it really has not been) is a roundabout way of preventing it from being replaced. Still, the reason why an entry is replaced (it hasn't been applied) is documented and it is that state of not being applied yet that causes the replacement. I don't expect we'll change the reasons for replacement or have other things depend on the "not applied" state (I hope we never touch AnimationState again!) but who knows. I'm drawing a blank trying to name the method to be related to preventing the replacement, eg preventReplacement. :/

The underlying problem is AnimationState is hard to serialize and deserialize. We could allow entries to be created on their own, then the use case of setting up AnimationState mid-mix, etc would move the problem to assembling the entries rather than using set/addAnimation. This would be a new method, AnimationState setTrack(int, TrackEntry), which would set the entry for a track, discarding any existing entries:

public void setTrack (int trackIndex, TrackEntry entry) {
	if (entry == null) throw new IllegalArgumentException("entry cannot be null.");
	TrackEntry current = expandToIndex(trackIndex);
	if (current != null) {
		queue.interrupt(current);
		queue.end(current);
		disposeNext(current);
	}
	tracks.set(trackIndex, entry);
	queue.start(entry);
	queue.drain();
}

The other part of this is that the method to obtain an entry from the pool would need to be exposed to code outside the class (don't need method to free since it would be used with setTrack):

TrackEntry trackEntry (int trackIndex, Animation animation, boolean loop, TrackEntry last)

(loop could be removed.)

My worry is that this pig has a lot of state, so we'd have to expose enough (maybe not all, but maybe close) to allow storing and restoring any setup. Going through TrackEntry fields, the fields missing only getters are: next, mixingFrom, mixingTo. Fields missing both setters and getters are: nextAnimationLast, trackLast, nextTrackLast, interruptAlpha, totalAlpha. Which of those is necessary for serialization? Definitely: next, mixingFrom, mixingTo, trackLast, interruptAlpha. I'm not sure about the others.

Is it worth the effort and slight growth of the API to support serialization?

[badlogic]

Torn on this. I'm in favor of full AnimationState serializability. But the proposed solution would allow only partial serialization of AnimationState. What about events still hanging in the queue? Also, what about manual changes made to skeletons by a user, which is entirely outside of AnimationState?

I feel for full serialization, we'd need to consider much more than just AnimationState. I don't see a clear path towards full serialization. Unless we can figure such a path out, I don't think we should frame it as a serialization problem.

I do however think that the proposed setTrack() method and exposure of trackEntry() makes sense. It also plays nice with our manually memory managed runtimes. Would have to double check which other fields of TrackEntry we need to expose.

[NathanSweet]

What about events still hanging in the queue?

AnimationStateListener events are only queued within AnimationState methods. It should always be drained before returning.

Also, what about manual changes made to skeletons by a user, which is entirely outside of AnimationState?

We wouldn't be trying to serialize the skeletons themselves, but such code would be application level code, so out of scope for AnimationState serialization. Presumably the app would do what is needed to reapply such changes. App code would also be responsible for reattaching listeners to AnimationState and track entries, if any.

This initiative, if we choose to follow it up, would be just to expose getters and setters so the AnimationState can be stored and restored, which currently isn't possible.

[badlogic]

We've agreed to add two methods to AnimationState.

1. byte[] serialize() generates a binary blob that saves the entire state of the animation state. Basically, all track entries, and the pointers between each of them, including ones no longer on a track but still being mixed out from.
2. void deserialize(byte[] serializedState) takes a binary blob previously generated via serialize() and restores the state. The state will be cleared before deserialization.

Actually storing/loading the binary blob is left to the user as it depends on their game/app.