Do not process tween if it has already ended
AlexandreSi opened this issue · 12 comments
Hi,
I am working on GDevelop game engine. We use shifty to add tweens in the games made with GDevelop so thanks for your work!
One of our user is having troubles and I ended up digging into your code (See 4ian/GDevelop#5097).
I noticed that you use a flag _hasEnded
in your instance (thanks @arthuro555 for this!). I figured that it could be logic that when processing tweens, this flag should be checked to know if the render method should be called or not. Something like:
const processTween = (tween, currentTime) => {
if (tween._hasEnded) return;
let timestamp = tween._timestamp
const currentState = tween._currentState
const delay = tween._delay
if (currentTime < timestamp + delay) {
return
}
...
What do you think?
Hi @AlexandreSi, thanks for reporting this and for using Shifty in GDevelop! What you're proposing seems reasonable, but I don't know if such a change would be safe to make. There are two reasons for this:
- In all cases, tweens should render the final frame. This can be handled by just checking to see if the "end" frame has been rendered and early return from
processTween
if so. - For immediate mode rendering use cases, this might break the animation. Specifically, the tweened object would simply disappear after its final frame.
Regarding point 2, Shifty is generally designed around immediate mode use cases (and therefore happens to work well with retained mode at a minor performance cost). It looks like the suggested change would need to be implemented as opt-in behavior to avoid changing the established behavior of Shifty (which, as far as I know, works well for most users).
I can try to implement support for this behavior in the next week or so, but feel free to open a PR if you need it sooner than that. Let me know if this sounds good to you!
Thanks for your quick answer!
I'm not sure I fully understand the strategy that you suggest so I might not put work on the right place.
It's not urgent but if you have the time to provide some guidance, I can help!
Thanks again
For sure! For reference, either for myself later or anyone who gets to this before me, here's what I think a solution for this might look like:
- Add the following private properties to the
Tweenable
class:
/** @private */
this._doRenderAfterStopping = true
/** @private */
this._didFinalRender = false
-
Add
doRenderAfterStopping
to theshifty.tweenConfig
typedef as a boolean. -
In
setConfig
, pulldoRenderAfterStopping
out of theconfig
object and set it tothis._doRenderAfterStopping
, and setthis._didFinalRender
tofalse
. -
Update the logic of
processTween
like so:
a. Early return iftween._doRenderAfterStopping === false && tween._didFinalRender === true
b. At the final render, settween._didFinalRender = true
I think that should at least mostly implement this functionality. We'll need some unit tests for this new behavior as well.
After spending some time with this, I'm questioning if this is actually an issue within Shifty.
I noticed that you use a flag
_hasEnded
in your instance (thanks @arthuro555 for this!). I figured that it could be logic that when processing tweens, this flag should be checked to know if the render method should be called or not. Something like:
I think this is effectively what is already happening. Here's an overview of the tween "stopping logic":
-
The linked list of "active" tweens are processed in a loop:
Lines 191 to 202 in c22f6c3
-
In
processTween
, the tween is processed to see if it is complete. If so,tween.stop(true)
is called:Lines 120 to 141 in c22f6c3
-
In
stop()
, the tween is removed from the linked list of active tweens, effectively preventingprocessTween()
from being called upon it in the future:Lines 720 to 727 in c22f6c3
Lines 301 to 330 in c22f6c3
The end result of all of this is that render
is not called upon a tween once it has stopped or ended. So, this seems like it may be either be an issue specific to GDevelop, or our understanding of the issue within Shifty may be incorrect.
In any case, I've done some experimental work based off my original plan to change Shifty: https://github.com/jeremyckahn/shifty/compare/feature%2F175__dont-render-after-stop?w=1
@AlexandreSi would you be able to pull down that branch and see if it changes the problematic behavior you're seeing in GDevelop? You'll just need to build the library before importing it into GDevelop to try it out. The built file you'll want to use is dist/shifty.js
.
Thanks for the explanation, I missed that!
Indeed, the build with your commit does not improve things in GDevelop, I'll dig deeper.
Sorry for the bad lead!
So, here is where I'm at now:
- When a GDevelop scene is paused, you can come back to it with a "resume" action (similar to shifty's scene resume method)
- Here is what happens when the scene is resumed:
- When looking at what
shifty.Tweenable.resume
does, I don't see a check whether the tween has ended or not, but I don't know if this check should be done in the context that users usually use shifty for:
Lines 656 to 683 in ba285da
So when the GDevelop scene is resumed, the shifty scene is resumed and all stopped tweens have their flag _isPlaying
set back to true and are added to the list.
Thanks for the analysis @AlexandreSi! This is really helpful. I think the issue isn't with Tweenable#resume
, but rather Scene#resume
. It seems that Scene#resume
needs to do some filtering to only call resume
on Tweenable
s that have not ended.
I'll explore a solution in a new branch to see if it helps the issue you're seeing in GDevelop.
I've started a branch with an exploratory fix for this issue: https://github.com/jeremyckahn/shifty/compare/fix%2F175__dont-resume-complete-tweenables-in-scene?w=1
@AlexandreSi would you mind checking out the fix/175__dont-resume-complete-tweenables-in-scene
branch and seeing if it improves the behavior in GDevelop?
Just tested it out, it works just fine!
Thank you for checking @AlexandreSi! I'll work to get a release out in the next day or so.
Thank you for your work and your explanations! Looking forward to release a new version of GDevelop with this fixed!
The fix for this issue has been released in v2.20.4
.
Thanks again for reporting this @AlexandreSi!