-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
[p5.strands] Significant refactor for p5.strands #8009
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
[p5.strands] Significant refactor for p5.strands #8009
Conversation
…not p5 defined structs such as Vertex inputs)
Apologies for the wall of text & let me know if any of this seems wrong, it's pretty complex and I'm drawing from compiler theory, but it is a bit unique as we don't parse the code. P.s. I will address the reviews above in code shortly. Overall, this should be more comprehensible than the previous implementation. There are some fine details to be clarified as you mentioned, and it could be that the two graphs need more specific names. Overview:
The DAG doesn't store states of variables perse, as it doesn't track names. We would need to do something like What the DAG does is track data dependencies in a simple form, as in The control flow graph, which is maybe not the most accurate name, indicates that a certain code generation pattern is required for something related to control flow. I.e. output
Yes, exactly. And the example would be like your first one, because the condition has its own block, and the let a = 1
let b = 2
if (b > 1) {
let c = 3
a = c
}
return a We will capture names in the let assignments = If(condtion, () => {
x: x.add(10),
y: x
}).Else(() => {
x,
y,
});
{x, y} = assignments; We could either use this context and introduce a symbol table (this is more 'correct' and probably more stable), or just grab the temp name for the node during codegen and sneakily reassign it. We wouldn't add names too all nodes, only if they're modified in this way. |
…pes to strings for debug, attach API to fn instead of window
Hey @davepagurek, I think this is pretty close now. When you have some time to review, let me know if there are any final changes. Maybe it is good to collapse the changes into fewer new files, for example. |
…n hooks didnt cast from float->vec types
…s logged a warning
|
||
const dim = target.dimension; | ||
|
||
const lanes = new Array(dim); |
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.
are "lanes" like the underlying values for each dimension, which may go under different name aliases? e.g. lane 0 could be x
or r
? (If so maybe add a comment explaining)
src/strands/ir_builders.js
Outdated
return Reflect.set(...arguments); | ||
|
||
for (let j = 0; j < chars.length; j++) { | ||
const canonicalIndex = basis.indexOf(chars[j]); |
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.
Possibly also worth a comment tying this back to the lane definition. e.g. map x
and r
to 0, y
and g
to 1, etc
|
||
target.id = newID; | ||
if (typeof onRebind === 'function') { | ||
onRebind(newID); |
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.
This is the bit that ensures dependency ordering works after reassigning a property right? Maybe comment around here explaining what this is for
get() { | ||
const propNode = getNodeDataFromID(dag, dag.dependsOn[structNode.id][i]) | ||
return createStrandsNode(propNode.id, propNode.dimension, strandsContext); | ||
const onRebind = (newFieldID) => { |
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.
Worth reiterating what rebinding is for here too from the member assignment section
src/strands/ir_types.js
Outdated
}; | ||
|
||
export const StructType = { | ||
Vertex: { |
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.
Ok I think this is actually the last code thing we need: rather than hardcoding these, we need to map from the output of the hook info function to something in this format. There's one failing test and it's due to this
Addresses #7868
Changes:
This (draft) PR is for a significant refactor for p5.strands I've been working on for the past month. Thank you to the contributors in other issues who have been patient in waiting for this update as it has blocked some progress in other areas. And thanks to all in Discord showing an interest too. I would love to get the thoughts of those who have been interested in contributing to p5.strands thus far (or any newcomers). This refactor is all about developer ergonomics for p5.strands:
@LalitNarayanYadav @perminder-17 @reshma045 @pratham-radadiya @ShaunakMishra25 @Orsenna187
At current, the refactor is just missing swizzles, a slight change needs to be made to the transpiler to make Unary operations work. Then, I need to do a once over and remove any extra types etc. which are left over from earlier stages in this refactor.
Overview of the refactor
The main purpose of this refactor is to make it more extendable to WGSL in the future, to modularise for developer ergonomics generally, and to make tests and FES easier to re-implement. It separates concerns throughout the p5.strands architecture and adds a much clearer type system. By modularising the codebase, a more straightforward roadmap and contributor documentation can be written up for p5.strands. More on that at the end of this PR, and I will leave some stubs for new issues related to this.
Entry point
p5.strands is still accessible through the same
p5.Shader.modify
method. The function override for this now exists inp5.strands.js
, however. This file also initialises astrandsContext
object, and also initialises the user API with this context. In the future, this file could potentially overridecreateShader()
.User API
The user API in
strands_api.js
includes all of the hooks, i.e. methods availablep5.Shader.modify()
such asgetWorldInputs()
,getFinalColor()
and so on.It also includes
StrandsNode
, a simplified class as compared to the previous implementation. Previously, the user had handles to classes derived fromBaseNode
. There were between 10-15 of these, each with slightly different methods and data, to handle all edge cases for both operations and also for code generation. This was confusing for developer experience, but also created the problem that it was hard to know where to document strands features, and what to document.The
StrandsNode
class only contains user facing methods, like.add()
,.mult()
, and members for swizzling such as.xyz
,.rrg
etc. Apart from that, it has athis.id
which corresponds to an ID in the compilers Intermediate Representation. More on this later, but overall the user API is less tied to backend specifics now.This file also contains a few more functions like type constructors (
vec3
,float
, also now withivec3
,bool
etc.),strandsIf()
anddiscard()
which are in progress, and (now I'm reminded I need to add this:)instanceID()
as before.Finally, it also pulls in functions from
strands_builtins.js
. These are similar as in the previous implementation, except now with a more robust type system which is explained below. @LalitNarayanYadav, you might be interested in reviewing this and potentially re-portinglerp
here (sorry!) and copyingnoise
across too, which shouldn't need to change!Stages of the compiler
The p5.strands compiler is broken more clearly into separate stages. These are similar, but a bit different, to the classic three stages of a compiler. Previously, These stages were shared between the
BaseNode
class and its children, theShaderGenerator
class, and thep5.Shader.modify()
method. The resulting codebase was becoming difficult to extend, and also difficult to summarise.1. Front-end: Transpile Stage
Overview: Transpiles from the p5.strands 'language' to the JavaScript API.
Files:
strands_transpiler.js
External Dependencies: ESCodegen and Acorn
2. Middle-end: Building the Intermediate Representation (IR)
Overview: Builds graphs which represent the user's code
Files:
ir_dag.js
,ir_cfg.js
,ir_types.js
,ir_builders.js
ir_builders.js
is one step beyond the User API file. The functions in here do most of the heavy lifting in building up the IR graphs. All of the functions in the User API call to here.When the user calls methods like
.add()
orvec3()
, they are returned a user facingStrandsNode
as mentioned above. However, this also builds a node in the IR's directed acyclic graph (DAG), which model data dependencies, and records its existence in the control flow graph (CFG), which models data flow. These graphs are implemented in their_dag.js
andir_cfg.js
files respectively.The users nodes are handles to nodes in the DAG. So this includes variables and operations (that's it for the most part). There are no 'no-ops' at current. Inside of
strandsIf()
, a new 'basic block' is made in the CFG. ThestrandsContext
(via the builder functions) keeps track of the current block, and any user instructions (like a function call or addition) are recorded in the current block.The
ir_types.js
file has a number of pseudo enums and look up tables for different types. These includeBlockType
or basic blocks,NodeType
for variables vs operations (maybe name is too ambiguous now but use if obvious), etc.The most obvious (and complex) of these are
DataType
's which model types such asfloat
,int
and their vector variations. As a shader DSL based in JS, I've arrived at objects with a shape:{ baseType: 'float', dimension: '1', priority: '3' }
etc. Therefore you can compose a final shader type by doingnode.baseType + node.dimension
, which just separates our types from GLSL a bit for down the road.Once the user's code has finished running and all of the graphs are built, we do a topological sort on the CFG. We are able to topo sort because, although there are kind of back-edges in the graph, we don't really need to model
goto
's purely, we just need to output code genif()
in the codegen. This is still a work in progress, however.3. Back-end: Code generation
Overview: Generates GLSL code from the intermediate representation
Files:
strands_codegen.js
,strands_glslBackend.js
This does as it says: generates GLSL code from the IR. We currently do the CFG sort in this section, and create
generationContext
object to store our lines of generated code, and temporary variable names. Next, we loop over the basic blocks and output the code for each visited node.We only have to use some of the same types from the IR, but most of the heavy lifting is already done (as mentioned) and the code output is relatively simpler code. It is similarly structured to Acorn's visitor functions: we define an object with different visitor functions for different node types.
Importantly, the WGSL implementation should be a similarly simple process to add, and could be done by a direct port of the ``strands_glslBackend.js` file.
FES file
I have also disabled and reenabled FES in the
strandsContext
object as before, however I have also added a temporarystrands_FES.js
file here. There are several places in which I have added user errors, but I'm not sure on the best approach for this and have to look more deeply at the rest of FES before overriding it.Next steps / input
strandsContext
could becomeclass StrandsRuntime
or similar:Proxy
objects as in the previous implementation. I don't like attaching hundreds of members ofxyzw
permutations to theStrandsNodes
prototype, what do you think @davepagurek ?strands
folder added to the repo in this PR. How do you feel about that and also naming conventions @ksen0?ir_types
. I'm just not sure whether this will actually optimize anything, or whether the respective backend compiler (GLSL/ WGPU) will do a better job anyway.Will write anything down here as I think of more
PR Checklist
npm run lint
passes