Merge pull request #8367 from processing/fix/minified-strands

davepagurek · web-flow · commit a213e53eaba7 · 2025-12-27T15:56:19.000-05:00
Fix p5.strands filters not working on minified code
diff --git a/src/core/filterShaders.js b/src/core/filterShaders.js
@@ -6,7 +6,7 @@ import * as constants from './constants';
 export function makeFilterShader(renderer, operation, p5) {
   switch (operation) {
     case constants.GRAY:
-      return renderer.baseFilterShader().modify(() => {
+      return renderer.baseFilterShader().modify(({ p5 }) => {
         p5.getColor((inputs, canvasContent) => {
           const tex = p5.getTexture(canvasContent, inputs.texCoord);
           // weighted grayscale with luminance values
@@ -16,7 +16,7 @@ export function makeFilterShader(renderer, operation, p5) {
       }, { p5 });
 
     case constants.INVERT:
-      return renderer.baseFilterShader().modify(() => {
+      return renderer.baseFilterShader().modify(({ p5 }) => {
         p5.getColor((inputs, canvasContent) => {
           const color = p5.getTexture(canvasContent, inputs.texCoord);
           const invertedColor = p5.vec3(1.0) - color.rgb;
@@ -25,7 +25,7 @@ export function makeFilterShader(renderer, operation, p5) {
       }, { p5 });
 
     case constants.THRESHOLD:
-      return renderer.baseFilterShader().modify(() => {
+      return renderer.baseFilterShader().modify(({ p5 }) => {
         const filterParameter = p5.uniformFloat();
         p5.getColor((inputs, canvasContent) => {
           const color = p5.getTexture(canvasContent, inputs.texCoord);
@@ -38,7 +38,7 @@ export function makeFilterShader(renderer, operation, p5) {
       }, { p5 });
 
     case constants.POSTERIZE:
-      return renderer.baseFilterShader().modify(() => {
+      return renderer.baseFilterShader().modify(({ p5 }) => {
         const filterParameter = p5.uniformFloat();
         const quantize = (color, n) => {
           // restrict values to N options/bins
@@ -60,7 +60,7 @@ export function makeFilterShader(renderer, operation, p5) {
       }, { p5 });
 
     case constants.BLUR:
-      return renderer.baseFilterShader().modify(() => {
+      return renderer.baseFilterShader().modify(({ p5 }) => {
         const radius = p5.uniformFloat();
         const direction = p5.uniformVec2();
 
@@ -120,7 +120,7 @@ export function makeFilterShader(renderer, operation, p5) {
       }, { p5 });
 
     case constants.ERODE:
-      return renderer.baseFilterShader().modify(() => {
+      return renderer.baseFilterShader().modify(({ p5 }) => {
         const luma = (color) => {
           return p5.dot(color.rgb, p5.vec3(0.2126, 0.7152, 0.0722));
         };
@@ -150,7 +150,7 @@ export function makeFilterShader(renderer, operation, p5) {
       }, { p5 });
 
     case constants.DILATE:
-      return renderer.baseFilterShader().modify(() => {
+      return renderer.baseFilterShader().modify(({ p5 }) => {
         const luma = (color) => {
           return p5.dot(color.rgb, p5.vec3(0.2126, 0.7152, 0.0722));
         };
@@ -180,7 +180,7 @@ export function makeFilterShader(renderer, operation, p5) {
       }, { p5 });
 
     case constants.OPAQUE:
-      return renderer.baseFilterShader().modify(() => {
+      return renderer.baseFilterShader().modify(({ p5 }) => {
         p5.getColor((inputs, canvasContent) => {
           const color = p5.getTexture(canvasContent, inputs.texCoord);
           return p5.vec4(color.rgb, 1.0);
diff --git a/src/core/p5.Renderer3D.js b/src/core/p5.Renderer3D.js
@@ -1911,7 +1911,7 @@ export class Renderer3D extends Renderer {
   _getSphereMapping(img) {
     if (!this.sphereMapping) {
       const p5 = this._pInst;
-      this.sphereMapping = this.baseFilterShader().modify(() => {
+      this.sphereMapping = this.baseFilterShader().modify(({ p5 }) => {
         const uEnvMap = p5.uniformTexture();
         const uFovY = p5.uniformFloat();
         const uAspect = p5.uniformFloat();
diff --git a/src/strands/p5.strands.js b/src/strands/p5.strands.js
@@ -75,7 +75,7 @@ function strands(p5, fn) {
 
   p5.Shader.prototype.modify = function (shaderModifier, scope = {}) {
     try {
-      if (shaderModifier instanceof Function) {
+      if (shaderModifier instanceof Function || typeof shaderModifier === 'string') {
         // Reset the context object every time modify is called;
         // const backend = glslBackend;
         initStrandsContext(strandsContext, this._renderer.strandsBackend, {
@@ -92,7 +92,9 @@ function strands(p5, fn) {
         if (options.parser) {
           // #7955 Wrap function declaration code in brackets so anonymous functions are not top level statements, which causes an error in acorn when parsing
           // https://github.com/acornjs/acorn/issues/1385
-          const sourceString = `(${shaderModifier.toString()})`;
+          const sourceString = typeof shaderModifier === 'string'
+            ? `(${shaderModifier})`
+            : `(${shaderModifier.toString()})`;
           strandsCallback = transpileStrandsToJS(
             p5,
             sourceString,
diff --git a/src/strands/strands_for.js b/src/strands/strands_for.js
@@ -359,7 +359,7 @@ export class StrandsFor {
     CFG.popBlock(cfg);
 
     const loopVarNode = createStrandsNode(phiNode.id, phiNode.dimension, this.strandsContext);
-    this.bodyResults = this.bodyCb(loopVarNode, phiVars);
+    this.bodyResults = this.bodyCb(loopVarNode, phiVars) || {};
     for (const key in this.bodyResults) {
       this.bodyResults[key] = this.strandsContext.p5.strandsNode(this.bodyResults[key]);
     }
diff --git a/src/strands/strands_transpiler.js b/src/strands/strands_transpiler.js
@@ -130,22 +130,19 @@ const ASTCallbacks = {
     if (ancestors.some(nodeIsUniform)) { return; }
     if (_state.varyings[node.name]
       && !ancestors.some(a => a.type === 'AssignmentExpression' && a.left === node)) {
-        node.type = 'ExpressionStatement';
-        node.expression = {
-          type: 'CallExpression',
-          callee: {
-            type: 'MemberExpression',
-            object: {
-              type: 'Identifier',
-              name: node.name
-            },
-            property: {
-              type: 'Identifier',
-              name: 'getValue'
-            },
+        node.type = 'CallExpression';
+        node.callee = {
+          type: 'MemberExpression',
+          object: {
+            type: 'Identifier',
+            name: node.name
           },
-          arguments: [],
-        }
+          property: {
+            type: 'Identifier',
+            name: 'getValue'
+          },
+        };
+        node.arguments = [];
       }
     },
     // The callbacks for AssignmentExpression and BinaryExpression handle
@@ -208,13 +205,12 @@ const ASTCallbacks = {
           varyingName = node.left.object.name;
         }
         // Check if it's a getValue() call: myVarying.getValue().xyz
-        else if (node.left.object.type === 'ExpressionStatement' &&
-                 node.left.object.expression?.type === 'CallExpression' &&
-                 node.left.object.expression.callee?.type === 'MemberExpression' &&
-                 node.left.object.expression.callee.property?.name === 'getValue' &&
-                 node.left.object.expression.callee.object?.type === 'Identifier' &&
-                 _state.varyings[node.left.object.expression.callee.object.name]) {
-          varyingName = node.left.object.expression.callee.object.name;
+        else if (node.left.object.type === 'CallExpression' &&
+                 node.left.object.callee?.type === 'MemberExpression' &&
+                 node.left.object.callee.property?.name === 'getValue' &&
+                 node.left.object.callee.object?.type === 'Identifier' &&
+                 _state.varyings[node.left.object.callee.object.name]) {
+          varyingName = node.left.object.callee.object.name;
         }
 
         if (varyingName) {
@@ -595,7 +591,7 @@ const ASTCallbacks = {
 
       // Transform for statement into strandsFor() call
       // for (init; test; update) body -> strandsFor(initCb, conditionCb, updateCb, bodyCb, initialVars)
-      
+
       // Generate unique loop variable name
       const uniqueLoopVar = `loopVar${loopVarCounter++}`;
 
@@ -714,46 +710,71 @@ const ASTCallbacks = {
 
       // Analyze which outer scope variables are assigned in the loop body
       const assignedVars = new Set();
-      const analyzeBlock = (body, parentLocalVars = new Set()) => {
-        if (body.type !== 'BlockStatement') return;
 
-        // First pass: collect variable declarations within this block
-        const localVars = new Set([...parentLocalVars]);
-        for (const stmt of body.body) {
+      // Helper function to check if a block contains strands control flow calls as immediate children
+      const blockContainsStrandsControlFlow = (node) => {
+        if (node.type !== 'BlockStatement') return false;
+        return node.body.some(stmt => {
+          // Check for variable declarations with strands control flow init
           if (stmt.type === 'VariableDeclaration') {
-            for (const decl of stmt.declarations) {
-              if (decl.id.type === 'Identifier') {
-                localVars.add(decl.id.name);
-              }
-            }
+            const match = stmt.declarations.some(decl =>
+              decl.init?.type === 'CallExpression' &&
+              (
+                (
+                  decl.init?.callee?.type === 'MemberExpression' &&
+                  decl.init?.callee?.object?.type === 'Identifier' &&
+                  decl.init?.callee?.object?.name === '__p5' &&
+                  (decl.init?.callee?.property?.name === 'strandsFor' ||
+                    decl.init?.callee?.property?.name === 'strandsIf')
+                ) ||
+                (
+                  decl.init?.callee?.type === 'Identifier' &&
+                  (decl.init?.callee?.name === '__p5.strandsFor' ||
+                    decl.init?.callee?.name === '__p5.strandsIf')
+                )
+              )
+            );
+            return match
+          }
+          return false;
+        });
+      };
+
+      // First pass: collect all variable declarations in the body
+      const localVars = new Set();
+      ancestor(bodyFunction.body, {
+        VariableDeclarator(node, ancestors) {
+          // Skip if we're inside a block that contains strands control flow
+          if (ancestors.some(blockContainsStrandsControlFlow)) return;
+          if (node.id.type === 'Identifier') {
+            localVars.add(node.id.name);
           }
         }
+      });
 
-        // Second pass: find assignments to non-local variables
-        for (const stmt of body.body) {
-          if (stmt.type === 'ExpressionStatement' &&
-              stmt.expression.type === 'AssignmentExpression') {
-            const left = stmt.expression.left;
-            if (left.type === 'Identifier') {
-              // Direct variable assignment: x = value
-              if (!localVars.has(left.name)) {
-                assignedVars.add(left.name);
-              }
-            } else if (left.type === 'MemberExpression' &&
-                       left.object.type === 'Identifier') {
-              // Property assignment: obj.prop = value (includes swizzles)
-              if (!localVars.has(left.object.name)) {
-                assignedVars.add(left.object.name);
-              }
+      // Second pass: find assignments to non-local variables using acorn-walk
+      ancestor(bodyFunction.body, {
+        AssignmentExpression(node, ancestors) {
+          // Skip if we're inside a block that contains strands control flow
+          if (ancestors.some(blockContainsStrandsControlFlow)) {
+            return
+          }
+
+          const left = node.left;
+          if (left.type === 'Identifier') {
+            // Direct variable assignment: x = value
+            if (!localVars.has(left.name)) {
+              assignedVars.add(left.name);
+            }
+          } else if (left.type === 'MemberExpression' &&
+                     left.object.type === 'Identifier') {
+            // Property assignment: obj.prop = value (includes swizzles)
+            if (!localVars.has(left.object.name)) {
+              assignedVars.add(left.object.name);
             }
-          } else if (stmt.type === 'BlockStatement') {
-            // Recursively analyze nested block statements, passing down local vars
-            analyzeBlock(stmt, localVars);
           }
         }
-      };
-
-      analyzeBlock(bodyFunction.body);
+      });
 
       if (assignedVars.size > 0) {
         // Add copying, reference replacement, and return statements similar to if statements
@@ -959,7 +980,7 @@ const ASTCallbacks = {
     // Reset counters at the start of each transpilation
     blockVarCounter = 0;
     loopVarCounter = 0;
-    
+
     const ast = parse(sourceString, {
       ecmaVersion: 2021,
       locations: srcLocations
@@ -992,18 +1013,37 @@ const ASTCallbacks = {
     recursive(ast, { varyings: {} }, postOrderControlFlowTransform);
     const transpiledSource = escodegen.generate(ast);
     const scopeKeys = Object.keys(scope);
-    const internalStrandsCallback = new Function(
-        // Create a parameter called __p5, not just p5, because users of instance mode
-        // may pass in a variable called p5 as a scope variable. If we rely on a variable called
-        // p5, then the scope variable called p5 might accidentally override internal function
-        // calls to p5 static methods.
-      '__p5',
-      ...scopeKeys,
-      transpiledSource
-      .slice(
-        transpiledSource.indexOf('{') + 1,
-        transpiledSource.lastIndexOf('}')
-      ).replaceAll(';', '')
-    );
-    return () => internalStrandsCallback(p5, ...scopeKeys.map(key => scope[key]));
+    const match = /\(?\s*(?:function)?\s*\(([^)]*)\)\s*(?:=>)?\s*{((?:.|\n)*)}\s*;?\s*\)?/
+      .exec(transpiledSource);
+    if (!match) {
+      console.log(transpiledSource);
+      throw new Error('Could not parse p5.strands function!');
+    }
+    const params = match[1].split(/,\s*/).filter(param => !!param.trim());
+    let paramVals, paramNames;
+    if (params.length > 0) {
+      paramNames = params;
+      paramVals = [scope];
+    } else {
+      paramNames = scopeKeys;
+      paramVals = scopeKeys.map(key => scope[key]);
+    }
+    const body = match[2];
+    try {
+      const internalStrandsCallback = new Function(
+          // Create a parameter called __p5, not just p5, because users of instance mode
+          // may pass in a variable called p5 as a scope variable. If we rely on a variable called
+          // p5, then the scope variable called p5 might accidentally override internal function
+          // calls to p5 static methods.
+        '__p5',
+        ...paramNames,
+        body,
+      );
+      return () => internalStrandsCallback(p5, ...paramVals);
+    } catch (e) {
+      console.error(e);
+      console.log(paramNames);
+      console.log(body);
+      throw new Error('Error transpiling p5.strands callback!');
+    }
   }
diff --git a/src/webgl/p5.Shader.js b/src/webgl/p5.Shader.js
@@ -239,7 +239,8 @@ class Shader {
    * p5.strands functions are special, since they get turned into a shader instead of being
    * run like the rest of your code. They only have access to p5.js functions, and variables
    * you declare inside the `modify` callback. If you need access to local variables, you
-   * can pass them into `modify` with an optional second parameter, `variables`. If you are
+   * can pass them into `modify` with an optional second parameter, `variables`. These will
+   * then be passed into your function as an argument. If you are
    * using instance mode, you will need to pass your sketch object in this way.
    *
    * ```js example
@@ -248,7 +249,7 @@ class Shader {
    *
    *   sketch.setup = function() {
    *     sketch.createCanvas(200, 200, sketch.WEBGL);
-   *     myShader = sketch.baseMaterialShader().modify(() => {
+   *     myShader = sketch.baseMaterialShader().modify(({ sketch }) => {
    *       sketch.getPixelInputs((inputs) => {
    *         inputs.color = [inputs.texCoord, 0, 1];
    *         return inputs;
diff --git a/test/unit/webgl/p5.Shader.js b/test/unit/webgl/p5.Shader.js
@@ -958,6 +958,35 @@ suite('p5.Shader', function() {
         assert.approximately(pixelColor[2], 0, 5);   // 0.0 * 255 = 0
       });
 
+      test('handle for loop modifying multiple variables after minification', () => {
+        myp5.createCanvas(50, 50, myp5.WEBGL);
+
+        const testShader = myp5.baseMaterialShader().modify(() => {
+          myp5.getPixelInputs(inputs => {
+            let red = myp5.float(0.0);
+            let green = myp5.float(0.0);
+
+            for (let i = 0; i < 4; i++) {
+              // Note the comma!
+              red = red + 0.125,    // 4 * 0.125 = 0.5
+                green = green + 0.25; // 4 * 0.25 = 1.0
+            }
+
+            inputs.color = [red, green, 0.0, 1.0];
+            return inputs;
+          });
+        }, { myp5 });
+
+        myp5.noStroke();
+        myp5.shader(testShader);
+        myp5.plane(myp5.width, myp5.height);
+
+        const pixelColor = myp5.get(25, 25);
+        assert.approximately(pixelColor[0], 127, 5); // 0.5 * 255 ≈ 127
+        assert.approximately(pixelColor[1], 255, 5); // 1.0 * 255 = 255
+        assert.approximately(pixelColor[2], 0, 5);   // 0.0 * 255 = 0
+      });
+
       test('handle for loop with conditional inside', () => {
         myp5.createCanvas(50, 50, myp5.WEBGL);
 

Original file line number	Diff line number	Diff line change
`@@ -359,7 +359,7 @@ export class StrandsFor {`
`359`	`359`	`CFG.popBlock(cfg);`
`360`	`360`
`361`	`361`	`const loopVarNode = createStrandsNode(phiNode.id, phiNode.dimension, this.strandsContext);`
`362`		`- this.bodyResults = this.bodyCb(loopVarNode, phiVars);`
	`362`	`+ this.bodyResults = this.bodyCb(loopVarNode, phiVars) \|\| {};`
`363`	`363`	`for (const key in this.bodyResults) {`
`364`	`364`	`this.bodyResults[key] = this.strandsContext.p5.strandsNode(this.bodyResults[key]);`
`365`	`365`	`}`