镜像站点 · 本页由第三方 GitHub 只读镜像提供,非 GitHub 官方站点,不接受任何登录或凭据输入。前往 github.com
Skip to content

feat: Re-enable FES in p5 strands - #9209

Draft
Ayush4958 wants to merge 1 commit into
processing:mainfrom
Ayush4958:enabling-fes-in-strands
Draft

Ayush4958 wants to merge 1 commit into
processing:mainfrom
Ayush4958:enabling-fes-in-strands

Conversation

@Ayush4958

Copy link
Copy Markdown

Resolves #7899

Changes:
This PR re-enables FES for p5.strands while resolving the false negative errors that previously forced it to be disabled.

  • src/strands/p5.strands.js: Re enabled FES by dynamically tracking when a strands compilation context is active.
  • src/friendly_errors/param_validator.js: Updated FES validation logic to support StrandsNode arguments, eliminating false negatives for functions like sin() that wrap parameters in dynamic nodes. Also added a FES bypass for shader builder functions so they can handle and display their own internal error messaging.
  • src/strands/strands_api.js: Exported GLSL specific function signatures (e.g., atan() taking 1 or 2 arguments) so FES correctly validates and permits valid GLSL syntax without throwing warnings.

Screenshots of the change:

Code Used for Testing :-

let myShader;
function setup() {
  createCanvas(400, 400, WEBGL);

  myShader = buildMaterialShader(() => {
    let offset = sin(frameCount * 0.01);
    let angle = atan(10, 5); 
    color(offset, angle, 0.5);
  });
}

function draw() {
  background(220);
  shader(myShader);
  sphere(100);
}

Before
image

After
image

PR Checklist

  • npm run lint passes
  • [Inline reference] is included / updated
  • [Unit tests] are included / updated

@bhabishnu bhabishnu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested this locally and it seems to work as expected for me. valid strands values with sin() and 2-arg atan() no longer hit false FES errors, while an invalid 3-arg atan() still gives the friendly parameter error.

I didn't notice any new tests for this path in the diff. maybe it'd be worth adding coverage for these cases?

@aashu2006 aashu2006 left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @Ayush4958 I tested it locally and have found few things

1. This turns off FES for a bunch of normal functions (the main issue)

The sig === true bypass runs even when we are not inside strands. So map, lerp, random, noise, millis, color, texture, red, hue, float, int etc. lose param validation in regular sketches

map('a', 'b');
// main: "Expected at least 5 arguments, but received fewer in map()"
// this PR: nothing

The build*Shader regex does the same thing, so buildMaterialShader('nope') gets no friendly error either. Can the bypass only apply when p5._isStrandsContextActive is true?

2. FES doesn't actually run inside buildMaterialShader()

buildMaterialShader is a decorated p5 method, so every call inside its callback counts as an internal call and validation is skipped. I ran the sketch from your description on main and on this branch, in global and instance mode, and got the same output both times. For atan(1, 2, 3) the only error is the existing strands one

your screenshots show this too: the "Before" console is empty (no false warnings), and the "After" one only has the experimental notice, which prints on main as well. I only saw the new FES path run with baseMaterialShader().modify(...)

3. With .modify(), the error shows up twice

atan(1, 2, 3) gives the strands error plus a new FES one that points at [strands_transpiler.js, line 2027] instead of the user's sketch. Strands already checks arg counts for builtins, so do we need the overload schema in validate() at all?

cc @davepagurek @ksen0

@aashu2006

Copy link
Copy Markdown
Member

@Ayush4958 couple of small things too, like a quick test can be added that map('a', 'b') still errors outside strands which would have caught the regression above. Also #7899 mentions the "overriding p5 variables" warnings which this doesn't touch, so maybe say like it partially addresses the issue. And ctx.previousFES plus the second p5._strandsSignatures = ... || new Map() line looks unused now, so they can go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[p5.strands] Reenabling and implementing FES in strands

3 participants