Skip to content

fix builtin global accessors for instance mode in strands shaders#8878

Open
aashu2006 wants to merge 2 commits into
processing:dev-2.0from
aashu2006:fix/strands-instance-mode-globals
Open

fix builtin global accessors for instance mode in strands shaders#8878
aashu2006 wants to merge 2 commits into
processing:dev-2.0from
aashu2006:fix/strands-instance-mode-globals

Conversation

@aashu2006

Copy link
Copy Markdown
Contributor

Resolves #8877

Changes:

Fixed installBuiltinGlobalAccessors in strands_api.js so that the builtin globals like mouseX and width work correctly in instance mode when used inside strands shader callbacks.

Previously only window.mouseX was intercepted, so in instance mode (p.mouseX), the values got hardcoded into the shader instead of becoming live uniforms.

Now the function also installs property interceptors on p5.prototype and p5.Graphics.prototype. It correctly handles both data properties (like mouseX) and getter based properties (like width, which delegates to this._renderer?.width) by saving the original property descriptor and delegating to it when strands isn't active.

Also I've added a unit test that verifies myp5.mouseX and myp5.width return strands nodes inside hooks and normal numbers outside.

PR Checklist

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

@p5-bot

p5-bot Bot commented Jun 8, 2026

Copy link
Copy Markdown

Continuous Release

CDN link

Published Packages

Commit hash: 94fb7b6

Previous deployments

This is an automated message.

Comment thread src/strands/strands_api.js Outdated
const sym = Symbol(`_strands_${name}`)

// Define on window for global mode
Object.defineProperty(window, name, {

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.

I think we need to check _isGlobal here, to avoid adding accessors to the global window object on instance mode.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

thanks for catching it @eupthere, I've added the _isGlobal check so we only define window accessors in global mode now. Also updated the tests to use instance access (myp5.width instead of window.width) since the test framework runs in instance mode anyway.


// For data properties (like mouseX), save the initial value into Symbol-keyed store
if (originalProtoDesc && 'value' in originalProtoDesc) {
strandsContext.p5.prototype[sym] = originalProtoDesc.value;

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.

Is there a reason for using a symbol here as opposed to a string?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Using a symbol here to avoid any chance of colliding with existing properties on the instance/prototype. A string like _strands_mouseX would also work, but symbols are guaranteed to be unique and stay out of things like Object.keys() and for...in, so the backing store remains hidden.

happy to switch to strings if you think that's simpler 🙂

//In instance mode, myp5.mouseX and myp5.width should return strands nodes
const mxInHook = myp5.mouseX;
const wInHook = myp5.width;
assert.isTrue(mxInHook.isStrandsNode);

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.

nice!

@aashu2006

aashu2006 commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

Hi @davepagurek , I added the _isGlobal check so the window accessors are only defined in global mode. Also updated the tests to use instance access (myp5.width) instead of window.width, since the test environment runs in instance mode anyway.

The full test suite is passing now.

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.

3 participants