new animation groups / caching paradigm

[rogueSkib]

• Animators for a specific subject and group ID are created once and only once
• Animators are stored on the subject itself in the '__anims' cache object
  • replaces internal animate groups cache
  • this way Animators are garbage collected only when their subject is garbage collected
• fixes native animate commit bug

[rogueSkib]

@mgh @fairfieldt any thoughts on this PR?

[rogueSkib]

• removed the Group class altogether
• removed deprecated code and comments
• minor performance improvements, simplified some things

[rogueSkib]

added utility functions to handle all anims on a subject

• clearSubjectAnimations
• commitSubjectAnimations
• pauseSubjectAnimations
• resumeSubjectAnimations

[rogueSkib]

I think that's all I've got for now ... tested on various games, and so good so far :)

[rogueSkib]

Good feedback from @oodavid, the Groups API was convenient, in that it allowed for easy callback at the end of a set of disparate animations, for example:

animate(el1, 'outro').now({ opacity: 0.6      }, 500, animate.easeOut);
animate(el2, 'outro').now({ x: 0              }, 700, animate.easeOutBack);
animate(el3, 'outro').now({ y: centery-(gx*7) }, 800, animate.easeOutBack);
animate(el4, 'outro').now({ x: (gx*5)         }, 500, animate.easeOutBack);
animate.getGroup('outro').once('Finish', 'resumeUserInput');

I'm adding a Finish event when an Animator completes all of its queued frames, but it's not as powerful as the former. Going to think about a solution or alternative.

[rogueSkib]

I was able to preserve the animate.getGroup and Finish event API:

• the Group class is now only created upon request
  • managed by user, still no internal cache
  • fires Finish event when all active animations finish
  • added support for clear, commit, pause, and resume to apply to all animations within the group

[rogueSkib]

Summary of new Group class:

• finds animations w the same groupID across different subjects
• created and returned by animate.getGroup(groupID)
• reset populates the group with all active animations w matching groupID
• subscribe to the 'Finish' event to fire a callback when all animations in the group complete, for example: myGroup.once('Finish', function () { ... });
• exposes clear, commit, pause, and resume to apply to all group animations
• creating or resetting a group traverses your view hierarchy
• saving a group will also save any subjects from being garbage collected

[rogueSkib]

From Devkit-core PR: https://github.com/gameclosure/devkit-core/pull/34

[mgh]

I like the new functionality added to groups!

I'm on the fence about groups iterating over the view hierarchy. A number of edge cases come to mind -- for example, if a view is removed from the hierarchy while animating in a group, will its animation finish, or will the corresponding group never finish? (does the behavior differ on native) These edge cases may not actually matter in practical usage. Performance seems like it might be a real issue though. This API scales with number of views, which is a big departure from the previous API. We try to avoid O(n) APIs[ most of our worst APIs like input are height of view tree, which is ~log(n) (worst case O(n) when your tree is a stick).

We're changing the group API, so it may require code changes for anyone who's already using it. It might be worth considering if there's a better API for this. Maybe Group should be an independent class where you explicitly create an instance and add animations to it, regardless of the group name.

Potentially relevant, and worth noting, getGroup used to return the same group instance every time, but now it returns a new one each time (is that a bug? will it invalidate older group instances with the same name?). Also, on the subject of edge cases, will this work as expected if an animation emits finish multiple times?

[oodavid]

I've been emailing Jimmy directly about this, but since mgh is chatting I'll move here.

Noticed that Groups act strangely, the "anims" array never appears to reset, I've made a little gist that demonstrates it nicely:

https://gist.github.com/oodavid/202177ae2f0e6baee7ad#file-application-js-L29

The first callback is triggered OK, but subsequent callbacks aren't, I see this in my console:

-- NEW ANIMATION --
Expected group size: 5
Reported group size: 5
callback fired!
-- NEW ANIMATION --
Expected group size: 8
Reported group size: 11
-- NEW ANIMATION --
Expected group size: 11
Reported group size: 13

^ the group size keeps growing as new animations are added to the group, but old ones are never removed, I'm guessing that's why the callback never fires.

I've tried using g.reset(); in various places but it doesn't appear to function as expected.

EDIT

I've noticed another oddity, when animating a zero-sized group the Finish callback isn't triggered instantly as expected, however it IS triggered on the _subsequent_ group Finish event:

-- NEW ANIMATION --
Expected group size: 0
Reported group size: 15
-- NEW ANIMATION --
Expected group size: 8
Reported group size: 15
callback fired!

I have a feeling thats a distinct, but related issue.

[rogueSkib]

Thanks for the feedback @mgh and @oodavid !

To address some things David mentioned over email, I'm adding back the Group.isActive function. It will rely on Animator.hasFrames for each animation like it used to. hasFrames is already implemented in native as well. You also asked about the Finish callback firing on the next tick in the event of an empty (all finished) group: the reason it's implemented like this is because the callback is created and hooked up in the reset function which is also called from init. If I fired it instantly, it would fire before it was possible to subscribe to Finish. I'll make sure to add a comment about this.

