Key frame management rfc

[tsherif]

Simple system to support animations used by the Elevate team. Allows the application to set the key frame time points, and then uses the time values from a Timeline object to provide two pieces of information useful in key frame animations:

• the current frame index
• the interpolation factor between them

Note that this system is completely agnostic to the type of animation being performed.

[coveralls]

Coverage remained the same at 44.46% when pulling ba01d2a on key-frame-rfc into f0e8bc9 on master.

[ibgreen]

Just some quick thoughts from quick scan:

It seemed to me that the const kf = new KeyFrames(timeline); design creates a sub-optimal ownership chain?

With the proposed constructor:

1. animationLoop owns timeline
2. keyframes have animationLoop as parent
3. But who owns KeyFrames? The app needs to hold references to both the animationLoop and the keyframes?

I normally prefer designs with one manager class in control of other things. In this case Timeline seems the natural owner.

timeline.addKeyFrames(new KeyFrames(...))

This gives the animationLoop->timeline->keyFrame[] ownership/resource management chain.

I would also have liked to see the key frame concept could just be new type of "channel" on the timeline, that would

• make it straightforward to manage many key frames for different data sets that update at different times.
• automatically make these available in the animation loop props

[chrisgervang]

I like Ib's idea here for all the benefits he mentioned:

the key frame concept could just be a new type of "channel" on the timeline.

It could look like timeline.addKeyframesChannel([]) which returns a KeyFramesChannel object.

To take it a bit further, I would like to propose a bit more extensibility. Could KeyFramesChannel be extended from Channel and then reuse timeline.addChannel() for different kinds of channels?

Since I plan to extend/compose the "Keyframe" object while developing different type interpolations, it would be nice to actually extend from KeyFramesChannel and supply my custom objects to timeline.addChannel() (as long as they satisfy the Channel interface). This is a very OOP approach, so my only concern with it is if that fits within luma's existing code style - I'm open to other styles, but I do like this proposed design.

For more context, examples of various type interpolations I'll make are combinations of:

• Data groupings: single property, multi-property, camera, etc..
• Data types: color, number, string set, number range, etc..
• Easing functions between each keyframe: sin wave, smooth, linear, etc.

[chrisgervang]

As is, wouldn't this also take a channel as an input? I think I'd like to create a channel for each unique property I'm animating, and organize as a set of keyframes for each channel.

[tsherif]

Yes, definitely. I thought the channel might complicate the example somewhat, since it implies that timeline.setTime(t) doesn't necessarily mean the key frame time is t. But I'll just add it if it makes it clearer for the RFC.

[Pessimistress]

For deck's animation feature to utilize this, we also need ways to:

• break (cancel all future key frames)
• append new key frames

[tsherif]

If I understand correctly, I don't think you need a break, since the key frames don't run on their own. You would just stop animating based on the key frames if you want to stop. And you can arbitrarily change the key frame set by calling setKeyFrames with new data.

[Pessimistress]

Not a deal breaker but it would be great if I can attach additional information to a keyframe (e.g. a key string with which I can retrieve the states associated with the ongoing segment)

[tsherif]

I was thinking the index would be the identifier. Would that not be enough?

Could also go with making each key frame a pair, time plus some arbitrary data.

[tsherif]

I normally prefer designs with one manager class in control of other things. In this case Timeline seems the natural owner.

The problem with this approach is that it creates a dependency chain, where the application is forced to pull in code that it doesn't need. And in this case, KeyFrames would go to everyone, since Timeline is imported by AnimationLoop.

I'd like keep the luma.gl classes on the simpler side, moving towards that concept of a composable toolbox rather than a full engine, and let deck.gl (or other users) put together tools as needed.

[tsherif]

Made a little change to the API that I hope addresses some of @Pessimistress and @chrisgervang's concerns. API now allows associating arbitrary data with each key frame and separate API endpoints for start and end key frames:

const kf = new KeyFrames(timeline, channel);  // Assume channel has a rate of 1

// Each key frame is pair, [time point, arbitrary data]
kf.setKeyFrames([
  [0, { val1: [1, 0, 1], val2: 0} ],
  [500, { val1: [1, 1, 1], val2: 2} ],
  [800, { val1: [0, 0, 1], val2: 1} ],
  [1200, { val1: [0, 1, 0], val2: 4} ],
  [1500, { val1: [1, 0, 1], val2: 5} ]
]);

timeline.setTime(1000);

kf.getStartIndex(); // => 2                            (i.e. key frame at time=800)
kf.getEndIndex();   // => 3                            (i.e. key frame at time=1200)
kf.getStartData()   // => { val1: [0, 0, 1], val2: 1}  (i.e. data at index 2)
kf.getEndData()     // => { val1: [0, 1, 0], val2: 4}  (i.e. data at index 3)
kf.getFactor();     // => 0.5                          (i.e. halfway between 800 and 1200)

I want to stop short of attempting to actually interpolate the data as that requires knowledge of data types, which I think is better left in the application. I also added some text about the design decisions behind the API, and why I want to keep this functionality out of Timeline.

[chrisgervang]

I think the changes to the API work for my needs. I'm not really sure how to comment on composable toolbox vs full engine, but will work with what you all agree on.

[ibgreen]

The problem with this approach is that it creates a dependency chain, where the application is forced to pull in code that it doesn't need. And in this case, KeyFrames would go to everyone, since Timeline is imported by AnimationLoop.

Just as a clarification, the way I was imagining the implementation of timeline.addKeyFrames(new KeyFrames(...)) there would be no static dependency chain.

Since Timeline never creates KeyFrames instances itself, it doesn't need to import the KeyFrames class (in JavaScript one does not need to import a class to call methods on it) and so the decision on whether to depend on KeyFrames is left to the application.

I.e. if the application does not import {KeyFrames} the KeyFrames class remains fully tree-shakeable even in the changed design, but the ownership chain is IMHO cleaner.

[tsherif]

Oh, I see. That could actually simplify things internally too. Since the timeline would update the keyframes whenever it's updated. And to add support for channels, it could be like this:

const kf = new KeyFrames([...]);
const kfHandle = timeline.addKeyFrames(kf, channel);
timeline.setTime(t);
kf.getStartData();
// ...etc.
timeline.removeKeyFrames(kfHandle);

[ibgreen]

it could be like this:

Yes... or maybe like this? (just brainstorming around simplification...)

timeline.addChannel(channelName, new KeyFramesChannel([...]));
timeline.setTime(t);
timeline.getChannel(channelName).getStartData();
// ...etc.
timeline.removeChannel(channelName);

[tsherif]

I don't want to couple the timeline and keyframes too tightly. The timeline is intended to produce time values that can be used by any system, not just keyframes. To drive that decoupling home, maybe the API should just be:

const handle = timeline.addAnimation(animation, channel);
timeline.setTime(t);
timeline.removeAnimation(handle);

And animation could be any object that has a setTime method (which would include KeyFrames objects).

[tsherif]

Ok, got a few final updates in:

• Timeline can now "attach" and "detach" animations (defined as any object with a setTime method), and will update their times when necessary.
• KeyFrames no longer takes timeline/channel as constructor arguments. Instead, can take the array of key frames directly on construction. setKeyFrames will still exist for updates.

[rolyatmax]

Will we require these keyframes to be sorted before they are passed to the KeyFrames constructor? Must the first keyframe always be 0?

[tsherif]

The key frame times should be sorted. The first key frame does not have to be 0.

[rolyatmax]

What will we return as the index (and as the data) when the current time is before the first keyframe or after the last keyframe? We won't be animating from or to any values in those cases.

[tsherif]

They will will be clamped to pairs (0, 1) and (numKeys - 2, numKeys - 1).