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

More fixes for minified filters

Author: davepagurek

src/core/filterShaders.js

@@ -1,7 +1,13 @@
 import * as constants from './constants';
 
 /*
- * Creates p5.strands filter shaders for cross-platform compatibility
+ * Creates p5.strands filter shaders for cross-platform compatibility.
+ *
+ * NOTE: These work a little differently than p5.js web editor shaders work!
+ * Firstly, it uses instance mode, so we have to explicitly pass in context
+ * variables in an argument to your callback and as a second argument to `modify`.
+ * Secondly, always manually specify uniform names, as variable names will change
+ * in minified builds.
  */
 export function makeFilterShader(renderer, operation, p5) {
   switch (operation) {
@@ -26,7 +32,7 @@ export function makeFilterShader(renderer, operation, p5) {
 
     case constants.THRESHOLD:
       return renderer.baseFilterShader().modify(({ p5 }) => {
-        const filterParameter = p5.uniformFloat();
+        const filterParameter = p5.uniformFloat('filterParameter');
         p5.getColor((inputs, canvasContent) => {
           const color = p5.getTexture(canvasContent, inputs.texCoord);
           // weighted grayscale with luminance values
@@ -39,7 +45,7 @@ export function makeFilterShader(renderer, operation, p5) {
 
     case constants.POSTERIZE:
       return renderer.baseFilterShader().modify(({ p5 }) => {
-        const filterParameter = p5.uniformFloat();
+        const filterParameter = p5.uniformFloat('filterParameter');
         const quantize = (color, n) => {
           // restrict values to N options/bins
           // and floor each channel to nearest value
@@ -61,8 +67,8 @@ export function makeFilterShader(renderer, operation, p5) {
 
     case constants.BLUR:
       return renderer.baseFilterShader().modify(({ p5 }) => {
-        const radius = p5.uniformFloat();
-        const direction = p5.uniformVec2();
+        const radius = p5.uniformFloat('radius');
+        const direction = p5.uniformVec2('direction');
 
         // This isn't a real Gaussian weight, it's a quadratic weight
         const quadWeight = (x, e) => {

src/core/p5.Renderer3D.js

@@ -1912,13 +1912,13 @@ export class Renderer3D extends Renderer {
     if (!this.sphereMapping) {
       const p5 = this._pInst;
       this.sphereMapping = this.baseFilterShader().modify(({ p5 }) => {
-        const uEnvMap = p5.uniformTexture();
-        const uFovY = p5.uniformFloat();
-        const uAspect = p5.uniformFloat();
+        const uEnvMap = p5.uniformTexture('uEnvMap');
+        const uFovY = p5.uniformFloat('uFovY');
+        const uAspect = p5.uniformFloat('uAspect');
         // Hack: we don't have matrix uniforms yet; use three vectors
-        const uN1 = p5.uniformVec3();
-        const uN2 = p5.uniformVec3();
-        const uN3 = p5.uniformVec3();
+        const uN1 = p5.uniformVec3('uN1');
+        const uN2 = p5.uniformVec3('uN2');
+        const uN3 = p5.uniformVec3('uN3');
         p5.getColor((inputs) => {
           const uFovX = uFovY * uAspect;
           const angleY = p5.mix(uFovY/2.0,  -uFovY/2.0, inputs.texCoord.y);

src/strands/strands_transpiler.js

@@ -111,19 +111,34 @@ const ASTCallbacks = {
   VariableDeclarator(node, _state, ancestors) {
     if (ancestors.some(nodeIsUniform)) { return; }
     if (nodeIsUniform(node.init)) {
-      const uniformNameLiteral = {
-        type: 'Literal',
-        value: node.id.name
+      // Only inject the variable name if the first argument isn't already a string
+      if (node.init.arguments.length === 0 ||
+          node.init.arguments[0].type !== 'Literal' ||
+          typeof node.init.arguments[0].value !== 'string') {
+        const uniformNameLiteral = {
+          type: 'Literal',
+          value: node.id.name
+        }
+        node.init.arguments.unshift(uniformNameLiteral);
       }
-      node.init.arguments.unshift(uniformNameLiteral);
     }
     if (nodeIsVarying(node.init)) {
-      const varyingNameLiteral = {
-        type: 'Literal',
-        value: node.id.name
+      // Only inject the variable name if the first argument isn't already a string
+      if (
+        node.init.arguments.length === 0 ||
+        node.init.arguments[0].type !== 'Literal' ||
+        typeof node.init.arguments[0].value !== 'string'
+      ) {
+        const varyingNameLiteral = {
+          type: 'Literal',
+          value: node.id.name
+        }
+        node.init.arguments.unshift(varyingNameLiteral);
+        _state.varyings[node.id.name] = varyingNameLiteral;
+      } else {
+        // Still track it as a varying even if name wasn't injected
+        _state.varyings[node.id.name] = node.init.arguments[0];
       }
-      node.init.arguments.unshift(varyingNameLiteral);
-      _state.varyings[node.id.name] = varyingNameLiteral;
     }
   },
   Identifier(node, _state, ancestors) {
@@ -362,45 +377,75 @@ const ASTCallbacks = {
       };
       // Analyze which outer scope variables are assigned in any branch
       const assignedVars = new Set();
-      const analyzeBlock = (body) => {
-        if (body.type !== 'BlockStatement') return;
-        // First pass: collect variable declarations within this block
-        const localVars = new Set();
-        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;
+        });
+      };
+
+      const analyzeBranch = (functionBody) => {
+        // First pass: collect all variable declarations in the branch
+        const localVars = new Set();
+        ancestor(functionBody, {
+          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;
+        });
+
+        // Second pass: find assignments to non-local variables using acorn-walk
+        ancestor(functionBody, {
+          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
+              // 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
-            analyzeBlock(stmt);
           }
-        }
+        });
       };
+
       // Analyze all branches for assignments to outer scope variables
-      analyzeBlock(thenFunction.body);
-      analyzeBlock(elseFunction.body);
+      analyzeBranch(thenFunction.body);
+      analyzeBranch(elseFunction.body);
       if (assignedVars.size > 0) {
         // Add copying, reference replacement, and return statements to branch functions
         const addCopyingAndReturn = (functionBody, varsToReturn) => {

src/webgl/p5.Shader.js

@@ -243,15 +243,20 @@ class Shader {
    * 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.
    *
+   * If you are also using a build system for your sketch, variable names may be changed as
+   * part of minification. When creating a uniform, you can pass the name of the uniform in
+   * as a first parameter to ensure it doesn't get changed.
+   *
    * ```js example
    * new p5((sketch) => {
    *   let myShader;
    *
    *   sketch.setup = function() {
    *     sketch.createCanvas(200, 200, sketch.WEBGL);
    *     myShader = sketch.baseMaterialShader().modify(({ sketch }) => {
+   *       let b = uniformFloat('b');
    *       sketch.getPixelInputs((inputs) => {
-   *         inputs.color = [inputs.texCoord, 0, 1];
+   *         inputs.color = [inputs.texCoord, b, 1];
    *         return inputs;
    *       });
    *     }, { sketch });
@@ -260,6 +265,7 @@ class Shader {
    *   sketch.draw = function() {
    *     sketch.background(255);
    *     sketch.noStroke();
+   *     myShader.setUniform('b', 0.5);
    *     sketch.shader(myShader); // Apply the custom shader
    *     sketch.plane(sketch.width, sketch.height); // Draw a plane with the shader applied
    *   }

test/unit/webgl/p5.Shader.js

@@ -426,6 +426,54 @@ suite('p5.Shader', function() {
         myp5.plane(myp5.width, myp5.height);
       }).not.toThrowError();
     });
+
+    test('handle custom uniform names with automatic values', () => {
+      myp5.createCanvas(50, 50, myp5.WEBGL);
+      const testShader = myp5.baseMaterialShader().modify(() => {
+        // Variable name is 'brightness' but uniform name is 'customBrightness'
+        const brightness = myp5.uniformFloat('customBrightness', () => 0.8);
+        myp5.getPixelInputs(inputs => {
+          inputs.color = [brightness, brightness, brightness, 1.0];
+          return inputs;
+        });
+      }, { myp5 });
+
+      myp5.noStroke();
+      myp5.shader(testShader);
+      myp5.plane(myp5.width, myp5.height);
+
+      // Check that the shader uses the automatic value (0.8)
+      const pixelColor = myp5.get(25, 25);
+      assert.approximately(pixelColor[0], 204, 5); // 0.8 * 255 = 204
+      assert.approximately(pixelColor[1], 204, 5);
+      assert.approximately(pixelColor[2], 204, 5);
+    });
+
+    test('handle custom uniform names with manual setUniform', () => {
+      myp5.createCanvas(50, 50, myp5.WEBGL);
+      const testShader = myp5.baseMaterialShader().modify(() => {
+        // Variable name is 'brightness' but uniform name is 'customBrightness'
+        const brightness = myp5.uniformFloat('customBrightness');
+        myp5.getPixelInputs(inputs => {
+          inputs.color = [brightness, brightness, brightness, 1.0];
+          return inputs;
+        });
+      }, { myp5 });
+
+      // Set the uniform using the custom name
+      testShader.setUniform('customBrightness', 0.6);
+
+      myp5.noStroke();
+      myp5.shader(testShader);
+      myp5.plane(myp5.width, myp5.height);
+
+      // Check that the shader uses the manual value (0.6)
+      const pixelColor = myp5.get(25, 25);
+      assert.approximately(pixelColor[0], 153, 5); // 0.6 * 255 = 153
+      assert.approximately(pixelColor[1], 153, 5);
+      assert.approximately(pixelColor[2], 153, 5);
+    });
+
     suite('if statement conditionals', () => {
       test('handle simple if statement with true condition', () => {
         myp5.createCanvas(50, 50, myp5.WEBGL);
@@ -844,6 +892,37 @@ suite('p5.Shader', function() {
         assert.approximately(pixelColor[1], 127, 5); // Green channel should be ~127
         assert.approximately(pixelColor[2], 127, 5); // Blue channel should be ~127
       });
+
+      test('handle comma-separated expressions with assignments', () => {
+        myp5.createCanvas(50, 50, myp5.WEBGL);
+        const testShader = myp5.baseMaterialShader().modify(() => {
+          const condition = myp5.uniformFloat(() => 1.0); // true condition
+          myp5.getPixelInputs(inputs => {
+            let red = myp5.float(0.0);
+            let green = myp5.float(0.0);
+            let blue = myp5.float(0.0);
+
+            if (condition > 0.5) {
+              // Use comma-separated expressions with assignments
+              red = myp5.float(1.0), green = myp5.float(0.5), blue = myp5.float(0.2);
+            } else {
+              red = myp5.float(0.0), green = myp5.float(0.0), blue = myp5.float(0.0);
+            }
+
+            inputs.color = [red, green, blue, 1.0];
+            return inputs;
+          });
+        }, { myp5 });
+        myp5.noStroke();
+        myp5.shader(testShader);
+        myp5.plane(myp5.width, myp5.height);
+
+        // Check that the center pixel has the expected color (condition was true)
+        const pixelColor = myp5.get(25, 25);
+        assert.approximately(pixelColor[0], 255, 5); // Red channel should be 255
+        assert.approximately(pixelColor[1], 127, 5); // Green channel should be ~127
+        assert.approximately(pixelColor[2], 51, 5);  // Blue channel should be ~51
+      });
     });
 
     suite('for loop statements', () => {