Martin mentioned the view hierarchy being modified. This does affect Group but only at the time of reset or init. The group should still finish because animations are scheduled independently from their views or subjects. Views that aren't on the hierarchy won't get added to a group, but removing them or moving their position within the hierarchy will have no effect.

I do agree on the issue of performance though. If traversing the view hierarchy proves to be a bottle neck, it would be risky to use getGroup in a tight game loop. The alternative so far was to remove Group altogether and change the API. I'd prefer to add a bolder warning label than remove the API. When I added the API back in, my goal was to avoid having anyone make code changes, but if we do opt in that direction, I'm open to a Group class that's manually constructed or enabled in some way.

As currently implemented, Group only listens to each Animator once. If you're hanging onto an instance of Group, you'd need to call group.reset before you'd get another Finish event from the same Animators. Alternatively, letting your Group instances get garbage collected, you can just call animate.getGroup again. It's possible to have multiple Groups with the same ID, but they shouldn't interfere with each other. It all depend on how you subscribe to them and what you expect. For example, the use case is to call animate several times, and then get a group using your groupID. If you animate some things with a groupID, call getGroup, animate more things with the same ID, and call getGroup again, you could have differently sized groups on the same ID, but they will fire when all of the active animations that existed at the time of the getGroup call are finished.

David, your last comment raises a bug I realized existed when I first read your email! The view hierarchy traversal does not take into account whether or not the Animators are active (hasFrames). Animators on non-view subjects require that animations be active because they are collected based on whether or not they are scheduled. Adding the hasFrames check to the view traversal collection in reset should resolve the issue you found. The only other way you'd see that is if you're tapping quickly enough so that you're also getting views that are still animating from your previous touch.

[mgh]

What's the reasoning behind creating groups with getGroup rather than the previous style of creating groups when animations are created? The previous method did not require hierarchy iteration.

With regards to multiple groups with the same id, it looks like anim._groupFinishCB will get overwritten. Is this a concern?

[rogueSkib]

@mgh I ran some performance tests on the new getGroup, and as expected, it's painfully slow to traverse the view hierarchy. The view ID caching / traversal was a work-around for the fact that we can't easily find out which animators are scheduled in native animate (which only applied to views).

We have a couple of ways forward from here. We need to decide on a few things, I'm generally open to any of the options.

• We either preserve or break the current API
• We either keep or change the Animator caching paradigm

We have some interesting native code to write unless we choose to break the current API and keep the new Animator caching.

The caching paradigm change was created because JS and native did not create and destroy Animators symmetrically and because I liked the idea of creating Animators once and only once, and caching them on the subject for garbage collection. If we keep this change, then we either need to write a native function for getting scheduled animations to avoid view traversal, similar to how we are looking up PubSub listeners in JS, or we throw out the Groups API altogether.

If we throw out the new caching, we can try to tackle the native bug in Animator creation / destruction. Otherwise, multiple Animators on the same subject / groupID can fight over precedence and will continue to break animate().commit on native.

[rogueSkib]

@mgh You're right, anim._groupFinishCB getting overwritten could potentially cause problems. I added that so that clear could remove only that listener as opposed to all listeners on Finish.

[rogueSkib]

Had a conversation with Martin earlier, and we found a much better solution for Group caching. Once you use a certain groupID, there will always be one and only one corresponding Group cached in animate, accessible via animate.getGroup(groupID). The group automatically fills and empties based on which animators in the group are active. Every time the group empties, it fires a 'Finish' event. It's tremendously cleaner than what I had earlier, and the native pieces were trivial to implement. I've tested on android, iOS, and in the browser, so far so good.

[rogueSkib]

So the final API changes are:

• Group.get no longer exists
  • animate(view, 'groupID') accomplishes the same thing (returns an Animator)
• Group.add changed, and should primarily be used internally

[oodavid]

@rogueSkib - pushing and slicing the group array like that is how I expected it to act, I think that's a really robust solution. It will handle extra animations being added to the group and it's useful to return the same Group object each time.

A and B the C of D as always!

[oodavid]

@rampr - _if_ you were having issues with animation Groups on Bubble Quest, I think this branch may be worth testing.

[rogueSkib]

@oodavid Thanks again, David!

@rampr this branch of devkit-core has all the submodules updated for the changes, including this timestep update and both native platforms, if you're interested: https://github.com/gameclosure/devkit-core/pull/34

[rogueSkib]

Good catch! I meant to move this line: groupID = groupID || DEFAULT_GROUP_ID; up before the group creation, but forgot!

I also updated the add / remove functions to protect against missing groups, just in case.

[rogueSkib]

Since we have the groups cache working, I added some global animation functions to pause, resume, clear and commit everything. I always found myself tracking tons of animations, and pausing all of them when someone tapped a pause button. If you animate your pause UI, you'd want to make sure you animate it after calling animate.pauseAllAnimations() though :)

[rogueSkib]

I ended up creating a test-suite of sorts to finish up this PR. In the process, I found a few differences in behavior on native from the expected behavior seen in the browser. I fixed these, and I think we're finally at the finish line here. I'll test with some existing games over the next day or so, but I expect this is ready to merge.