Enhancement limit WebGL textures to gl.MAX_TEXTURE_SIZE #6651

capGoblin · 2023-12-24T17:07:39Z

Resolves #6546

Changes:

For both Graphics and Framebuffer

Calculated the maximum allowed pixel dimensions using the hardware limit and pixel density.
Limited the dimensions to stay within these calculated maximum dimensions.

Screenshots of the change:

PR Checklist

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

… enhancement/limit-textures

capGoblin · 2023-12-24T17:33:54Z

Hi @davepagurek,

Here's the problem:

Let's say both the w and h given by the users are above the calculated maxAllowedPixelDimensions(let's say 8192), in this case, both the w and h are limited to maxAllowedPixelDimensions(8192), which works well for Graphics.
But for Framebuffer restricting both w and h to maxAllowedPixelDimensions(8192) results in a CONTEXT_LOST_WEBGL error.
Oddly, the error doesn't occur when only one dimension is limited to 8192 and the other is kept below 4000.

Is there something I might have overlooked or misunderstood in this context? Any insights on what might be causing this behaviour?

davepagurek

Oddly, the error doesn't occur when only one dimension is limited to 8192 and the other is kept below 4000.

I think this SO answer's explanation also applies here: basically, gl.MAX_TEXTURE_SIZE is definitely the max that can be supported in any dimension, but it doesn't make any guarantees that you have enough memory to store MAX x MAX textures, or even if you can store one, that you can store a second, if you choose to make it.

Do you run into issues with framebuffers only when the main canvas is also really big? Or do they work if you have a tiny main canvas but a big framebuffer? If it's the latter, it sounds like maybe it's running into a memory limit.

In any case, I think we won't be able to prevent all context losses when trying to make a large texture, but I think the checks being done here are a helpful start!

davepagurek · 2023-12-28T15:31:48Z

src/core/p5.Graphics.js

+        maxTextureSize / this._pixelDensity
+      );
+      w = Math.min(w, maxAllowedPixelDimensions);
+      h = Math.min(h, maxAllowedPixelDimensions);


We might want to log a warning here (and in p5.Framebuffer.js) if the width/height we end up picking is different than what the user requested.

davepagurek · 2023-12-28T15:32:38Z

src/core/p5.Graphics.js

