[WIP] Mechanic Draw Loop for FPS throttling by lucasdinonolte · Pull Request #152 · designsystemsinternational/mechanic

lucasdinonolte · 2022-09-19T15:55:27Z

This explores the possibilities of adding FPS throttling by introducing a drawLoop util as part of Mechanic core.

What this does

Adds new frameRate setting to design functions
- frameRate is used to by the WebM exporter
- frameRate is used to build drawLoop function that is exposed to the function's engine
Wraps rendering in engines in drawLoop
- drawLoop calls handler over and over again
- current frameCount is passed to the handler
- animation stops as soon as done is called from within the handler
- in this initial setup it is up to the developer to build the logic for when an animation should stop within their functions
Adds examples canvas-fps, react-fps, svg-fps and p5-fps
- examples are really just for the deploy preview and will be deleted in the future

Observations

In the current set up the drawLoop forces the Canvas Engine to rebuild a new canvas on every frame
- this could be solved if the canvas is handed to the user by the engine as suggested in [WIP] Add export density to canvas renderer #146
This changes the mental model of design functions a bit, as this now makes them pure functions of the current frameCount (this is in line with the React mental model, but opposes the p5 mental model)
- There could be shared state by defining variables outside of the handler
- However there currently is no solution for shared state that depends on an input
  - This could be achieved by bringing memoization helpers to mechanic...
  - ... or maybe also by enforcing a bit more structure in the functions, splitting them into something resembling p5's setup and draw functions
I guess the p5 engine is the "odd one out" here, as p5 brings it's own drawLoop and frameRate utils. In this approach I just made sure to pass the correct frameRate to p5 and left the rest to p5

Todo

netlify · 2022-09-19T15:55:31Z

✅ Deploy Preview for dsi-logo-maker ready!

Name	Link
🔨 Latest commit	`11c0e8f`
🔍 Latest deploy log	https://app.netlify.com/sites/dsi-logo-maker/deploys/633d70e5873ab70009da562e
😎 Deploy Preview	https://deploy-preview-152--dsi-logo-maker.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

runemadsen · 2022-09-20T09:21:14Z

I think this is really lovely and a very nice upgrade for the design function API. Let's discuss with @fdoflorenzano when we find a time.

Making sure there always is only one instance of the draw loop at any given point in time allows us to stop the current draw loop before requesting a re-run of a function when input values changed.

fdoflorenzano

I'm leaving just little remarks of stuff I noticed. I wanna give it another round of thought still.

But my initial impression is that it does look nice, but I worry it does seem to impose itself as implementation and kinda affect other options of animation. Are we looking to make a breaking change and define how to work with animation in Mechanic, or do we want to add an opt-in API?

fdoflorenzano · 2022-09-22T15:38:49Z

