Shader Animations w/ Dynamic Types for Shaders and Effects #314
Conversation
- Still need to add comments and maybe refactor a bit
examples/tests/shader-animation.ts
Outdated
|
|
||
| await snapshot({ name: 'animation2 finished' }); |
There was a problem hiding this comment.
Can you remove the space? This goes into the file name
frank-weindel
force-pushed
the
feat/shader-controller-dyn
branch
from
566e662
to
b2a2cf9
Compare
frank-weindel
changed the title
| @@ -1079,7 +1062,7 @@ export class CoreNode extends EventEmitter { | |||
| return false; | |||
| } | |||
|
|
|||
| if (this._shader) { | |||
| if (this.props.shader === this.stage.defShaderCtr) { | |||
There was a problem hiding this comment.
Should this be the opposite?
There was a problem hiding this comment.
Not sure I understand
There was a problem hiding this comment.
Ah yes I see it now.
| type: 'DefaultShader', | ||
| props: {}, | ||
| shader: new UnsupportedShader('DefaultShader'), | ||
| getResolvedProps: () => () => { |
There was a problem hiding this comment.
That looks incorrect, and wasteful as it creates garbage on every invocation.
| super(); | ||
|
|
||
| this.props = { | ||
| ...props, | ||
| parent: null, | ||
| texture: null, | ||
| shader: null, | ||
| shader: stage.defShaderCtr, |
There was a problem hiding this comment.
Something odd around shaders initialisation:
propsare supposed to already have default values forshader,parent,texture, etc. (though it hassrc: null),this.propshas a spread ofpropsbut overridesshaderwithstage.defShaderCtr,- a few lines below
this.shader = props.shader, which setsthis.props.shaderusing the setter.
There was a problem hiding this comment.
props do already have default values assigned prior to CoreNode construction. The things happening in the constructor that you are calling out are to ensure that the setters are run for those particular properties. If the props weren't overridden in the spread then the setters would not actually run because of the "change check" at the top of each one.
But there is actually no reason at this point to do this for shader so I will remove it.
|
|
||
| set shaderProps(value: Record<string, unknown> | null) { | ||
| this.props.shaderProps = value; | ||
| if (value === this.stage.defShaderCtr) { |
There was a problem hiding this comment.
setting a custom shader doesn't change update type?
There was a problem hiding this comment.
Yep not sure what I was thinking there.
| const shaderPropKeys = Object.keys(props.shaderProps!); | ||
| const spLength = shaderPropKeys.length; | ||
| let j = 0; | ||
| for (; j < spLength; j++) { |
| this.progress, | ||
| startValue, | ||
| endValue, | ||
| const dynEntries = Object.keys(this.dynPropValuesMap); |
There was a problem hiding this comment.
Considering how rare the shader animations are going to be, it might have been more efficient to skip entirely this kind of code by having for instance this.dynPropValuesMap be undefined by default.
Added a new implementation for animating shaderProps.
Added ShaderController & DynamicShaderController functionalities. This allows us to change properties of shaders during runtime.
Optimised DynamicShader prop calculations, by having the DynamicShaderController do the calculations, which previously was done every time a node+dynamic shader where used in a renderOp.