Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cainmartin
Copy link
Collaborator

@cainmartin cainmartin commented May 24, 2017

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
Copy link

cc @bchevalier :)

@cainmartin cainmartin assigned ronkorving and unassigned ronkorving May 24, 2017
@bchevalier
Copy link
Contributor

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
Copy link
Collaborator Author

cainmartin commented May 25, 2017

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
Copy link
Collaborator Author

cainmartin commented May 30, 2017

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

  1. 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?

  2. Once the main Timer starts, it never stops - is this by design?

@bchevalier
Copy link
Contributor

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:

  1. I think we should guarantee the order of completion of the tweens.
  2. 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
Copy link
Collaborator Author

  1. Agreed regarding order of completion (although I would address that in a separate fix)
  2. There is no actual issue with regards to the fact that timers do not stop
  3. 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants