theatre icon indicating copy to clipboard operation
theatre copied to clipboard

Enable manual flush & explore custom animation drivers

Open AndrewPrifer opened this issue 2 years ago • 17 comments

This PR introduces the concept of animation drivers as separate from tickers.

Whereas a ticker is responsible for batching updates to ticks, a driver is responsible for driving the ticker.

The private APIs introduced in this PR are:

  • class AnimationDriver: represents the concept of drivers. Can drive anything that needs to be updated at regular intervals.
  • Ticker.applyDriver(): applies the provided AnimationDriver to the Ticker.

Public APIs exposed:

  • function applyAnimationDriver() with 4 overloads:
    • function applyAnimationDriver(driver: 'raf'): Applies a driver using Window.requestAnimationFrame().
    • function applyAnimationDriver(driver: 'xrDriver', xrSession: XRSession): Applies a driver using XRSession.requestAnimationFrame() and the provided XRSession.
    • function applyAnimationDriver(driver: DriverFn): Applies a driver using the provided custom updater function.
    • function applyAnimationDriver(driver: null): Removes any animation drivers.

Note: Ticker is not exposed in this PR, instead, the concept of drivers is introduced. This both reduces the API surface area exposed and reduces the responsibility of users and the number of mistakes they can make.

How to use:

import { applyAnimationDriver } from '@theatre/core'

// Drive animations using a rAF
applyAnimationDriver('raf')

// Drive animations using a rAF in XR
applyAnimationDriver('xrRaf', myXrSession)

// Drive animations using a custom updater
applyAnimationDriver((update) => {
  let id = 0
  const onAnimationFrame = (t: number) => {
    update(t)
    id = myCustomScheduler.requestFrame(onAnimationFrame)
  }
  id = myCustomScheduler.requestFrame(onAnimationFrame)
  return () => myCustomScheduler.cancelFrame(id )
})

// Remove any drivers and stop animations from updating altogether
applyAnimationDriver(null)

Solves #39 and would replace #88 as a solution. @felthy, @donmccurdy, does this solution look like something you could use, or even prefer? This is just an experiment to expose this functionality as minimally (and as conveniently) as possible.

AndrewPrifer avatar Jun 22 '22 13:06 AndrewPrifer

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
theatre-playground ✅ Ready (Inspect) Visit Preview Jun 26, 2022 at 2:40PM (UTC)

vercel[bot] avatar Jun 22 '22 13:06 vercel[bot]

Whereas a ticker is responsible for batching updates to ticks, a driver is responsible for driving the ticker.

I'm a bit lost on this distinction — what's a tick, in relationship to e.g. an rAF frame? If the user has defined a custom driver, what does it mean to 'batch' its updates?

If I were to implement rAF 'from scratch' using the new API, would this be correct usage, and does it apply updates to the theatre properties synchronously in the update(t) call?

applyAnimationDriver((update) => {
  let requestID = -1;
  function render(t) {
    update(t);
    requestID = requestAnimationFrame(render);
  }
  requestID = requestAnimationFrame(render);
  return () => cancelAnimationFrame(requestID);
})

And — is the idea that different drivers could eventually be used for different sequences? so e.g. some properties are time-driven and others are scroll-driven?

Thanks!

donmccurdy avatar Jun 22 '22 14:06 donmccurdy

@donmccurdy thank you for exposing the holes in my explanation! :D

