Adding decision document around new animation api #156

runemadsen · 2022-09-27T13:38:33Z

You can read a nicer layout by visiting the markdown file directly.

netlify · 2022-09-27T13:38:38Z

✅ Deploy Preview for dsi-logo-maker ready!

Name	Link
🔨 Latest commit	`de8afea`
🔍 Latest deploy log	https://app.netlify.com/sites/dsi-logo-maker/deploys/6343dda2ac917b0008edb90c
😎 Deploy Preview	https://deploy-preview-156--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.

lucasdinonolte

I think this is a very nice proposal already. My only real remark would be that we should ship drawLoop utils a user can import when they are using the new customAnimation behavior. So they only need to write their own drawLoops if they really want to.

decisions/01-new-animation-api.md

lucasdinonolte · 2022-09-28T07:34:58Z

decisions/01-new-animation-api.md

+export const handler = async ({ inputs, frame, done, useCanvas }) => {
+  const canvas = useCanvas(inputs.width, inputs.height);
+  let frameCount = 0;
+  const myDrawLoop = () => {


I like this. Maybe the drawloop could still just come as a helper from the core.

I agree. That would be very nice!

lucasdinonolte · 2022-09-28T07:37:59Z

decisions/01-new-animation-api.md

+export const handler = async ({ inputs, frameCount, done }) => {
+  // drawing code
+  if (frameCount >= 100) {
+    done(svgString);


Just thinking about this. This forces users to keep svgString in a variable so it can either be returned or be send to done. While this isn't necessarily a problem as you have to the same using frame and done currently I think it might feel weird as return encourages to just build the return value there (at least to me).

So maybe the API could be simplified, that in animation mode done just flips a boolean in the engine that tells the engine to stop rendering after the next return value is received.

I was actually just thinking about this. I think there's some weirdness around whether the user needs to pass something to the callbacks or not. So I like the idea of animation removing the need to do that and customAnimation requiring you to do that. We just need to keep in mind the difference that in animation mode, the done function will be called before the return, thus the last frame will not be recorded as a part of the animation, whereas it will be when you pass the string to the done function in customAnimation.

Oh good point. So the logic should be

done flips a boolean flag in the engine

the next return value is awaited

the return value is passed on to mechanic.done to make sure the last frame is captured too

That, or we change it so the done function never receives anything and always ends the animation immediately. That way, the signature for the frame and the done function will stay consistent, and the different modes are simply about whether you use the frame function or not. I think I like that slightly better.

lucasdinonolte · 2022-09-28T07:44:17Z

decisions/01-new-animation-api.md

+};
+```
+
+The example above re-renders the component for every frame. If a user wants to use something like `react-spring` to speed up animation while keeping the frame-based approach, it could be done like this:


Just leaving this here to consider when implementing this:

I think the way the react engine currently works this would cause a re-render of everything every time, because the engine would call React DOM's render over and over again. When implementing this using the the new animation approach the engine should probably be re-build to render a wrapping component only once and then use a drawLoop within that parent component to re-render to function over and over again.

Yes, I agree. We need to take advantage of the React render loop, so the component should not render from scratch using render every time. good point!

lucasdinonolte · 2022-09-28T07:45:23Z

decisions/01-new-animation-api.md

+
+### `engine-p5`
+
+For `engine-p5`, `mode` can only be set to `static` or `customAnimation`. Or perhaps we default both `animation` and `customAnimation` to the code below?


I like this. Maybe a descriptive info could be printed on the console, to educate developers about why the new animation isn't possible using p5

lucasdinonolte · 2022-09-28T07:46:11Z

decisions/01-new-animation-api.md

+
+The main disadvantage I can see is:
+
+- Users who are not comfortable writing pure functions are now defaulted into this, and if they opt out, they need to write their own animation code.


I think this disadvantage can be mitigated by shipping animation loop utils for each engine that a user can just import into their function.

Co-authored-by: Lucas Nolte <hello@lucas-nolte.com>

runemadsen · 2022-09-28T07:57:14Z

Great! After thinking about it, I'm not a big fan of the customAnimation mode name. I'm wondering if we can rename it to something that makes more sense, but I don't really have a good idea yet. static and animation still seem good to me.

fdoflorenzano · 2022-09-27T20:38:20Z

decisions/01-new-animation-api.md

+The new animation API comes with a few changes to the settings:
+
+- The `animated` setting is renamed to `mode` (default: `static`) and the options are `static` (for static images), `animation` (for the new animation API) and `customAnimation` (for a user to implement their own animation code using the `frame` and `done` callbacks).
+- `frameRate` (default: `60`) is a number that can be used to change the number of frames per second that the design function is called in `animation` mode.


Why only in animation mode?

Currently when creating video, we use a WebMWriter that receives a frameRate param. And that can be used on both modes, I think.

fdoflorenzano · 2022-09-28T15:42:17Z

decisions/01-new-animation-api.md

+
+```js
+export const handler = async ({ inputs, frameCount, done, useCanvas }) => {
+  const canvas = useCanvas(inputs.width, inputs.height);


I'm not sure I love the "use" prefix here. To me that's a React specific convention, and this specific example isn't React. I would try to expose arguments and functions that live in the same universe at the current engine.

fdoflorenzano · 2022-09-28T15:44:34Z

decisions/01-new-animation-api.md

+export const handler = async ({ inputs, frameCount, done, useCanvas }) => {
+  const canvas = useCanvas(inputs.width, inputs.height);
+  if (frameCount >= 100) {
+    done(canvas);


Maybe this is very engine specific, but does done have to receive the canvas? what happens with the one returned right after? I guess that's something that sticks out to me in this new way of writing design functions.

EDIT: now I see you commented on this too in another section. I agree with something like that should also be applied here.

fdoflorenzano · 2022-09-28T15:55:16Z

Great! After thinking about it, I'm not a big fan of the customAnimation mode name. I'm wondering if we can rename it to something that makes more sense, but I don't really have a good idea yet. static and animation still seem good to me.

Since they are strings anyways, I was thinking on going with "custom-animation" really, but yeah maybe it's not great. I'm honestly fine with it. Maybe a separate setting only applicable for "animated", that ahs values:"frame-based" and "event-based", so we start this animation basis language in the code too.

runemadsen · 2022-09-29T11:07:19Z

Thanks @fdoflorenzano! I think both of those could work. If we keep the single mode setting, then perhaps static, animation and animation-custom?

lucasdinonolte · 2022-09-30T11:58:13Z

I updated #152 with a draft implementation showing the new engine behavior for the two new animation modes.

A few observations:

Rewriting the frame and done methods as discussed above removes the need for treating them mutually exclusive in an if-else-block.
- you only need to wrap done in an if clause, but can call frame however you want, as done will make sure to exit the function after the next call of frame
- this logic is needed to correctly capture the last frame
Refactoring settings from animated?: booelan to mode?: 'static' | 'animation' | 'animation-custom' is a breaking change to existing design functions
- how do we want to communicate this? Is it docs only? Or should we add a checkDeprecations method to the mechanic core, that can run on initialization and check for any deprecated settings being present?
Additionally I just realized this should probably change the way static functions are handled too. I think static functions can always be thought of as pure, so I'd argue for them being treated similar to animation functions. But I think we can even go one step further here and say, that they don't even need to call done but can just return whenever they are done drawing.

runemadsen · 2022-10-10T08:54:20Z

@fdoflorenzano I have edited the doc and I think it's ready for you to take a look :)

fdoflorenzano · 2022-10-11T20:56:33Z

@runemadsen I like it! A couple of thoughts came to mind:

Is there an animated setting or similar in the settings of a design function? I didn't see any in the examples. Maybe having an example for the static case will help this feel complete.
For an animation DF, if I understood correctly, the api let's the user not use the drawLoophelper, right? Maybe adding an example of that too would help exemplify the flexibility aspect of the setup.

adding decision document around new animation api

4e16098

runemadsen requested review from lucasdinonolte and fdoflorenzano September 27, 2022 13:38

lucasdinonolte reviewed Sep 28, 2022

View reviewed changes

Update decisions/01-new-animation-api.md

ccde6f9

Co-authored-by: Lucas Nolte <hello@lucas-nolte.com>

fdoflorenzano reviewed Sep 28, 2022

View reviewed changes

lucasdinonolte and others added 4 commits October 7, 2022 11:11

update decision log with experiences of implementing this

880617c

update docs

f2f9255

Merge branch 'main' into decision-new-animation-api

10f7bbe

update decision doc

de8afea

lucasdinonolte mentioned this pull request Oct 18, 2022

New Animation API #157

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding decision document around new animation api #156

Adding decision document around new animation api #156

runemadsen commented Sep 27, 2022

netlify bot commented Sep 27, 2022 •

edited

lucasdinonolte left a comment

lucasdinonolte Sep 28, 2022

runemadsen Sep 28, 2022

lucasdinonolte Sep 28, 2022

runemadsen Sep 28, 2022

lucasdinonolte Sep 28, 2022

runemadsen Sep 28, 2022

lucasdinonolte Sep 28, 2022

lucasdinonolte Sep 28, 2022

runemadsen Sep 28, 2022

lucasdinonolte Sep 28, 2022

runemadsen Sep 28, 2022

lucasdinonolte Sep 28, 2022

runemadsen Sep 28, 2022

runemadsen commented Sep 28, 2022

fdoflorenzano Sep 27, 2022

fdoflorenzano Sep 28, 2022

fdoflorenzano Sep 28, 2022

fdoflorenzano commented Sep 28, 2022

runemadsen commented Sep 29, 2022

lucasdinonolte commented Sep 30, 2022 •

edited

runemadsen commented Oct 10, 2022

fdoflorenzano commented Oct 11, 2022


		### `engine-p5`

		For `engine-p5`, `mode` can only be set to `static` or `customAnimation`. Or perhaps we default both `animation` and `customAnimation` to the code below?


		The main disadvantage I can see is:

		- Users who are not comfortable writing pure functions are now defaulted into this, and if they opt out, they need to write their own animation code.

Adding decision document around new animation api #156

Are you sure you want to change the base?

Adding decision document around new animation api #156

Conversation

runemadsen commented Sep 27, 2022

netlify bot commented Sep 27, 2022 • edited

✅ Deploy Preview for dsi-logo-maker ready!

lucasdinonolte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

runemadsen commented Sep 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fdoflorenzano commented Sep 28, 2022

runemadsen commented Sep 29, 2022

lucasdinonolte commented Sep 30, 2022 • edited

runemadsen commented Oct 10, 2022

fdoflorenzano commented Oct 11, 2022

netlify bot commented Sep 27, 2022 •

edited

lucasdinonolte commented Sep 30, 2022 •

edited