-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Add WebGL2 support #6035
Add WebGL2 support #6035
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, thanks so much for pulling it together @davepagurek !
I left a few notes but one other thing I think we should include are some tests. Can you add a few for each new piece of the api, and maybe some others to make sure that font rendering is still working in both versions. Also maybe one more to make sure webGL versions are working with p5.graphics?
src/core/main.js
Outdated
* and will fall back to a WebGL1 context if WebGL2 is not available. If you | ||
* want to always use a WebGL1 context, you can set `p5.disableWebGL2 = true`. | ||
* | ||
* @property {Boolean} disableWebGL2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we do this in the setAttributes function instead? It feels against the grain of existing p5 patterns to be setting something on the p5 object before the sketch is created. Something like:
setAttributes('version' 1);
// or
setAttributes({version: 2});
Is there a reason we wouldn't do it this way?
Also, I think there are a couple more spots in the library that use webGL1 extensions that might need some special handling. There's the float textures in p5.Texture |
I think in webGL two we can also do away with the power of two texture size checks as well in p5.Texture, especially if people are trying to use repeat wrap functions Line 350 in af0fc0d
|
Definitely not a blocker for this PR but at some point in the future it might be good for us to try and switch over the rendering to use VAO's if possible, as described in this article |
Thanks for catching those other WebGL1 extension cases @aferriss! I've updated those parts to do nothing if using WebGL2, added some tests, and switched to using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple small changes and then I think we'll be good to roll!
Can we add documentation for the new setAttributes property here: https://github.com/processing/p5.js/blob/main/src/webgl/p5.RendererGL.js#L405
Is there anywhere else in the docs that we can indicate that webGL mode uses webGL2 by default now? Maybe a note on https://p5js.org/reference/#/p5/createCanvas and https://p5js.org/reference/#/p5/createGraphics ?
Can you link the inconsolata font from the manual test examples instead of adding another copy?
src/core/environment.js
Outdated
@@ -369,6 +369,36 @@ p5.prototype.noCursor = function() { | |||
this._curElement.elt.style.cursor = 'none'; | |||
}; | |||
|
|||
/** | |||
* If the was created in WebGL mode, then `weglVersion` will indicate which |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit— should probably say: "If the sketch was created...."
HTMLCanvasElement.prototype.getContext = prevGetContext; | ||
}); | ||
|
||
test('should return WEBGL1', function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you added this test twice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops good catch, I left the setAttributes({ version: 1})
in there by accident! I'll take that line out again so that it tests to make sure you still end up with webgl1 even when you don't request it if the browser doesn't support webgl2 (mocked in the suite
this is nested in.)
Updated those tests and docs! I have an idea for a better visual example of the difference between webgl1 and 2 so I'll make one more update after work with that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! 🚀
Oh wow, this is exciting! Thanks @davepagurek and @aferriss for working on it. Hi @stalgiag, does this PR look good to you to be merged? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing! This is so exciting! Thanks for the hard work @aferriss and @davepagurek!
defaultShaders.fontFrag | ||
this._webGL2CompatibilityPrefix('vert', 'mediump') + | ||
defaultShaders.fontVert, | ||
this._webGL2CompatibilityPrefix('frag', 'mediump') + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am curious if this float precision is always guaranteed to work well on a device that supports WebGL2. Not sure but it is irrelevant because this design will make adjustments to precision based on the device easier in the future.
if (widthPowerOfTwo && heightPowerOfTwo) { | ||
if ( | ||
this._renderer.webglVersion === constants.WEBGL2 || | ||
(widthPowerOfTwo && heightPowerOfTwo) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth rolling this into a simple function that returns a boolean just to improve readability. I find the nested conditionals and multiline conditions to be a challenge to read in this section (and many other sections of the GL code). Not a new problem with this PR by any means but one worth keeping in mind.
Resolves #2536
Changes
p5.disableWebGL2
to force WebGL1 modewebglVersion
to graphics and the main canvas to read which version was pickedfwidth
. This exists by default in WebGL2, but only if you upgrade the glsl version from 2 to 3. I added some glsl macros which will be the correct glsl 2 or 3 keyword depending on the environment.MIN
) before falling back to the extension version (e.g.MIN_EXT
)Screenshots of the change:
No visual change should occur!
I have a test sketch here https://editor.p5js.org/davepagurek/sketches/-XNYgrz_z mirroring the example added to the docs. Commenting/uncommenting the
p5.disableWebGL2
line will make the sketch show "WebGL 1" or "WebGL 2" on canvas, and also demonstrates the font shader working in both environments.PR Checklist
npm run lint
passes