-
Notifications
You must be signed in to change notification settings - Fork 25
Node/Param refactor and addition of Shots WIP #503
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: alpha
Are you sure you want to change the base?
Conversation
@@ -137,7 +137,11 @@ export class HedronEngine { | |||
debugScene.addPass(pass) | |||
}) | |||
} | |||
instance.update({ deltaFrame: 1, params: paramValues }) | |||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this fixed an issue with bad code in a sketch hard crashing the app, but I might be mistaken, would be good to test in isolation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no harm in some extra try/catch
blocks!
@@ -12,9 +13,25 @@ type SketchInstance = { | |||
getPasses?: (engineScene: EngineScene) => Pass[] | |||
} | |||
|
|||
let instance: SketchManager |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really dislike all this (the class needing to be passed the engine, making it a singleton to access it in a way that works with the exported function), though it's better than needing an engine ref elsewhere.
I tried going down the route of a create* function, but I hit a wall with needing to both call a function on the sketch instance, and make a change to the state
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can solve this with React context. We could have an engine provider and then you can get the engine by doing something like const engine = useHedronEngine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for going through and tidying up all the naming! It makes sense to me that params and shots are both nodes.
One issue though: I'm not sure if we should be separating out params
and shots
in the store. Perhaps it makes more sense to have a single nodes
object that holds everything.
The main reason I'm suggesting a single nodes
object is that when we have inputs coming in, they'll have to know which type they are assigned to, and then check against that in order to know which part of the store to look in. I feel like it's better for the input to head straight to the node it's looking for and then deal with the type (e.g. shot/param). This feels more scalable too, in case in the future we have a new node type.
Went down a rabbit hole getting shots working, probably don't want to think about merging until after #502
Makes the split between
Node
andParam
more defined, with bothParam
andShot
being aNode
. Cleared up all the language throughout the project that was using node when it was only working with sketch params.Hedron desktop will display buttons for every shot in the config, behaves as the prior version where you can return an object with your param values to update them in the store.