Ticker is a class that all dataverse (theatre's internal rfp engine) APIs use to execute updates. Dataverse can schedule updates using the Ticker, and the ticker is responsible for batching these updates together to execute at regular intervals. The implementation of the ticker, the batching strategy, its API could change. .tick() is just a method to let the Ticker know that it can execute things (some, or all, or as it pleases, this could change in the future).

Since all our use case requires is calling tick(), it makes sense to decouple this functionality from the internal scheduling system of theatre, and just expose that. Exactly, in the current implementation, calling the passed update() function just calls tick() synchronously internally, but you don't have to care about that.

Your example code is correct, and it does exactly what you describe - the 'raf' and 'xrRaf' options are just there for convenience, since I imagine 99% of the time people would want to use one of the two.

As for different drivers for different sequences: drivers (and the ticker) are part of theatre's scheduling system. They need to update at regular intervals, otherwise nothing will work. Plus studio and core need to use the same scheduling, otherwise things can break.

The use case of synchronizing theatre's internal cycles to the platform's animation frame scheduling system is distinct from the use case of synchronizing the playhead to the scroll position. For the latter use case, we could provide a more convenient way to bind the playhead position to outside data (this could even be a a two-way binding, imagine that), and we could even interpolate the movement of the playhead, so that it would be smooth even on platforms with less granular scroll pos updates. But theatre would still need to schedule to a stable source of ticks, like a raf or an XR raf.

AndrewPrifer avatar Jun 22 '22 14:06 AndrewPrifer

@colelawrence what you mention is valid, in fact the original version of this function only allowed DriverFn, and I explored multiple solutions to make common use cases easier.

The flip side of the coin, and why I decided to go with what we have here, is that raf and xr account for pretty much all the current uses cases of this function. In fact, I'd be hard pressed to think of a case when one might want to implement a custom driver. And when (let's just say it out loud :D) 100% of the time someone will either want raf, or xr raf, it seems a little silly to make them jump through hoops to get there imo, but I don't hold this opinion too strongly, so if you're confident we should explore alternatives, I'm happy to accommodate that.

AndrewPrifer avatar Jun 22 '22 20:06 AndrewPrifer

In fact, I'd be hard pressed to think of a case when one might want to implement a custom driver. And when (let's just say it out loud :D) 100% of the time someone will either want raf...

While a majority of WebGL applications do use rAF loops, that's not the same as wanting theatre to run its own rAF loop. The application may be composed of many parts (renderer, physics engine, animation system, profiler, ...) that all need to update at the right time within a single animation loop. So typically I wouldn't want one component (e.g. theatre.js) running an animation loop independently.

They need to update at regular intervals, otherwise nothing will work. Plus studio and core need to use the same scheduling, otherwise things can break...

I totally get this in the context of /studio, the UI needs to be updated regularly and it doesn't make sense for the /studio UI to be tied to scroll events or whatever. But I'm not so sure it makes sense for a deployed production application (using just the /core module) to have this scheduling constraint — if nothing is happening, do we really need to keep a rAF loop going? Being able to render at an adaptive framerate is useful, I think I'd generally want a production app to have a very predictable render loop:

function render (t) {
  // (1)
  animationSystem.update(t);

  // (2) 
  physics.update(t);

  // (3) 
  renderer.render(scene, camera);

  // (4) 
  profiler.update(t);

  // 

  requestAnimationFrame(render);
}

More concretely — I wouldn't want theatre doing extra work and updating the scene at 60fps in production mode if the rest of my application has downgraded to rendering at 20-30fps because the device can't keep up.

Does this make sense, or am I mis-matching concepts? I think I'd seen that setting sheet.sequence.position (is this the "playhead"?) causes theatre.js to flush changes to objects synchronously — if that's the case then maybe I'd just do that in my render loop above, and theatre.js's own rAF is unrelated. But the name 'animation driver' leads me to assume more work is happening in the rAF too...

donmccurdy avatar Jun 22 '22 21:06 donmccurdy

Just seconding @donmccurdy, the use cases for this are, in order of importance:

  1. scheduling animation updates in relation to other work in the application's own main render loop
  2. flushing animation updates synchronously on demand
  3. using another animation frame like XRSession. But if this applies, use case 1 probably also applies.

It looks like this solution would work, but it seems unnecessarily awkward when all we need is:

  1. a method to flush updates on demand
  2. a way disable the default rAF

Should I be forced to apply a custom animation driver if all I want to do is flush animation updates? That should just be a method call.

To illustrate a use case for flushing updates, imagine I want to animate a transition between two points on a timeline. These steps would need to be executed within a single animation frame:

  • jump playhead to A, flush theatre.js
  • render scene into buffer A
  • jump playhead to B, flush theatre.js
  • render scene into buffer B
  • combine buffer A and B and render to screen

