Skip to content

Conversation

@avinxshKD
Copy link

Resolves #8171

Changes:

What was changed:

  • Modified src/strands/strands_api.js to automatically intercept 16 p5 global properties when strands is active
  • Properties are automatically converted to uniforms with callbacks to their live values
  • Implemented proper context restoration when strands becomes inactive

Screenshots of the change:
N/A - This is an API enhancement without visual changes.

PR Checklist

@avinxshKD
Copy link
Author

hey @davepagurek, kindly review this too when you get a chance

Copy link
Contributor

@davepagurek davepagurek left a comment

Choose a reason for hiding this comment

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

Hi, sorry for the delay, we were busy getting the 2.1 release out. Now that that's done, I can give an actual review! Your general approach looks good.

  • Do you think you can add some unit tests that make use of these globals so we can both verify that it works, and also help ensure that it doesn't break in the future after other changes?
  • It would also be great to put up a test sketch in the p5 web editor to play around with and help find bugs. To do that, you can run npm run build, upload lib/p5.js to a web editor sketch, and edit its p5 script tag in index.html to point to your uploaded file.

{ name: 'pmouseY', type: DataType.float1 },
{ name: 'winMouseX', type: DataType.float1 },
{ name: 'winMouseY', type: DataType.float1 },
{ name: 'frameCount', type: DataType.int1 },
Copy link
Contributor

Choose a reason for hiding this comment

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

While this is an integer value, it's fairly unintuitive for users of strands for it to actually be a glsl int instead of a float, since in JS all numbers are floats, and you'd expect frameCount / 2 to have decimals sometimes. So maybe let's turn all the ints into floats here?

{ name: 'displayHeight', type: DataType.float1 },
{ name: 'windowWidth', type: DataType.float1 },
{ name: 'windowHeight', type: DataType.float1 },
{ name: 'mouseButton', type: DataType.int1 },
Copy link
Contributor

Choose a reason for hiding this comment

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

In p5 2.x this isn't an int, it's an object: https://beta.p5js.org/reference/p5/mousebutton/ We might need to special case this one by either making each property a uniform, or by just omitting it for 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.

2 participants