Fix for issue #54 - onComplete not being called for nested timeline.

cainmartin commented 7 years ago

Description

In certain circumstances Tina was killing the main timeline before all it's children were completed. This would result in some Tweens not fully completing, and not triggering their onComplete callbacks.

The issue results from the fact that the both players and playables (timelines/sequences and Tweens) inherit from BriefExtension, and it is in this object that the test for "completion" is made. There are no checks in this code to ensure that we are not currently a player with active dependencies - it treats players and playables as having the same completion conditions, therefore it can complete early - leaving any active tweens stranded.

In many (most) circumstances this would be okay, however, when all or some of the children have a duration that would complete within the current frame this could trigger the issue. Each playable/player in the hierarchy updates themselves, INCLUDING the main timeline.

To this end I added:

A check to make sure we only complete IF:
- We are a tween (and therefore have no children)
- We are a player, and all children are no longer active

Concerns

Due to the implementation - any child tweens that are due to finish within the same frame, have no guarantee of finishing order. So for example - if a frame is 16ms, and Tween one has a duration of 10, and Tween two has a duration of 15, there is no apparent guarantee which order they will finish.

Images

tina-image

ronkorving commented 7 years ago

cc @bchevalier :)

bchevalier commented 7 years ago

So the callback of the timeline would trigger before the callback of all its playables? If the completion is not triggered on the timeline if one active playable has not completed but is supposed to complete within the same tick, I am afraid that the completion of the timeline will be one tick late. I would rather make sure that the completion of a timeline happens, within the same tick, after the completion of all its playables.

cainmartin commented 7 years ago

The callback for each sub-timeline triggers after all of it's playables are finished (same behavior as it is now).

However, as you surmised - it is possible for the timeline to finish on a different (later) tick than one of it's children. For example, if there are two children that should finish on the same tick, then the first will finish, the next tick will occur, then the last child and the timeline will finish in the next tick.

I will need to have a look at how we can ensure they complete on the same tick - obviously this is not the current behavior anyway when durations are within the same frame.

cainmartin commented 7 years ago

@bchevalier Regarding the issue above. The current implementation does not appear to make any attempt to ensure the tweens/timelines finish on the same tick. So I think this will require some additional work to make sure this can happen in a robust way.

So I am investigating potential fixes for this. I have a couple of options that may work:

A) Implement messaging so that the tweens can inform their parent that they have been completed - when we add the tween to the timeline, the timeline can register for an onComplete event with the tween. Then we can keep track of completed / uncompleted tweens.

or

B) Modify the loop that iterates through a Players tweens. Currently it updates the list of playabables before iterating through them, which makes sense, but has the side effect that when you have finished iterating, the Player doesn't know whether they are all complete until the next tick. So I could check the playables once they have been processed and determine if they have completed, then complete the Playable (timeline) that contains them.

Do you have any thoughts on this?

Few last questions:

There is no sorting of durations currently - so say there are two tweens 1ms apart on the same Timeline - there is no guarantee which order they will complete (it will likely be based on order of insertion into the tween). The upshot is - if a user is expecting their callbacks to fire in a particular order, there are circumstances where this will currently not happen. How do you feel about sorting by duration before we kick off the main timer?
Once the main Timer starts, it never stops - is this by design?

bchevalier commented 7 years ago

I am not really in favor of adding an event emission system in TINA, would make it heavier and less performant. I would say solution B is the way to go.

About your questions:

I think we should guarantee the order of completion of the tweens.
Timers do not need to stop, there is usually one per game and it is responsible for playing all the playables. Are you having an issue with the fact that they do not stop?

cainmartin commented 7 years ago

Agreed regarding order of completion (although I would address that in a separate fix)
There is no actual issue with regards to the fact that timers do not stop
I will take another look at option b. In the past couple of weeks, I had an attempt at fixing it using this method - it was actually far more complex than I had hoped and resulted in artifacts. Will need to allocate more time to it.

Wizcorp / tina.js