I think I'd seen that setting sheet.sequence.position (is this the "playhead"?) causes theatre.js to flush changes to objects synchronously

Setting sheet.sequence.position does not flush any changes. With #88 the default ticker is exposed so you can flush via ticker.tick(). Maybe instead, we could expose Theatre.core.flush() and not expose the ticker?

felthy avatar Jun 22 '22 23:06 felthy

@donmccurdy yes, you are understanding the concept of drivers correctly, the use case you are describing with downgrading frame-rate and having a predictable render loop in the application as a whole is exactly what I'm trying to achieve. You filled in exactly the missing piece with the need to orchestrate animation updates as part of a larger system. I guess the concept of driver functions, while easy to use when integration is not a concern, get in the way when you just want to be able to tell the animation system "flush your updates now". I think for this case having just a simple update(t) function would be more convenient (which would flush updates synchronously).

In this scenario, you would disable theatre's default raf driver and just keep calling update() from then on.

I will have to look into the behavior of sheet.sequence.position and whether it flushing changes synchronously is intentional and/or stable.

In any case I'd just like to reiterate that the goal of this proposal was more to explore how we could make things both more convenient and reduce the surface area of the additional API we are exposing, not to enforce any particular way of doing things. :)

AndrewPrifer avatar Jun 22 '22 23:06 AndrewPrifer

@felthy thank you for the additional context, @donmccurdy already thoroughly convinced me that just exposing the concept of drivers, while they give a name to what theatre is doing with the default raf loop, makes integrating the update loop unnecessarily awkward (see my comment above). :) I'll update the PR with the flush method.

AndrewPrifer avatar Jun 22 '22 23:06 AndrewPrifer

Right. Then, if you've exposed a flush method, the “driver” concept becomes unnecessary. You just need a way to disable the default theatre.js rAF, then your application can call the flush method from its own rAF (or whatever).

felthy avatar Jun 23 '22 00:06 felthy

Not super convinced yet that the driver concept is entirely unnecessary, at the very least it gives a proper name and utilities to handle theatre's default raf loop and loops of the like as distinct from the ticker, which for lack of a better word, we've just been calling the "default ticker" till now. I think mixing it in with the ticker is a little confusing, because the raf loop is not the ticker, the ticker is the ticker. :)

I do fully agree on the point of needing to expose a flush() function though, all the rest is just an experiment.

Changed the title to reflect the priorities. :D

AndrewPrifer avatar Jun 23 '22 00:06 AndrewPrifer

Thanks @AndrewPrifer and @felthy! Sounds like we're on the same page so far. Setting aside API for a moment, would the general idea be ...

  1. when /studio is being used, update() (or whatever) should run at something like 60fps to keep the UI going
  2. when only /core is being used (e.g. production), update() should run just before the application itself is about to render, at whatever rate that might be

..., correct? I suppose that could be split into a separate ticker for /studio, perhaps.

donmccurdy avatar Jun 27 '22 20:06 donmccurdy

I've just had a look through the code, and my understanding is that the existing studio ticker drives things that react to the studio state, like any onSelectionChange() handlers you add, and re-drawing the frame grid when the sequence changes.

So I don't think there's a use case to expose the studio ticker at all. Even when using the studio, I'd still want my prop changes to be flushed on my application's own schedule, and the core ticker does that, even when using the studio. So exposing a single update() from core is all we need, and the application can call that as it chooses, perhaps at a reduced frame rate, whether using the studio or not.

felthy avatar Jun 28 '22 00:06 felthy

Hi @donmccurdy and @felthy, not sure if you saw the commit, in the meantime I exposed a flush() function (naming is hard, this might change :)). This will flush pending animation updates and can be called in your frame loop.

AndrewPrifer avatar Jun 28 '22 00:06 AndrewPrifer

@donmccurdy your thinking is correct, I think for now, without introducing more abstractions, the easiest way would indeed be if you had a dev/production check and only downgraded frame rate if studio is not in use.

I'm really curious how we could (or if we should) make this easier/more obvious.

AndrewPrifer avatar Jun 28 '22 00:06 AndrewPrifer

