Fix build*Shader methods in instance mode by forwarding optional scope

[aashu2006]

Resolves #8520

Changes

The five build*Shader methods in src/webgl/material.js were calling .modify(callback) without forwarding the optional scope parameter.

In instance mode, users pass { p5 } or { sketch } as scope so the callback receives the correct instance. Since this argument was not forwarded, the callback didn’t receive the expected context.

This PR updates these methods to accept an optional scope parameter and pass it to .modify(callback, scope)

No changes are made to .modify() itself, and existing code without scope continues to work.

I have also added a unit test under test/unit/webgl/p5.Shader.js (inside the existing p5.strands suite) to verify that buildMaterialShader correctly forwards the optional scope to .modify(). JSDoc comments were updated to document the optional scope parameter.

// Before
fn.buildFilterShader = function (callback) {
  return this.baseFilterShader().modify(callback);
};

// After
fn.buildFilterShader = function (callback, scope) {
  return this.baseFilterShader().modify(callback, scope);
};

PR Checklist

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

[davepagurek]

Thanks for taking this on! It's looking good so far, but can we add a unit test for one of these methods to make sure it continues to work going forward? Also, can we add the optional context parameter to the doc comments, mirroring what is found in the p5.Shader.modify doc comments?

[aashu2006]

Hii @davepagurek I have added the unit test for scope forwarding and updated the JSDoc comments as suggested. All tests pass locally. Thanks ❤️

[davepagurek]

Hi! The test looks good, is there a reason why this is in its own file as opposed to the existing p5.Shader unit tests file? It might be better in there.

Also, generally the tests in that file don't create a new instance, they reuse a myp5 instance. To have less moving parts, they also omit a setup function, they just create a canvas, initialize whatever they need to, and then make their assertions. This means you wouldn't need a done function.

[aashu2006]

Thanks, that's right! I will move the test into p5.Shader.js and update it to reuse myp5 like the other tests, without creating a new instance or using done.

[davepagurek]

p5.strands shaders don't have the ability to assign to variables outside of their functions -- since we transpile them, they lose all external scope, which is the reason why we have to manually pass scope in.

Rather than doing something like this in the test, we can maybe just use scope the way we use it in the .modify calls in the other tests, e.g. this call, before your changes, wouldn't work with buildMaterialShader:

p5.js/test/unit/webgl/p5.Shader.js

Lines 555 to 565 in 3a7992c

	const testShader = myp5.baseMaterialShader().modify(() => {
	const x = myp5.uniformFloat(() => 1.0); // true condition
	myp5.getPixelInputs(inputs => {
	let color = myp5.float(0.5); // initial gray
	if (x > 0.5) {
	color = myp5.float(1.0); // set to white in if branch
	}
	inputs.color = [color, color, color, 1.0];
	return inputs;
	});
	}, { myp5 });

We can just wrap the creation of the shader in an expect(() => { /* ... */ }).not.toThrow() call maybe?

[aashu2006]

Thanks for explaining, I will update the test to follow the existing .modify() pattern and wrap the shader creation in an expect(() => {}).not.toThrowError() call instead of assigning to external variables.

[aashu2006]

Hi @davepagurek . I have moved the scope test into p5.Shader.js under the existing p5.strands suite and aligned it with the surrounding test style. All tests have passed locally. Thanks 🙂

[davepagurek]

Awesome! thanks again!

[aashu2006]

Awesome! thanks again!

Thanks a lot for the review and guidance, I really appreciate the time you took to explain the test structure & strands behavior. Learned a lot from this one 🙂