Do not process tween if it has already ended

[AlexandreSi]

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?

[jeremyckahn]

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:

1. 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.
2. 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!

[AlexandreSi]

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

[jeremyckahn]

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:

1. Add the following private properties to the Tweenable class:

/** @private */
this._doRenderAfterStopping = true
/** @private */
this._didFinalRender = false

2. Add doRenderAfterStopping to the shifty.tweenConfig typedef as a boolean.

3. In setConfig, pull doRenderAfterStopping out of the config object and set it to this._doRenderAfterStopping, and set this._didFinalRender to false.

4. Update the logic of processTween like so:
   a. Early return if tween._doRenderAfterStopping === false && tween._didFinalRender === true
   b. At the final render, set tween._didFinalRender = true

I think that should at least mostly implement this functionality. We'll need some unit tests for this new behavior as well.

[jeremyckahn]

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":

1. The linked list of "active" tweens are processed in a loop:

   shifty/src/tweenable.js

   Lines 191 to 202 in c22f6c3

   	export const processTweens = () => {
   	let nextTweenToProcess
   	const currentTime = Tweenable.now()
   	let currentTween = listHead
   	while (currentTween) {
   	nextTweenToProcess = currentTween._next
   	processTween(currentTween, currentTime)
   	currentTween = nextTweenToProcess
   	}
   	}

2. In processTween, the tween is processed to see if it is complete. If so, tween.stop(true) is called:

   shifty/src/tweenable.js

   Lines 120 to 141 in c22f6c3

   	const processTween = (tween, currentTime) => {
   	let timestamp = tween._timestamp
   	const currentState = tween._currentState
   	const delay = tween._delay
   	if (currentTime < timestamp + delay) {
   	return
   	}
   	let duration = tween._duration
   	const targetState = tween._targetState
   	const endTime = timestamp + delay + duration
   	let timeToCompute = currentTime > endTime ? endTime : currentTime
   	tween._hasEnded = timeToCompute >= endTime
   	const offset = duration - (endTime - timeToCompute)
   	const hasFilters = tween._filters.length > 0
   	if (tween._hasEnded) {
   	tween._render(targetState, tween._data, offset)
   	return tween.stop(true)
   	}

3. In stop(), the tween is removed from the linked list of active tweens, effectively preventing processTween() from being called upon it in the future:

   shifty/src/tweenable.js

   Lines 720 to 727 in c22f6c3

   	stop(gotoEnd = false) {
   	if (!this._isPlaying) {
   	return this
   	}
   	this._isPlaying = false
   	remove(this)

   shifty/src/tweenable.js

   Lines 301 to 330 in c22f6c3

   	const remove = ((previousTween, nextTween) => tween => {
   	// Adapted from:
   	// https://github.com/trekhleb/javascript-algorithms/blob/7c9601df3e8ca4206d419ce50b88bd13ff39deb6/src/data-structures/doubly-linked-list/DoublyLinkedList.js#L73-L121
   	if (tween === listHead) {
   	listHead = tween._next
   	if (listHead) {
   	listHead._previous = null
   	} else {
   	listTail = null
   	}
   	} else if (tween === listTail) {
   	listTail = tween._previous
   	if (listTail) {
   	listTail._next = null
   	} else {
   	listHead = null
   	}
   	} else {
   	previousTween = tween._previous
   	nextTween = tween._next
   	previousTween._next = nextTween
   	nextTween._previous = previousTween
   	}
   	// Clean up any references in case the tween is restarted later.
   	tween._previous = tween._next = null
   	})()

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.

[AlexandreSi]

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!

[AlexandreSi]

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:

https://github.com/4ian/GDevelop/blob/440205531fa586c400ed0bbfdd7b9beee7efe9eb/Extensions/TweenBehavior/shifty_setup.ts#L35-L48

• 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:

shifty/src/tweenable.js

Lines 656 to 683 in ba285da

	_resume(currentTime = Tweenable.now()) {
	if (this._timestamp === null) {
	return this.tween()
	}
	if (this._isPlaying) {
	return this._promise
	}
	if (this._pausedAtTime) {
	this._timestamp += currentTime - this._pausedAtTime
	this._pausedAtTime = null
	}
	this._isPlaying = true
	if (listHead === null) {
	listHead = this
	listTail = this
	} else {
	this._previous = listTail
	listTail._next = this
	listTail = this
	}
	return this
	}

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.

[jeremyckahn]

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 Tweenables 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.

[jeremyckahn]

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?

[AlexandreSi]

Just tested it out, it works just fine!

[jeremyckahn]

Thank you for checking @AlexandreSi! I'll work to get a release out in the next day or so.

[AlexandreSi]

Thank you for your work and your explanations! Looking forward to release a new version of GDevelop with this fixed!

[jeremyckahn]

The fix for this issue has been released in v2.20.4.

Thanks again for reporting this @AlexandreSi!