-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Feature/detect outside variables strands #8208
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
Open
Aayushdev18
wants to merge
10
commits into
processing:dev-2.0
Choose a base branch
from
Aayushdev18:feature/detect-outside-variables-strands
base: dev-2.0
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
84832ba
Add detection for outside variable references in p5.strands uniforms
9bb6ec3
Fix outside variable detection: handle optional defaultValue and ance…
795e50c
Fix outside variable detection based on Dave's feedback
4440d6e
Implement proper scope-aware variable detection
2129ca0
Simplify scope checking - ancestor chain naturally excludes sibling s…
1694f2d
Fix tests to match actual function input (just body, not wrapper)
70644dc
Fix outside variable detection in p5.strands
09c3f46
Merge branch 'dev-2.0' into feature/detect-outside-variables-strands
Aayushdev18 9307929
Merge branch 'dev-2.0' into feature/detect-outside-variables-strands
Aayushdev18 cd4b0d0
Merge branch 'dev-2.0' into feature/detect-outside-variables-strands
Aayushdev18 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| import { detectOutsideVariableReferences } from '../../../src/strands/strands_transpiler.js'; | ||
|
|
||
| suite('Strands Transpiler - Outside Variable Detection', function() { | ||
| test('should allow outer scope variables in uniform callbacks', function() { | ||
| // OK: mouseX in uniform callback is allowed | ||
| const code = ` | ||
| const myUniform = uniformFloat(() => mouseX); | ||
| getWorldPosition((inputs) => { | ||
| inputs.position.x += myUniform; | ||
| return inputs; | ||
| }); | ||
| `; | ||
|
|
||
| const errors = detectOutsideVariableReferences(code); | ||
| assert.equal(errors.length, 0, 'Should not error - mouseX is OK in uniform callback'); | ||
| }); | ||
|
|
||
| test('should detect undeclared variable in strand code', function() { | ||
| // ERROR: mouseX in strand code is not declared | ||
| const code = ` | ||
| getWorldPosition((inputs) => { | ||
| inputs.position.x += mouseX; // mouseX not declared in strand! | ||
| return inputs; | ||
| }); | ||
| `; | ||
|
|
||
| const errors = detectOutsideVariableReferences(code); | ||
| assert.ok(errors.length > 0, 'Should detect error'); | ||
| assert.ok(errors.some(e => e.variable === 'mouseX'), 'Should detect mouseX'); | ||
| }); | ||
|
|
||
| test('should not error when variable is declared', function() { | ||
| const code = ` | ||
| let myVar = 5; | ||
| getWorldPosition((inputs) => { | ||
| inputs.position.x += myVar; // myVar is declared | ||
| return inputs; | ||
| }); | ||
| `; | ||
|
|
||
| const errors = detectOutsideVariableReferences(code); | ||
| assert.equal(errors.length, 0, 'Should not detect errors'); | ||
| }); | ||
|
|
||
| test('should handle empty code', function() { | ||
| const errors = detectOutsideVariableReferences(''); | ||
| assert.equal(errors.length, 0, 'Empty code should have no errors'); | ||
| }); | ||
| }); |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 approach is currently hard-coding these identifiers, but they can be almost anything (a custom shader can create its own hooks for other users), so we'll need a different approach. A number of the identifiers in this list don't appear anywhere in any default shaders either, such as
getWorldBinormal, and feel like AI hallucinations. Given the complex nature of this task and the system it's interacting with, I think we really need to have a deeper understanding of what the approach here should be before proceeding with writing code, otherwise it will take a significant amount of back-and-forth in review. Is that something you're up for? If so, we can discuss an approach to how to tackle this (and feel free to reach out in the p5 discord, I'm happy to explain in a less asynchronous way there!) If not, also no worries -- this is a complex issue, and there are others that you can tackle, and we can open this one up to other contributors.