ScaryFrank

I noticed in spAnimationState_addAnimation() that it adjusts the delay to be the end time of the last animation added. This seems to work as long as they are added in the same frame. But if you add a new animation after the previous animation has updated then it adds extra time.

Seems like it should account for the last animation's current time. Something like:
if (delay <= 0) {
if (last) {
if (last->time<last->endTime) delay += (last->endTime - last->time);
if (animation) delay -= spAnimationStateData_getMix(self->data, last->animation, animation);
} else
delay = 0;
}
Or maybe I'm missing something.
ScaryFrank
  • Сообщения: 16

Nate

Sounds good, committed. Had to make sure the remaining time is not < 0 though. Thanks!
Аватара пользователя
Nate

Nate
  • Сообщения: 9766

ScaryFrank

No proplem! Which time could be < 0? The "if (last->time<last->endTime)" check was to make sure the remaining time is >= 0.
ScaryFrank
  • Сообщения: 16

Nate

Oh you're right of course. :)
Аватара пользователя
Nate

Nate
  • Сообщения: 9766

ScaryFrank

:)

One thing I did notice when testing this is when I add an animation that has with no mix, it doesn't seem to play the last frame of the previous animation.

For instance, I play an animation where an attachment is made visible at the start and then made invisible on the last frame. Then I add a new animation to play after. It seems that sometimes the attachment never goes invisible before the second animation plays, as if the last frame is never reached.

I'm still debugging this, but maybe you have some insight? I can easily set the attachment to invisible at the start of all animations, but that seems like a bad workaround.
ScaryFrank
  • Сообщения: 16

Nate

I don't seem to be seeing that. With the spineboy example in spine-sfml:
AnimationState_setAnimationByName(drawable->state, 0, "drawOrder", false);
AnimationState_addAnimationByName(drawable->state, 0, "walk", false, 0);
0 start: drawOrder
0 event: drawOrder, headPop: 0, 0.000000, pop.wav
0 event: drawOrder, behind: 0, 0.000000, (null)
0 event: drawOrder, behind: 0, 0.000000, (null)
0 complete: drawOrder, 1
0 event: drawOrder, headAttach: 0, 0.000000, attach.wav
0 end: drawOrder
0 start: walk
The headAttach event happens on the last frame of the "drawOrder" animation. Applying an attachment key should work the same as triggering an event key.
Аватара пользователя
Nate

Nate
  • Сообщения: 9766

Nate

The complete happening before the headAttach event is a bit lame. I've rearranged things so complete happens in apply, not update, and I now get:
0 start: drawOrder
0 event: drawOrder, headPop: 0, 0.000000, pop.wav
0 event: drawOrder, behind: 0, 0.000000, (null)
0 event: drawOrder, behind: 0, 0.000000, (null)
0 event: drawOrder, headAttach: 0, 0.000000, attach.wav
0 complete: drawOrder, 1
0 end: drawOrder
0 start: walk
I also made some changes to the delay check when the next queued animation is played. Can you update and see if this fixes the issue you were seeing?
Аватара пользователя
Nate

Nate
  • Сообщения: 9766

ScaryFrank

Still has the issue, but I think the following fixed it.

In spAnimationState_update():
if (current->lastTime >= current->next->delay) _spAnimationState_setCurrent(self, i, current->next);
Changed to:
if (current->lastTime >= current->endTime) _spAnimationState_setCurrent(self, i, current->next);
ScaryFrank
  • Сообщения: 16

Nate

How do you repro the issue? Can you modify the spineboy example to make it show the issue?

That fix isn't correct unfortunately. The current animation should run until it reaches the delay for the next animation, then the next animation because the current animation. The delay can be less or more than the duration (end time) of the current animation.
Аватара пользователя
Nate

Nate
  • Сообщения: 9766

ScaryFrank

I see. In that case should it be:
if (current->lastTime >= (current->endTime + current->next->delay)) _spAnimationState_setCurrent(self, i, current->next);
I haven't looked at the Spineboy example. I will if this isn't current.
ScaryFrank
  • Сообщения: 16

Nate

Also doesn't work. The delay is the time at which to change animations.
Аватара пользователя
Nate

Nate
  • Сообщения: 9766

ScaryFrank

Ah, that's what I was missing, the delay is the start time for the next animation, not a delta time. But the change from my very first post changed it to be a delta time. Doh.. sorry for the confusion.
ScaryFrank
  • Сообщения: 16

Nate

Delay that is on a TrackEntry is always the start time of the next animation. Delay as specified as a parameter to addAnimation is the start time of the next animation if > 0 but if <= 0 then it is added to the end time of the current animation (since it is <= it means the end of the current animation or sooner).

