[p5.js 2.0 Bug Report]: [p5.strands] && and || in if conditions not working correctly

[davepagurek]

Most appropriate sub-area of p5.js?

• Accessibility
• Color
• Core/Environment/Rendering
• Data
• DOM
• Events
• Image
• IO
• Math
• Typography
• Utilities
• WebGL
• Build process
• Unit testing
• Internationalization
• Friendly errors
• Other (specify if possible)

p5.js version

2.1.2

Web browser and version

Any

Operating system

Any

Steps to reproduce this

Steps:

1. Create a p5.strands shader
2. Include an if statement with a compound condition

Take this if:

let c = [1, 1, 1, 1]
// Doesn't work
if (abs(inputs.texCoord.x - 0.5) > 0.2 || abs(inputs.texCoord.y - 0.5) > 0.2) {
  c = [1, 0, 0, 1]
}

If I put a debugger statement in there, it's getting transpiled to this:

let c = __p5.strandsNode([
    1,
    1,
    1,
    1
])
{
    c = __p5.strandsNode(c)
    const __block_2 = __p5.strandsIf(abs(inputs.texCoord.x.sub(0.5)).greaterThan(0.2) || abs(inputs.texCoord.y.sub(0.5)).greaterThan(0.2), () => {
        let __copy_c_0 = c.copy()
        __copy_c_0 = __p5.strandsNode([
            1,
            0,
            0,
            1
        ])
        return { c: __copy_c_0 }
    }).Else(() => {
        let __copy_c_1 = c.copy()
        return { c: __copy_c_1 }
    })
    c = __block_2.c
}

Note that there's still an ||, which doesn't work because these are not actual booleans. It should be getting transpiled to a .or(...).

I thought this bit of the transpiler code would do that, but apparently it's not:

p5.js/src/strands/strands_transpiler.js

Lines 247 to 277 in 7fa997e

	BinaryExpression(node, _state, ancestors) {
	// Don't convert uniform default values to node methods, as
	// they should be evaluated at runtime, not compiled.
	if (ancestors.some(nodeIsUniform)) { return; }
	// If the left hand side of an expression is one of these types,
	// we should construct a node from it.
	const unsafeTypes = ['Literal', 'ArrayExpression', 'Identifier'];
	if (unsafeTypes.includes(node.left.type)) {
	const leftReplacementNode = {
	type: 'CallExpression',
	callee: {
	type: 'Identifier',
	name: '__p5.strandsNode',
	},
	arguments: [node.left]
	}
	node.left = leftReplacementNode;
	}
	// Replace the binary operator with a call expression
	// in other words a call to BaseNode.mult(), .div() etc.
	node.type = 'CallExpression';
	node.callee = {
	type: 'MemberExpression',
	object: node.left,
	property: {
	type: 'Identifier',
	name: replaceBinaryOperator(node.operator),
	},
	};
	node.arguments = [node.right];
	},

Snippet:

Live: https://editor.p5js.org/davepagurek/sketches/5MtHbKB-A

let sh
async function setup() {
  createCanvas(windowWidth, windowHeight, WEBGL)
  sh = baseFilterShader().modify(() => {
    getColor((inputs) => {
      let c = [1, 1, 1, 1]
      // Doesn't work
      if (abs(inputs.texCoord.x - 0.5) > 0.2 || abs(inputs.texCoord.y - 0.5) > 0.2) {
        c = [1, 0, 0, 1]
      }
      
      // Works
      // if (abs(inputs.texCoord.x - 0.5) > 0.2 ) {
      //   c = [1, 0, 0, 1]
      // }
      // if (abs(inputs.texCoord.y - 0.5) > 0.2 ) {
      //   c = [1, 0, 0, 1]
      // }
      
      return c
    })
  })
}

function draw() {
  filter(sh)
}

[Aayushdev18]

I can reproduce this issue and understand the transpiler problem with || not converting to .or().
@davepagurek Could you please assign this issue to me? I’d like to work on a fix.

[davepagurek]

Hi @Aayushdev18, I started looking into it, but if you want to take my findings and make a PR that would be great! I went to https://astexplorer.net/ and set the parser to acorn (the library we use for p5.strands and FES) and pasted in the callback that gets transpiled:

() => {
  getColor((inputs) => {
      let c = [1, 1, 1, 1]
      // Doesn't work
      if (abs(inputs.texCoord.x - 0.5) > 0.2 || abs(inputs.texCoord.y - 0.5) > 0.2) {
        c = [1, 0, 0, 1]
      }
      debugger
      
      // Works
      // if (abs(inputs.texCoord.x - 0.5) > 0.2 ) {
      //   c = [1, 0, 0, 1]
      // }
      // if (abs(inputs.texCoord.y - 0.5) > 0.2 ) {
      //   c = [1, 0, 0, 1]
      // }
      
      return c
    })
}

...and it looks like this is a LogicalExpression, not a BinaryExpression:

So we probably need to duplicate this BinaryExpression rule for LogicalExpression too.

[Aayushdev18]

Thanks! I’m looking into how LogicalExpression nodes are handled in the strands/FES transpilation. I’ll try to add proper support for && / || and follow up with a PR.

[davepagurek]

To clarify, this doesn't actually have anything to do with FES, we just happen to use Acorn for code parsing for both. We don't yet do anything for LogicalExpression nodes, but I suspect that doing the same thing that we do for BinaryExpression nodes will suffice. Adding a unit test for this would be great too!

[Aayushdev18]

Got it, thanks for the clarification! I’ll mirror the BinaryExpression handling for LogicalExpression and add a unit test, then open a PR.