+      const gl = this._renderer.GL;
+      const maxTextureSize = gl.getParameter(gl.MAX_TEXTURE_SIZE);
+      const maxAllowedPixelDimensions = Math.floor(
+        maxTextureSize / this._pixelDensity


Ah yes, this depends on the density. I think this means that we may need to do a similar check when the user sets pixelDensity() too for complete coverage

davepagurek · 2023-12-28T15:33:44Z

src/core/p5.Graphics.js

@@ -60,6 +60,13 @@ p5.Graphics = class extends p5.Element {

    if (r === constants.WEBGL) {
      this._renderer = new p5.RendererGL(this.canvas, this, false);


I think we want this for both graphics and the main canvas, so maybe we should do the check inside RendererGL instead?

davepagurek · 2023-12-28T15:34:36Z

src/core/p5.Graphics.js

@@ -60,6 +60,13 @@ p5.Graphics = class extends p5.Element {

    if (r === constants.WEBGL) {
      this._renderer = new p5.RendererGL(this.canvas, this, false);
+      const gl = this._renderer.GL;
+      const maxTextureSize = gl.getParameter(gl.MAX_TEXTURE_SIZE);


This theoretically shouldn't change over time, so maybe we can add a method to RendererGL to be something like this:

maxTextureSize() { if (!this._maxTextureSize) { this._maxTextureSize = this.GL.getParameter(this.GL.MAX_TEXTURE_SIZE); } return this._maxTextureSize; }

getParameter calls are generally a bit slow, so we want to cache their results (or avoid calling them if there's a convenient way to do so.)

…blin/p5.js into enhancement/limit-textures

davepagurek

Thanks for making the changes so far! I think the remaining changes would be:

can we also handle this in resizeCanvas (for the main canvas and p5.Graphics) and resize(forp5.Framebuffer`)?
can we add some unit tests for this? You can use sinon.stub(object, "method", func); to replace a system function with another function for easier testing (e.g. used here). Maybe you could use that to mock a specific max texture size (even like 100, just for testing) and make assertions on other things working accordingly

capGoblin · 2024-01-16T17:24:32Z

Hi @davepagurek,

I have spent significant time in debugging this. Strangely, I encountered a completely different error when running npm test, and after pushing the code, yet another error emerged.
Could you please assist me by suggesting any potential areas that might be causing this error? Also, let me know if the tests I wrote looks good to you

davepagurek · 2024-01-17T19:15:11Z

test/unit/core/rendering.js

+      myp5.createCanvas(10, 10, myp5.WEBGL);
+      myp5.resizeCanvas(200, 200);
+      assert.equal(myp5.canvas.width, 100);
+      assert.equal(myp5.canvas.height, 100);


Do you think myp5.width and myp5.height should also be 100 here? I think currently they aren't because we set this.width and this.height before adjusting the sizes, but I'm not sure just yet what the expected result would be. I think I lean towards having those values also be 100 for consistency, what do you think?

davepagurek · 2024-01-19T00:23:24Z

Looks like some of the tests are failing. It might be that in the test environment on CI, the max texture size is something unexpected. Does the same thing happen when you run npm run ci locally? That might help us figure out if it's something about github's test runner, or something else.

capGoblin · 2024-01-19T22:39:10Z

Sorry forgot to add comment during the previous commit, after updating this.height and this.width to adjusted sizes, by that time I was using glStub.restore() inside afterEach(), and I did npm run test all tests passed.
Since we are only using glStub in a couple of tests made sense to use glStub.restore() inside the tests itself. So, pushed the code after this change. And the tests failed.

Now, this commit uses glStub.restore() inside afterEach(), tests passed. But why does this happen?

P.S. I just tried glStub.restore() inside the test("it resizes the graphics based on max texture size") in p5.Graphics,
I usually get this (often),

davepagurek

I'm honestly not totally sure why doing it per-test doesn't work but doing it in an afterEach does. I'm glad you were able to find a fix though!

I think this is good to go now! Since we've started our release process with a beta build, I'm going to hold off on merging this until after we finish the release so that we don't accidentally introduce new bugs during the testing period. I'll merge this right after that though!

Qianqianye · 2024-03-19T20:11:36Z

Looks great! Thanks @capGoblin and @davepagurek!

capGoblin added 5 commits December 23, 2023 22:10

limit textures in p5.Graphics

f223d9e

Merge branch 'processing:main' into enhancement/limit-textures

6f5ea64

limit textures in p5.Framebuffer

7c92d81

Merge remote-tracking branch 'origin/enhancement/limit-textures' into…

d469bed

… enhancement/limit-textures

Merge branch 'processing:main' into enhancement/limit-textures

4a268fe

capGoblin changed the title ~~Enhancement/limit textures~~ Enhancement limit WebGL textures to gl.MAX_TEXTURE_SIZE Dec 24, 2023

davepagurek reviewed Dec 28, 2023

View reviewed changes

capGoblin added 3 commits December 30, 2023 21:15

refactor code

e755c68

Merge branch 'enhancement/limit-textures' of https://github.com/capGo…

b4c9787

…blin/p5.js into enhancement/limit-textures

fix w and h set to zero

d537edb

davepagurek reviewed Jan 7, 2024

View reviewed changes

add resize for framebuffer & resizeCanvas w/ unit tests

d7d794e

capGoblin requested a review from davepagurek January 16, 2024 17:26

davepagurek reviewed Jan 17, 2024

View reviewed changes

fix unit tests

fa2e615

fix unit tests

6d8919f

davepagurek approved these changes Jan 21, 2024

View reviewed changes

davepagurek mentioned this pull request Feb 22, 2024

1.9.1 Release #6822

Closed

3 tasks

Qianqianye merged commit 7ba5dc2 into processing:main Mar 19, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhancement limit WebGL textures to gl.MAX_TEXTURE_SIZE #6651

Enhancement limit WebGL textures to gl.MAX_TEXTURE_SIZE #6651

capGoblin commented Dec 24, 2023 •

edited

Loading

capGoblin commented Dec 24, 2023

davepagurek left a comment

davepagurek Dec 28, 2023

davepagurek Dec 28, 2023

davepagurek Dec 28, 2023

davepagurek Dec 28, 2023

davepagurek left a comment

capGoblin commented Jan 16, 2024 •

edited

Loading

davepagurek Jan 17, 2024

davepagurek commented Jan 19, 2024

capGoblin commented Jan 19, 2024

davepagurek left a comment

Qianqianye commented Mar 19, 2024

		@@ -60,6 +60,13 @@ p5.Graphics = class extends p5.Element {

		if (r === constants.WEBGL) {
		this._renderer = new p5.RendererGL(this.canvas, this, false);

Enhancement limit WebGL textures to gl.MAX_TEXTURE_SIZE #6651

Enhancement limit WebGL textures to gl.MAX_TEXTURE_SIZE #6651

Conversation

capGoblin commented Dec 24, 2023 • edited Loading

PR Checklist

capGoblin commented Dec 24, 2023

davepagurek left a comment

Choose a reason for hiding this comment

davepagurek Dec 28, 2023

Choose a reason for hiding this comment

davepagurek Dec 28, 2023

Choose a reason for hiding this comment

davepagurek Dec 28, 2023

Choose a reason for hiding this comment

davepagurek Dec 28, 2023

Choose a reason for hiding this comment

davepagurek left a comment

Choose a reason for hiding this comment

capGoblin commented Jan 16, 2024 • edited Loading

davepagurek Jan 17, 2024

Choose a reason for hiding this comment

davepagurek commented Jan 19, 2024

capGoblin commented Jan 19, 2024

davepagurek left a comment

Choose a reason for hiding this comment

Qianqianye commented Mar 19, 2024

capGoblin commented Dec 24, 2023 •

edited

Loading

capGoblin commented Jan 16, 2024 •

edited

Loading