Basically, I wanted the delay parameter to be the start of the next animation, which was fine. However, using the duration of the previous animation is common and a delay of <= 0 was meaningless, so I decided to overload the parameter by making <= 0 mean from the end of the previous animation. Maybe this is confusing and was a mistake?
Аватара пользователя
Nate

Nate
  • Сообщения: 9766

ScaryFrank

Ok, I think I understand. I would probably rename (spTrackEntry::delay) to (spTrackEntry::delayStartTime).

So, (spTrackEntry::delayStartTime) is the time to start the animation relative to the previous track entry.

If (delay == 0) the added animation starts when the previous animation finishes, so (delayStartTime) should be (last->endTime).

If (delay > 0) the added animation starts some time after the previous animation finishes, so (delayStartTime) should be (last->endTime + delay).

if (delay < 0) the added animation starts some time before the previous animation finishes, so (delayStartTime) should be (last->endTime + delay).

Does this seem right? Here's the (untested) code:
spTrackEntry* spAnimationState_addAnimation (spAnimationState* self, int trackIndex, spAnimation* animation, int/*bool*/loop,
float delay) {
spTrackEntry* last;

spTrackEntry* entry = _spTrackEntry_create();
entry->animation = animation;
entry->loop = loop;
entry->time = 0;
entry->endTime = animation->duration;

last = _spAnimationState_expandToIndex(self, trackIndex);
if (last) {
while (last->next)
last = last->next;
last->next = entry;
} else
self->tracks[trackIndex] = entry;

if (last) {
entry->delayStartTime = (last->endTime + delay);
entry->delayStartTime -= spAnimationStateData_getMix(self->data, last->animation, animation);
} else {
entry->delayStartTime = 0.0f;
}

return entry;
}

void spAnimationState_update (spAnimationState* self, float delta) {
int i;
float trackDelta;
delta *= self->timeScale;
for (i = 0; i < self->trackCount; i++) {
spTrackEntry* current = self->tracks[i];
if (!current) continue;

trackDelta = delta * current->timeScale;
current->time += trackDelta;
if (current->previous) {
current->previous->time += trackDelta;
current->mixTime += trackDelta;
}

if (current->next) {
if (current->lastTime >= current->next->delayStartTime) _spAnimationState_setCurrent(self, i, current->next);
} else {
/* End non-looping animation when it reaches its end time and there is no next entry. */
if (!current->loop && current->lastTime >= current->endTime) spAnimationState_clearTrack(self, i);
}
}
}
ScaryFrank
  • Сообщения: 16

ScaryFrank

Actually, I think it's less confusing to leave (spTrackEntry::delay) and have it be a time offset. I adjusted the code and tested it. Seems to work.
spTrackEntry* spAnimationState_addAnimation (spAnimationState* self, int trackIndex, spAnimation* animation, int/*bool*/loop,
float delay) {
spTrackEntry* last;

spTrackEntry* entry = _spTrackEntry_create();
entry->animation = animation;
entry->loop = loop;
entry->time = 0;
entry->endTime = animation->duration;

last = _spAnimationState_expandToIndex(self, trackIndex);
if (last) {
while (last->next)
last = last->next;
last->next = entry;
} else
self->tracks[trackIndex] = entry;

if (last) {
entry->delay = delay - spAnimationStateData_getMix(self->data, last->animation, animation);
} else {
entry->delay = 0.0f;
}

return entry;
}

void spAnimationState_update (spAnimationState* self, float delta) {
int i;
float trackDelta;
delta *= self->timeScale;
for (i = 0; i < self->trackCount; i++) {
spTrackEntry* current = self->tracks[i];
if (!current) continue;

trackDelta = delta * current->timeScale;
current->time += trackDelta;
if (current->previous) {
current->previous->time += trackDelta;
current->mixTime += trackDelta;
}

if (current->next) {
if (current->lastTime >= (current->endTime + current->next->delay)) _spAnimationState_setCurrent(self, i, current->next);
} else {
/* End non-looping animation when it reaches its end time and there is no next entry. */
if (!current->loop && current->lastTime >= current->endTime) spAnimationState_clearTrack(self, i);
}
}
}
ScaryFrank
  • Сообщения: 16

Nate

Let's consider the common use cases:

  1. Most often you want an animation to run after the previous animation, minus any mix time.
  2. The next most common use case is probably having a previous looped animation run for X seconds, then change to a new animation.
  3. Probably any other use case is uncommon enough users can calculate the time however is required.

