Skip to content

Add a read-only flag to indicate if the current canvas is Web GL #5844

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

spikyMalkallam
Copy link

Resolves #5750

Changes:
Added a read-only .isWebGL flag to p5 instances. This flag is set in createCavnas. Included comments, no inline docs.

Usage screenshot:
image

PR Checklist

  • npm run lint passes

@welcome
Copy link

welcome bot commented Oct 25, 2022

🎉 Thanks for opening this pull request! Please check out our contributing guidelines if you haven't already. And be sure to add yourself to the list of contributors on the readme page!

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.

Thanks for taking this issue on! I think before we merge this in, it would be great if:

  • we can add this property to p5.Graphics as well as to the main p5 instance (I also left a comment about maybe defining it a slightly different way to make it easier to set up, let me know what you think!)
  • we could add a small unit test that check that it works for p5 and for graphics so that we don't accidentally break it in the future

I'm happy to explain more about my suggestions or how to set up and run unit tests for this sort of thing if it would help 🙂

//isWebGL flag is set and made read-only
if (this.isWebGL == false) {
Object.defineProperty(this,"isWebGL" , {
value: this._renderer.drawingContext instanceof WebGLRenderingContext,
Copy link
Contributor

@davepagurek davepagurek Oct 27, 2022

Choose a reason for hiding this comment

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

If we use a get accessor here instead of value directly, then we can put this in the constructor for p5 and not have to worry about handling createCanvas not being called or being called more than once, since the value will be determined at runtime. i.e., Object.defineProperty(this, 'webgl', { get: () => this._renderer.drawingContext instanceof WebGLRenderingContext, writable: false }).

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also add the same accessor to the p5.Graphics constructor?

value: this._renderer.drawingContext instanceof WebGLRenderingContext,
writable: false
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, it looks like the spacing and equal signs aren't quite lined up here. Can you double check that npm run lint passes after making changes?

@Qianqianye
Copy link
Contributor

Thanks @spikyMalkallam for working on it. Will you be able to make the changes based on @davepagurek's suggestion? Please let us know if you need any help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a flag to indicate if the most recently created canvas is 3D
3 participants