search

KirmesBude / bevy_trickfilm

bevy_trickfilm
Apache License 2.0
6 stars 3 forks source link

Use keyframe index to set `animation.finished` instead of match arms #17

Closed PraxTube closed 10 months ago

PraxTube commented 10 months ago

The changes introduces in #15 are actually not working correctly. The reason for that is that Ok(n) and Err(n) don't seem to fire on the last frames. This implementation does work, which is a bit confusing as the returns should prevent it from working.

Addresses #14.

KirmesBude commented 10 months ago

Oh, yeah. Sorry, I might have been able to catch that, if I put some more effort in the previous PR review :D Another thing that I noticed after I approved it is the method name. I think you had the right approach with finished to stay consistent with paused. Not sure why I suggested is_finished. You can take care of that in this PR or I will do it tomorrow (probably).

I think this could also benefit from an example, but I will take care of that myself soon. Otherwise looks fine and I will merge it tomorrow. Thanks again.

PraxTube commented 10 months ago

Oh, yeah. Sorry, I might have been able to catch that, if I put some more effort in the previous PR review :D Another thing that I noticed after I approved it is the method name. I think you had the right approach with finished to stay consistent with paused. Not sure why I suggested is_finished. You can take care of that in this PR or I will do it tomorrow (probably).

No worries, I should have done a better job to begin with (I thought it worked because the last sprite of the animation I tested was simply blank, and I thought I despawned the entity when is_finished returned true :upside_down_face: ). Regarding the naming, the way you check if the animation is paused is through is_paused, so in that sense is_finished is actually more consistent. The AnimationPlayer2D has the private field paused, but that isn't used by the user.

EDIT: I just noticed that there is also speed(), so perhaps the most consistent way is to use paused() and finished()? Though this is really mostly a matter of personal preference.

KirmesBude commented 10 months ago

EDIT: I just noticed that there is also speed(), so perhaps the most consistent way is to use paused() and finished()? Though this is really mostly a matter of personal preference.

Good point, I will think about what makes the most sense and keep it consistent then.