To get 1, you can pass 0, whether delay is left as it is now or always added to the end time as you suggested. Eg, walk then jump:
state.setAnimation(0, "walk", false);
state.addAnimation(0, "jump", false, 0);
To get 2, as it is now you can just specify a positive time and this is the time the next animation is run:
state.setAnimation(0, "walk", false);
state.addAnimation(0, "jump", false, 5);
For your suggestion, you'd need to fetch the duration of the previous animation and subtract it from the time you want the next animation to start. This is a little unintuitive what "end time" is when the previous animation is looping. Eg, if you want to walk for 5 seconds, then jump:
TrackEntry entry = state.setAnimation(0, "walk", true);
state.addAnimation(0, "jump", false, 5 - entry.endTime);
// or:
state.setAnimation(0, "walk", true);
state.addAnimation(0, "jump", false, 5 - skeleton.findAnimation("walk").duration);
I think I like the current implementation the most. Delay is the time to change to the next animation. Since it is never useful to queue an animation with a delay of 0 (you might as well use setAnimation), then we can overload 0 to mean, "the end of the previous animation, minus mix time". Since negative delay is also never useful, we might as well overload <= 0 to mean "the end of the previous animation, minus mix time. plus the negative delay". It complicates understanding what the delay parameter does, but seems like it is easiest to use.

It's still open to discussion of course, just sharing my thoughts on why I think the current approach is ok.
Аватара пользователя
Nate

Nate
  • Сообщения: 9766

ScaryFrank

For #2, the user doesn't need the duration from the previous animations since it uses endtime when it's checked.
state.setAnimation(0, "walk", false);
state.addAnimation(0, "jump", false, 5);
When jump is added spTrackEntry::delay == 5. Then in the update the current endtime is adjust by delay.
if (current->lastTime >= (current->endTime + current->next->delay))
_spAnimationState_setCurrent(self, i, current->next);
I basically just changed spTrackEntry::delay to be an offset not a absolute time. Functionality should be the same as the current code, but it may be a little easier to understand.

There's no real need to change it, but I think the change I made makes it easier to follow the intent of the code.

Either way, I believe the following change from my original post is incorrect and should be changed back:
if (last->time<last->endTime) delay += (last->endTime - last->time);
ScaryFrank
  • Сообщения: 16

Nate

ScaryFrank писал(а):
state.setAnimation(0, "walk", false);
state.addAnimation(0, "jump", false, 5);
When jump is added spTrackEntry::delay == 5. Then in the update the current endtime is adjust by delay.
The user wants walk to loop for 5 seconds, then jump. With your proposal the user needs to use 5 - walk.duration.
Either way, I believe the following change from my original post is incorrect and should be changed back:
if (last->time<last->endTime) delay += (last->endTime - last->time);
Currently using:
if (last.time < last.endTime) delay += last.endTime - last.time;
delay -= data.getMix(last.animation, animation);
Аватара пользователя
Nate

Nate
  • Сообщения: 9766

ScaryFrank

I see what you mean now, my changes made it play walk then loop for an additional 5 secs. I thought your intent was for delay to always be from the end of the last animation.

The different behaviour for >0 and <=0 is a bit confusing. Perhaps different function calls instead. spAnimationState_addAnimation() and spAnimationState_appendAnimation()?


As for the change from the original post, here's an example why I think it's wrong.

// Assume walk duration is 2 secs
state.setAnimation( 0, "walk", false );
// Now 1.5 secs later, jump is added.
// The intent is for jump to play as soon as walk is finished.
// Assume no mix.
state.addAnimation( 0, "jump", false, 0 );
// So when jump is added in spAnimationState_addAnimation()
if (last->time < last->endTime) delay += last->endTime - last->time;
delay -= spAnimationStateData_getMix(self->data, last->animation, animation);
// walk->endTime == 2.0
// walk->time == 1.5
// jump->delay == 0.5

// In next call to spAnimationState_update()
if (current->lastTime >= current->next->delay) _spAnimationState_setCurrent(self, i, current->next);
// walk->lastTime == 1.5
// jump->delay == 0.5

So the jump plays immediate instead of after the walk finishes.

I think this might be a better way:
if (delay <= 0) {
if (last) {
delay += last->endTime;
delay -= spAnimationStateData_getMix(self->data, last->animation, animation);
} else
delay = 0;
} else {
if (last) delay += last->lastTime;
}
This way <=0 still means from the last animation and >0 means from the current time.
ScaryFrank
  • Сообщения: 16

Nate

Thanks for the illustrations, gotcha! Fixed in git for all runtimes. :)
Аватара пользователя
Nate

Nate
  • Сообщения: 9766


Вернуться в Runtimes