Add detection for outside variable references in p5.strands uniforms #8195
Conversation
- Detect common problematic variables (mouseX, windowWidth, state_, pixelate) in uniform functions - Provide helpful error message when outside variables are referenced - Addresses GSoC issue processing#8172 for better user experience
|
🎉 Thanks for opening this pull request! For guidance on contributing, check out our contributor guidelines and other resources for contributors! Thank You! |
src/webgl/p5.Shader.js
Outdated
| if (initializer instanceof Function) { | ||
| // Check for common outside variable references | ||
| const funcString = initializer.toString(); | ||
| if (funcString.includes('mouseX') || funcString.includes('windowWidth') || |
There was a problem hiding this comment.
I think we'll want to go with a different approach than manually trying to make a list of variable names to exclude. A user could create a variable named anything they want, so we need something more robust to capture that. The test code in the issue was just an example.
We may need to detect variable declarations in the transpilation phase (https://github.com/processing/p5.js/blob/dev-2.0/src/strands/strands_transpiler.js) and then see if there are references to variables not included in that.
There was a problem hiding this comment.
Thanks @davepagurek for the feedback! You're absolutely right - checking for hardcoded variable names isn't robust. I've removed that implementation.
Following your suggestion, I need to implement this during the transpilation phase in strands_transpiler.js. The approach should:
- Track variable declarations during transpilation to build a scope
- Analyze uniform initializer functions to see which variables they reference
- Check if any referenced variables aren't in the strand's scope
- Provide helpful errors
Could you provide some guidance on how to best integrate this with the existing transpiler? Should I add a new callback to the ASTCallbacks object, or would it be better to analyze the AST separately before/after transpilation?
Thanks!
There was a problem hiding this comment.
Since nothing actually needs to be rewritten, this could be a new phase (maybe run before actual transpilation), but feel free to reuse a lot of the structure of the transpiler, since acorn will be a valuable tool in detecting these. You can use AST Explorer and change the AST to Acorn in the header to see how Acorn parses code, which can help you figure out what structures to look for when detecting declarations.
This is also the kind of thing that would really benefit from unit tests so we can see what's covered.
Following reviewer feedback, removed the approach of checking for specific variable names (mouseX, windowWidth, etc.) as it's not robust. Added TODO indicating this should be implemented in strands_transpiler.js during the transpilation phase by tracking variable declarations and detecting references to variables outside the strand's scope.
2 participants
Resolves #8172
Changes:
setDefaultUniforms()method inp5.Shader.jsp5._friendlyError()when outside variables are referencedThe implementation detects when users reference variables not defined within the shader's scope in p5.strands callbacks and provides clear feedback, improving the debugging experience for users working with shader uniforms.
Screenshots of the change:
N/A - This is a backend error detection feature
PR Checklist
npm run lintpasses