+    draw();
+  }
+
+  maybeDisptach(cb, isAnimated = false) {


Suggested change

maybeDisptach(cb, isAnimated = false) {

maybeDispatch(cb, isAnimated = false) {

fdoflorenzano · 2022-09-23T18:59:20Z

+  turns: {
+    type: 'number',
+    default: 3,
+  },


This input isn't being used anymore. Same happens in new examples.

fdoflorenzano · 2022-09-23T19:01:14Z

+  text: {
+    type: 'text',
+    default: 'mechanic',
+  },


This input isn't being used anymore.

fdoflorenzano · 2022-09-23T19:01:55Z

+  text: {
+    type: 'text',
+    default: 'mechanic',
+  },


This input isn't being used anymore.

runemadsen · 2022-09-23T19:43:23Z

@fdoflorenzano I think this is a great observation and something we should discuss on Monday. A good example is p5.js vs. React. I personally think it makes sense to make this change for React, since it's a big mess to make your own draw loop, but for p5.js that already has its own setup and draw it makes less sense. Perhaps it's something that's up to each engine? I can see a scenario where the default setting for the react engine is that it works like in this PR, but can be disabled with a setting. I'm in favor of making it really simple to code animations, but giving users the ability to do custom fps handling if needed.

lucasdinonolte · 2022-09-23T20:30:39Z

I think it's more than the difference between p5 and react. Fernando is right, this change would enforce the frame based way of animating. Which is also something I'm concerned (or at least unsure) about.

With this change each frame of an animation is a function of the current frame count. There is no shared state between frames and every calculation has to be made based on frame number if you need it to be deterministic.

While this approach feels natural to me (it's similar to how video editing software works) it isn't the only approach to think about animation. You can also think of them time based (seconds instead of frames) or event based (after x turns of the circle stop the animation). Especially the event approach might feel more natural to people coming to mechanic from a web development background.

In another exploration of the fps throttling I exposed the draw loop to the design function (instead of the frame count) and made it the user's job to use it in their function. I discarded this idea because at the time it felt like just a different way of having useDrawLoop in your function.

But now I'm thinking that maybe we should reconsider this approach as this would allow to also work with time and event based animation mental models as a user can decide what should be setup code and what should be run on every frame iteration.

Looking forward to discussing this on Monday 😊

runemadsen · 2022-09-24T07:49:10Z

My main concern with this new feature is to provide a simple API that users can easily pick up. My experience making libraries tell me that the makers of libraries are often concerned about flexibility while users just want ease of use. But maybe this thing of exposing the draw loop is a good one if we can do it in a simple way. Yes, let's discuss on Monday.

…anic core

lucasdinonolte · 2022-09-30T16:49:57Z

-      done: onDone,
-      state: mechanic.functionState,
-      setState: onSetState,
+  mechanic.dispatch({


I feel like I might have taken the abstraction too far here. But I kind of like having this centralized entrypoint for engines.

Not sure about the object that's passed to dispatch. I could also imagine this could be clean by registering the callbacks and then move frameOrDone also to core.

mechanic.registerFrameCallback((el) => ...); mechanic.registerDoneCallback((el) => ...);

We need to keep a reference of each engine's helpers in the core at runtime so the automatic return value frame capturing for static and animation works using maybeAddFrame.

Let me know what you think 😊

I just had some time and tried rewriting this to use register style functions. I think it's much more readable like this and provides a nice shared way to add the information to core that core needs to correctly control the draw loop

fdoflorenzano

Sorry for the delay in reviewing this. I did a pass on Monday and wanted to do another fresh one to make sure I was understanding everything, and with other projects of course that got pushed a bit.

I like how the general structure for the design functions and look like in now (the register of callbacks specially). One little detail that I wanted to mention is that I don't love the return for every frame aspect for design functions, I generally prefer a more explicit API for setting up frames and everything. That and other ideas that I've seen tossed around lately make me feel that design functions will start looking more and more like react components, which to me is to make the API too opinionated.

Where I think there's room for improvement, is with the general design and naming of functions for the main flow of execution in Mechanic core. Part of why I've struggled to make a review is because I'm having a hard time understanding the current structure and which part calls what and so on. There's so many callbacks and functions and not great names (maybe) for some of them that it's easy to get lost. I would be happy to propose or work with you in a more streamlined flow, but I would need a bit of time to think and do it.

I hope all this makes sense. We can talk about it tomorrow!

fdoflorenzano · 2022-10-03T15:27:18Z

+  /**
+   * @return {boolean} - Returns true if the design function should use the controlled drawloop
+   */
+  shouldUseControlledDrawloop() {


Suggested change

shouldUseControlledDrawloop() {

shouldUseControlledDrawLoop() {

fdoflorenzano · 2022-10-03T15:27:37Z

+   */
+  getDrawLoopHelper() {
+    return cb => {
+      if (!this.animated || this.shouldUseControlledDrawloop()) {


Suggested change

if (!this.animated || this.shouldUseControlledDrawloop()) {

if (!this.animated || this.shouldUseControlledDrawLoop()) {

fdoflorenzano · 2022-10-06T19:43:55Z

+### Added
+
+- Added frameRate setting to function’s settings. This controls the export frameRate


Haha I think there's a bit more here now. A little reminder to update when finished!

fdoflorenzano · 2022-10-06T20:26:29Z

+  mechanic.registerFinalizeCallback(async (el) => {
+    mechanic.drawLoop.stop();
    root.innerHTML = el.trim();
    if (!isPreview) {
      await mechanic.done(root.childNodes[0], { head });
      mechanic.download(name || functionName);
    }


The name argument disappeared here, and all other engines I think.

lucasdinonolte · 2022-10-07T07:47:41Z

Sorry for the delay in reviewing this. I did a pass on Monday and wanted to do another fresh one to make sure I was understanding everything, and with other projects of course that got pushed a bit.

I like how the general structure for the design functions and look like in now (the register of callbacks specially). One little detail that I wanted to mention is that I don't love the return for every frame aspect for design functions, I generally prefer a more explicit API for setting up frames and everything. That and other ideas that I've seen tossed around lately make me feel that design functions will start looking more and more like react components, which to me is to make the API too opinionated.

Where I think there's room for improvement, is with the general design and naming of functions for the main flow of execution in Mechanic core. Part of why I've struggled to make a review is because I'm having a hard time understanding the current structure and which part calls what and so on. There's so many callbacks and functions and not great names (maybe) for some of them that it's easy to get lost. I would be happy to propose or work with you in a more streamlined flow, but I would need a bit of time to think and do it.

I hope all this makes sense. We can talk about it tomorrow!

Thanks for taking the time to review this explorative implementation. I know the names are a bit off and there probably is one or two callbacks too many. So I'd love your help to turn this into clear and understandable code.

Having spent a lot of time exploring this approach and also refactoring parts of DSI-Logo-Maker to this new setup I must say I fully share your concerns. Working with return values instead of explicitly calling frame or done looks very cool at first and works very nicely in the simple examples I initially created to implement this new animation API.

The tricky parts

Refactoring DSI-Logo-Maker however showed the shortcomings of this approach: You don't just force people into a more React-like mindset as Fernando pointed out, you also lose a lot of flexibility and make things harder. This is not just true for the return based approach, but also for the idea of having the drawloop inside mechanic core. A few examples:

Return based approach – the tricky parts

Using returns makes it very hard to handle design functions that perform async loading of fonts or images first
- A good example is textAndImageCanvas in DSI-Logo-Maker that even has two output options, based on if an image was loaded or not
- The same is true for React based function's the in some cases return a null'ish value when the component is still loading things and really only want to register a frame when the component has updated

Controlled DrawLoop – the tricky parts

Treating the entire design function as "pure" seems like a good idea too, but thinking about it, they almost always have side effects (like loading fonts, generating random numbers and so on) that they most likely never will be pure
- It feels like a lot of potential performance overhead to re-run the entire function for every frame and not have shared state between frame-iterations
- Random values are hard using this approach, because a new random value is generated on every function run, so I had to bring in the memo feature from another PR to essentially cache random values

Summary: Simple API -> less flexibility

It could be that these issues arose because DSI-Logo-Maker wasn't build with frame-based animation in mind. Nonetheless, the above mentioned are issues of the approach we explored and end-users could very well encounter them, unless we go the fully opinionated route and somehow force everyone into a frame-based stateless approach—which I would disagree with for the same reasons Fernando mentioned.

What struck me the most is, how much flexibility is potentially lost by trying to make the API a bit more comfortable, because a lot of assumptions have to be made, to make the API changes work. And to be honest, explaining to someone they now have to memoize random values feels more confusing then telling them: "Hey, use frame when you want to add a frame to your animation and done when you're ready to export.".

The good parts

But I don't dislike everything I've explored. Moving the drawLoop away from a custom implementation and into something that is provided to each function as another method on the mechanic object (in animation-custom) mode felt really great and like a great simplification of things.

Also knowing that this is fully preconfigured with the correct frameRate and other settings helped reduce and simplify the code in a lot of DSI-Logo-Maker dramatically. The result is a very clear design function, that is clearly separated into the part that does the initial setup and the part that is running in every frame. Now it's up to the user if they want to treat the "every frame" part a s pure function, that only does operations based on the frameCount argument it gets (which is how I'd do it, when starting from scratch) or if they follow a different mindset of animation and manipulate variables in the global scopes within their frame callback. This feels flexible, because both approaches are possible and you can bring much more of what you already know about coding and put it to good use in your design function.

A compromise?

So I think doing this exploration and trying to understand the pros and cons of the different approaches to animation was very valuable. I increasingly get the feeling that the mechanic callbacks (or dare we call them hooks?) like frame and done aren't inconvenient at all, but rather at a level of expressiveness to your code that allows you great levels of flexibility.

After having explored how to remove things from the user's code and make the settings and mechanic core very smart I'd now say: Let's put some code back into the user's code and make mechanic core very simple. I'm proposing we add a third hook to the mechanic object exposed to a function called drawLoop. It takes a function that runs on every frame. If a function calls the drawLoop mechanic-core can know it's animated. If it doesn't mechanic-core will know it's a static function (so this could potentially even remove the animated or mode setting).

// Animated Example (Canvas/SVG)
export const handler = async ({ inputs, mechanic }) => {
  const { canvas, ctx } = mechanic.getCanvas(inputs.width, inputs.height);

  let someVariable = 0;

  mechanic.drawLoop((frameCount) => {
    // Either implement your drawing code as a pure function here
    // or manipulate the parent scope's state if that feels more natural to you
    someVariable += 1;

    // Exit condition is 100% up to the user, so frame-based or event-based exit
    // conditions are equally possible
    if (someVariable >= 100) {
      mechanic.done(canvas);
    } else {
      mechanic.frame(canvas);
    }
  })
};

// Static Example (Canvas/SVG)
export const handler = async ({ inputs, mechanic }) => {
  const { canvas, ctx } = mechanic.getCanvas(inputs.width, inputs.height);

  const someVariable = 0;

  ctx.clear();
  
  // Do some drawing using the value of someVariable here
  mechanic.done(canvas);
};

// Animated Example (React)
export const handler = async ({ inputs, mechanic }) => {
  // Instead of forcing the custom useEffect setup a predefined hook
  // could wrap all of that and just execute on every frame
  mechanic.useDrawLoop((frameCount) => {
    // Do whatever state updates you want to do on every frame here
    if (frameCount >= 100) {
      mechanic.done();
    } else {
      mechanic.frame();
    }
  });
  
  return <svg></svg>;
};

// Static Example (React)
export const handler = async ({ inputs, mechanic }) => {
  useEffect(() => {
    mechanic.done();
  }, []);
  
  return <svg></svg>;
};

lucasdinonolte · 2022-10-18T14:30:00Z

Closing this in favor of a new, clean PR that implements that changes we've agreed upon after using this to explore potential solutions.

lucasdinonolte added 3 commits September 19, 2022 14:23

feat: add framerate to settings

8c0233e

feat: add draw loop util function

94a6a9e

bring fps throttling to all renderers

b0be6ff

lucasdinonolte added the Work in progress label Sep 19, 2022

lucasdinonolte mentioned this pull request Sep 19, 2022

FPS Throttling #125

Closed

lucasdinonolte added 3 commits September 19, 2022 17:57

remove unused maxFrames from default settings

a084895

fix: add engine p5 as dependency for previewing fps functions

83d5345

fix: add engine svg as dependency to deploy preview

ad267fb

lucasdinonolte added 2 commits September 22, 2022 15:34

Merge branch 'main' into mechanic-draw-loop

2d80913

fix: refactor drawloop to singleton pattern

c2a4ce6

Making sure there always is only one instance of the draw loop at any given point in time allows us to stop the current draw loop before requesting a re-run of a function when input values changed.

fdoflorenzano reviewed Sep 23, 2022

View reviewed changes

lucasdinonolte added 3 commits September 29, 2022 21:17

feat: prototype animation api proposal for canvas and react

5048282

refactor: refactor svg engine to new animation model

8f08569

refactor: refactor p5-engine to new frame and done interface

d9e7d2b

lucasdinonolte mentioned this pull request Sep 30, 2022

Adding decision document around new animation api #156

Open

refactor: refactor engines to abstract most animation details to mech…

9283841

…anic core

lucasdinonolte commented Sep 30, 2022

View reviewed changes

refactor: use register style functions to set engine callbacks

11c0e8f

fdoflorenzano reviewed Oct 6, 2022

View reviewed changes

lucasdinonolte closed this Oct 18, 2022

	maybeDisptach(cb, isAnimated = false) {
	maybeDispatch(cb, isAnimated = false) {

	shouldUseControlledDrawloop() {
	shouldUseControlledDrawLoop() {

	if (!this.animated \|\| this.shouldUseControlledDrawloop()) {
	if (!this.animated \|\| this.shouldUseControlledDrawLoop()) {

		### Added

		- Added frameRate setting to function’s settings. This controls the export frameRate

Uh oh!

Conversation

lucasdinonolte commented Sep 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this does

Observations

Todo

Uh oh!

netlify Bot commented Sep 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for dsi-logo-maker ready!

Uh oh!

runemadsen commented Sep 20, 2022

Uh oh!

fdoflorenzano left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

runemadsen commented Sep 23, 2022

Uh oh!

lucasdinonolte commented Sep 23, 2022

Uh oh!

runemadsen commented Sep 24, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fdoflorenzano left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucasdinonolte commented Oct 7, 2022

The tricky parts

Return based approach – the tricky parts

Controlled DrawLoop – the tricky parts

Summary: Simple API -> less flexibility

The good parts

A compromise?

Uh oh!

lucasdinonolte commented Oct 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lucasdinonolte commented Sep 19, 2022 •

edited

Loading

netlify Bot commented Sep 19, 2022 •

edited

Loading