Thanks @AndrewPrifer, I think the flush() function is good and I'm comfortable with that name. Given that we need to expose flush() then, it follows that we only need to expose one further method, perhaps call it cancelAnimationFrame(), to achieve all of our needs. We don't need a more complex API than that.

  • If you want to keep the AnimationDriver as an internal API, I'd still argue against that because the existing Ticker code is simple and self-explanatory, and AnimationDriver doesn't add any value or solve any problems
  • Theatre.js doesn't need to support XRSession. That can be done easily in the application using flush() and cancelAnimationFrame()

felthy avatar Jun 28 '22 00:06 felthy

@felthy I think we still disagree on the driver. As I said, it just serves to give a name to our ad-hoc "default" raf loop. Exposing a theatre.canceAnimationFrame function would be a similarly ad-hoc, and unbalanced API. Where is theatre.requestAnimationFrame? And if we introduce that, then we just have a less pure version of the driver API that only supports managing raf loops IMO.

On the point of theatre not needing to support xrraf because it's easy to reproduce in user-land, the same thing could be said for the regular raf. Yet, we have it because we want to make getting started convenient.

I do appreciate your input a lot though and while this API might not reflect it, I'll make sure we take it into consideration going forward. :)

AndrewPrifer avatar Jun 28 '22 09:06 AndrewPrifer

@donmccurdy @felthy I have some good news, it seems that studio and core don't necessarily have to use the same ticker, it's just that currently there seem to be some edge-cases that prevent us from doing that. Separate tickers for the two (or for anything in dataverse) should entirely be possible. This means that in the future we could expose a more powerful API that is closer to the metal. This current conversation assumes that we want to hide the Ticker (which for now we do), but it might not (and does not have to) be the case for the future. :)

AndrewPrifer avatar Jun 28 '22 09:06 AndrewPrifer

@AndrewPrifer just want to point out that for the use case of flushing animation updates synchronously (point 2 in my earlier comment), exposing a method to flush the core ticker is not sufficient when using multiple projects - because it will flush updates for all projects rather than just the project of interest.

I'm handling this in my PR #88 by allowing user code to create its own Ticker and pass that to core.getProject() (and also studio.initialize() for symmetry).

Can you consider a solution for isolating flush() to a single project?

felthy avatar Oct 10 '22 02:10 felthy

@felthy: Can you consider a solution for isolating flush() to a single project?

Yes we could make it granular and specific per-project or even per-listener. The only thing holding it back right now is that there is a perf-gain opportunity if flushing the listeners happens on a predictable loop (for example a raf loop). The perf-gain would come from doing a topological sort of the dependency tree of the derivations, which would save the roundtrip that happens between _markAsStale() <-> getValue(). In other words, if we know that no derivation is going to be read in-between a raf callback, we can recalculate their values faster. But if we expose the flush() which may be called outside of a raf callback, we lose that guarantee.

That's why we're leaning towards more of a driver API rather than the simpler flush() API.

AriaMinaei avatar Oct 17 '22 11:10 AriaMinaei

Thanks for that explanation @AriaMinaei. Unfortunately, an interface that doesn't allow us to flush updates synchronously, at any time, won't meet our needs. Active Theory has already launched websites that depend on this (using a fork with #88), which do animations like I outlined in my earlier comment.

In other words, we have a requirement to move the playhead and flush more than once within a single browser-invoked animation frame callback.

Putting the studio aside, as a user I want theatre.js to be an engine to update all my props at once, based on a single argument, the playhead position.

With that in mind, even the fact that the current release of theatre.js defers updates until the next frame is not ideal for us.

felthy avatar Oct 17 '22 21:10 felthy

@felthy quick update that it looks like we can expose a simple flush()-like API without causing the roundtrip I mentioned which should cover the use-case you mentioned too. Will update this PR soon.

AriaMinaei avatar Oct 18 '22 21:10 AriaMinaei

This one is finally ready, via #374. I'd love to hear what you think @felthy @donmccurdy.

AriaMinaei avatar Jan 12 '23 14:01 AriaMinaei

Closing as #374 is now merged

AriaMinaei avatar Jan 14 '23 14:01 AriaMinaei