Adding framebuffer support for filter() + CreateFilterShader for 2D mode #6559

perminder-17 · 2023-11-16T11:09:03Z

Changes:

Screenshots of the change:

20231116162522.mp4

PR Checklist

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

perminder-17 · 2023-11-16T11:13:56Z

@davepagurek the code is working for all three cases (createGraphics, createFramebuffer, and normal). The yScale of the camera was inverted when called inside the framebuffer, so I adjusted the yScale with curCamera. Let me know if there's a chance this code might break. Also, I'm not sure if resizing the canvas the way I did is correct for framebuffers.

perminder-17 · 2023-11-20T19:09:53Z

UPDATED BUILD OF THE CODE FOR CHECKING PURPOSE
https://editor.p5js.org/aman12345/sketches/gYH9Q4CJm

davepagurek · 2023-11-21T14:04:41Z

This is looking good so far!

Some remaining tasks:

It looks like filter(BLUR) still breaks
I was trying to test createFilterShader. It seems to work in WebGL mode, but I forgot that this PR has not yet been merged to make sure it works in 2D mode: createFilterShader docs + fixes #6530 I'm going to see if I can get that in soon, but in the mean time, it might be worth merging that into your branch too so you can make sure filter shaders still work correctly in 2D canvases

davepagurek · 2023-11-21T14:06:39Z

src/webgl/p5.RendererGL.js

  filter(...args) {
+
+    // Treating 'pg' as a framebuffer.


Maybe we can rename this from pg? I think pg is a holdover from Processing where and the PGraphics class, whose p5 equivalent is p5.Graphics. Now that we're using a framebuffer, it might be less confusing to call this something else (often in p5 and in other Web/OpenGL code you use fbo, for "framebuffer object")

davepagurek · 2023-11-21T14:10:52Z

src/webgl/p5.RendererGL.js

+
+    // Set the yScale of the filterCamera for framebuffers.
+    if (target !== this) {
+      this.filterCamera.yScale = this._curCamera.yScale;


I think we're always using the filterCamera on a framebuffer now so I think its yScale should always be -1 (which is what your logic currently does as well.) But maybe a simpler way of achieving the same thing would be, when we define this.filterCamera, to say this.filterCamera = this.getFilterGraphicsLayer().createCamera() -- that way, its size will always match the size of the filter graphic when it resizes, and we won't need to touch its yScale value since it will be initialized to the correct value.

davepagurek · 2023-11-21T14:11:55Z

src/webgl/p5.RendererGL.js

  filter(...args) {
+
+    // Treating 'pg' as a framebuffer.
    let pg = this.getFilterGraphicsLayer();


Maybe we should rename this and getFilterGraphicsLayerTemp to not mention Graphics any more? Maybe just getFilterLayer and getFilterLayerTemp?

perminder-17 · 2023-11-21T14:13:09Z

still

This is looking good so far!

Some remaining tasks:

It looks like filter(BLUR) still breaks

I was trying to test createFilterShader. It seems to work in WebGL mode, but I forgot that this PR has not yet been merged to make sure it works in 2D mode: createFilterShader docs + fixes #6530 I'm going to see if I can get that in soon, but in the mean time, it might be worth merging that into your branch too so you can make sure filter shaders still work correctly in 2D canvases

Okay, @davepagurek can you please provide me the code where the filter(BLUR) is brekaing?? I will try to figure it out

davepagurek · 2023-11-21T14:30:21Z

src/webgl/p5.RendererGL.js

-      this.filterShader.setUniform('tex0', tmp);
-      pg.clear();
-      pg.rect(-this.width / 2, -this.height / 2, this.width, this.height);
+      tmp.draw(() =>


Right now this code is nested, which is causing this error and making it not render:

WebGL warning: drawElementsInstanced: Texture level 0 would be read by TEXTURE_2D unit 0, but written by framebuffer attachment COLOR_ATTACHMENT0, which would be illegal feedback.

I think a simplified order, without nesting and without extra copying of data, could be:

this.filterShader.setUniform('texelSize', texelSize); this.filterShader.setUniform('canvasSize', [target.width, target.height]); this.filterShader.setUniform('radius', Math.max(1, filterParameter)); // Horiz pass: draw `target` to `tmp` tmp.draw(() => { this.filterShader.setUniform('direction', [1, 0]); this.filterShader.setUniform('tex0', target); this._pInst.clear(); this._pInst.shader(this.filterShader); this._pInst.rect(-target.width / 2, -target.height / 2, target.width, target.height); }); // Vert pass: draw `tmp` to `pg` pg.draw(() => { this.filterShader.setUniform('direction', [0, 1]); this.filterShader.setUniform('tex0', tmp); this._pInst.clear(); this._pInst.shader(this.filterShader); this._pInst.rect(-target.width / 2, -target.height / 2, target.width, target.height); );

For testing, I just changed filter(INVERT) to filter(BLUR) in your latest p5 editor link.

Sorry @davepagurek if you come across any issues with the filter (blur), I also tested it and didn't find it breaking. but I've committed the changes, and it seems like the code is now more readable than the nested one. Thanks for your understanding

blur with nested one...from the above editor. is this not the expected behaviour??

sorry if i did'nt understood where the bug is happening.

maybe it was a browser-specific issue? in Firefox I was seeing no output on the canvas. Anyway in your most recent build it seems to be working!

Okay, got it @davepagurek

I think we no longer need this line now that we pass target in as a uniform below

perminder-17 · 2023-11-21T15:33:38Z

bluild of the code after the changes;-

https://editor.p5js.org/aman12345/sketches/WQl9tDMOs

perminder-17 · 2023-11-21T20:17:25Z

This is looking good so far!

Some remaining tasks:

It looks like filter(BLUR) still breaks

I was trying to test createFilterShader. It seems to work in WebGL mode, but I forgot that this PR has not yet been merged to make sure it works in 2D mode: createFilterShader docs + fixes #6530 I'm going to see if I can get that in soon, but in the mean time, it might be worth merging that into your branch too so you can make sure filter shaders still work correctly in 2D canvases

For this createFilterShader docs + fixes PR, should I copy all the changes into my branch and try running it locally to see whether it breaks for 2D or not without commiting any changes here? Or something else I need to do for this? If this breaks and something needs to be changed here in this PR createFilterShader docs + fixes PR then what steps should be taken??

davepagurek · 2023-11-21T20:27:53Z

src/webgl/p5.RendererGL.js

+      fbo.height !== this.height
+    ) {
+      // Resize fbo
+      fbo.resizeCanvas(this.width, this.height);


i think we'll have to do all this for tmp too. Maybe it's worth putting this into a function, something like matchSize(fboToMatch, target) so that you can call it on fbo here and also on tmp later?

davepagurek · 2023-11-21T20:32:34Z

For this createFilterShader docs + fixes PR, should I copy all the changes into my branch and try running it locally to see whether it breaks for 2D or not without commiting any changes here? Or something else I need to do for this? If this breaks and something needs to be changed here in this PR createFilterShader docs + fixes PR then what steps should be taken??

Maybe the fastest way forward is to merge those changes into your branch and get them working with your branch? And then if they're working here, I can just close my PR, since all the updates will be in this one too.

perminder-17 · 2023-11-21T22:59:09Z

here's the build after merging @davepagurek https://editor.p5js.org/aman12345/sketches/Rbz3lXYlA

perminder-17 · 2023-11-21T23:28:23Z

If anything else needs to be changed, then please let me know @davepagurek .

davepagurek · 2023-11-22T13:17:33Z

lib/empty-example/sketch.js

@@ -1,7 +0,0 @@
-function setup() {


Was this intentionally deleted? Otherwise I think we can keep this file around

Ooh... Sorry

davepagurek · 2023-11-22T13:21:55Z

src/webgl/p5.RendererGL.js

    this.clear();
    this._pInst.push();
    this.filterCamera._resize();
    this._pInst.setCamera(this.filterCamera);
    this._pInst.resetMatrix();
-    this._pInst.image(pg, -this.width / 2, -this.height / 2,
+    this._pInst.image(fbo, -this.width / 2, -this.height / 2,


I think we still need these two lines above this call:

this._pInst.imageMode(constants.CORNER); this._pInst.blendMode(constants.BLEND);

Currently, if you change the image mode or blend mode, the filter gets drawn with an offset: https://editor.p5js.org/davepagurek/sketches/q442qjF_1

perminder-17 · 2023-11-23T17:08:38Z

I have a small question, @davepagurek . In this test, we're using WEBGL for this filter? Shouldn't we change the name from filterGraphicsLayer tofilterLayer here too? However, when I make this change, the tests fail.

test('filter() uses WEBGL implementation behind main P2D canvas', function() {
      let renderer = myp5.createCanvas(3,3);
      myp5.filter(myp5.BLUR);
      assert.isDefined(renderer.filterGraphicsLayer);
    });

davepagurek · 2023-11-23T17:48:30Z

Only RendererGL has filterLayer I think, Renderer2D still has filterGraphicsLayer, which is a WebGL graphic. While WebGL mode can natively run shaders and can therefore just use a framebuffer directly, 2D mode can't run shaders, so it needs the extra step of making a WebGL graphic to run the filter on.

So:

In 2D mode, renderer.filterGraphicsLayer is a graphic, and renderer.filterGraphicsLayer.filterLayer is a framebuffer
In WebGL mode, renderer.filterLayer is a framebuffer and no graphic is involved

All that to say, I think that test is correct as it is.

perminder-17 · 2023-11-23T17:51:31Z

Only RendererGL has filterLayer I think, Renderer2D still has filterGraphicsLayer, which is a WebGL graphic. While WebGL mode can natively run shaders and can therefore just use a framebuffer directly, 2D mode can't run shaders, so it needs the extra step of making a WebGL graphic to run the filter on.

So:

In 2D mode, renderer.filterGraphicsLayer is a graphic, and renderer.filterGraphicsLayer.filterLayer is a framebuffer

In WebGL mode, renderer.filterLayer is a framebuffer and no graphic is involved

All that to say, I think that test is correct as it is.

Ooh....I was having a little confusion with this only. Thanks for your explanation.

davepagurek · 2023-11-23T17:59:16Z

src/core/p5.Renderer2D.js

+        );
+    }
+
+    return this.filterGraphicsLayer;


Before this, we might want to do a similar size matching to what we do in RendererGL with its framebuffer so that we ensure this.filterGraphicsLayer matches width, height, and pixel density to this._pInst. Right now filters on 2D mode don't seem to work when resizing: https://editor.p5js.org/davepagurek/sketches/OOKkLOINp

Maybe we can also copy and paste the current unit test that checks that the framebuffer gets resized in WebGL mode, and adapt it to check the size of filterGraphicsLayer in 2D mode? It would definitely be helpful to leave behind as many tests as we can so that we don't have to rely so much on thinking of new things to test the next time we make changes to this system.

okay, i will do the changes asap.

davepagurek

btw I just wanted to say thanks for your patience and continued work on this as we work through all the bugs! Hopefully it will be easier for future contributors the more tests we add, so they can rely more on those tests to let them know that they've fixed everything.

perminder-17 · 2023-11-23T18:05:22Z

btw I just wanted to say thanks for your patience and continued work on this as we work through all the bugs! Hopefully it will be easier for future contributors the more tests we add, so they can rely more on those tests to let them know that they've fixed everything.

btw i need to thankyou for your patience and support. Love to have a mentor like you.

davepagurek · 2023-11-23T22:08:33Z

src/core/p5.Renderer2D.js

+      this.filterGraphicsLayer.height !== this.height
+    ) {
+      // Resize the graphics layer
+      this.filterGraphicsLayer.resize(this.width, this.height);


I think in this case, since filterGraphicsLayer is a p5.Graphics, we can use resizeCanvas instead of resize. Unfortunately it's kinda confusing that the main canvas and graphics call it one thing but images and framebuffers call it something else :')

oops sorry, i commited it wrong file. sorry for this

actually take a look at the test for this. I am not good at writing tests.

davepagurek · 2023-11-23T22:10:11Z

test/unit/webgl/p5.RendererGL.js

+      let s = myp5.createShader(vert, frag);
+      myp5.filter(s);
+      myp5.resizeCanvas(5, 15);
+      assert.equal(g1.width, 5);


Technically we only care that the sizes are equal when we want to do a filter, so you might need to call myp5.filter(s) again before this assertion

perminder-17 · 2023-11-23T22:29:41Z

Are we doing the correct way for test? @davepagurek

perminder-17 · 2023-11-23T22:45:40Z

Let me know if we need something else @davepagurek ?

davepagurek · 2023-11-23T22:45:48Z

I did a little testing and agree that everything is looking good now! I think the tests are looking good. Is there anything else you can think of that we might need to add before merging?

perminder-17 · 2023-11-23T22:47:34Z

I did a little testing and agree that everything is looking good now! I think the tests are looking good. Is there anything else you can think of that we might need to add before merging?

I will open an issue for covering all the test including filter and many more from this area. I don't think we are missing something with area of filter :)

davepagurek · 2023-11-23T22:47:54Z

if everything also looks good to you then I think we're good to merge!

perminder-17 · 2023-11-23T22:49:18Z

if everything also looks good to you then I think we're good to merge!

I changed the heading of the issue? I think we are covering one more issue which we are merging (createFilterShader for 2D mode)

davepagurek

awesome, thanks for your work on this!

perminder-17 · 2023-11-23T23:26:58Z

awesome, thanks for your work on this!

Welcome. Love to contribute more like this❤

Adding framebuffer support in filter

198e456

perminder-17 added 6 commits November 16, 2023 16:45

Update p5.RendererGL.js

8b71469

Update material.js

0ec719e

Update p5.RendererGL.js

96bc262

Reset matrix.js

72cd97b

Added test for filterCamera.js

6f0b715

Checking if pixelDensity is defined.js

2789e2a

davepagurek reviewed Nov 21, 2023

View reviewed changes

perminder-17 added 2 commits November 21, 2023 20:47

Update p5.RendererGL.js

2df0b03

Update material.js

f42da57

Update p5.RendererGL.js

cefc285

davepagurek reviewed Nov 21, 2023

View reviewed changes

merging

9a86b19

perminder-17 added 2 commits November 22, 2023 04:29

Delete lib/empty-example/sketch.js

75b0042

function matchsize().js

ae37304

fixed typo.js

e807740

davepagurek reviewed Nov 22, 2023

View reviewed changes

Rename tests.js

2a330bf

davepagurek reviewed Nov 23, 2023

View reviewed changes

Qianqianye added this to the 1.9.0 milestone Nov 23, 2023

Qianqianye mentioned this pull request Nov 23, 2023

1.9.0 Release #6582

Closed

7 tasks

fixes.js

6d04ec6

davepagurek reviewed Nov 23, 2023

View reviewed changes

add test.js

b1ec441

davepagurek reviewed Nov 23, 2023

View reviewed changes

perminder-17 added 2 commits November 24, 2023 03:41

fixes.js

c01235f

fixes.js

8df7437

perminder-17 changed the title ~~Adding framebuffer support for filter()~~ Adding framebuffer support for filter() + CreateFilterShader for 2D mode Nov 23, 2023

davepagurek approved these changes Nov 23, 2023

View reviewed changes

davepagurek merged commit c0acef8 into processing:main Nov 23, 2023
2 checks passed

davepagurek mentioned this pull request Nov 23, 2023

createFilterShader docs + fixes #6530

Closed

3 tasks

perminder-17 deleted the framebuff branch November 23, 2023 23:33

perminder-17 mentioned this pull request Nov 29, 2023

lights() and specularMaterial() support on framebuffer block when filter is applied inside the block. #6596

Merged

3 tasks

davepagurek mentioned this pull request Nov 29, 2023

createFilterShader throws an error in 2D mode #6520

Closed

17 tasks

perminder-17 mentioned this pull request Feb 27, 2024

Add depth-of-field fog or blurring rendering options OpenChemistry/avogadrolibs#347

Closed

Adding framebuffer support for filter() + CreateFilterShader for 2D mode #6559

Adding framebuffer support for filter() + CreateFilterShader for 2D mode #6559

Conversation

perminder-17 commented Nov 16, 2023 • edited Loading

PR Checklist

perminder-17 commented Nov 16, 2023 • edited Loading

perminder-17 commented Nov 20, 2023

davepagurek commented Nov 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perminder-17 commented Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perminder-17 Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perminder-17 commented Nov 21, 2023

perminder-17 commented Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davepagurek commented Nov 21, 2023

perminder-17 commented Nov 21, 2023

perminder-17 commented Nov 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perminder-17 commented Nov 23, 2023 • edited Loading

davepagurek commented Nov 23, 2023

perminder-17 commented Nov 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davepagurek left a comment

Choose a reason for hiding this comment

perminder-17 commented Nov 23, 2023

Choose a reason for hiding this comment

perminder-17 Nov 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perminder-17 commented Nov 23, 2023 • edited Loading

perminder-17 commented Nov 23, 2023

davepagurek commented Nov 23, 2023

perminder-17 commented Nov 23, 2023

davepagurek commented Nov 23, 2023

perminder-17 commented Nov 23, 2023 • edited Loading

davepagurek left a comment

Choose a reason for hiding this comment

perminder-17 commented Nov 23, 2023

perminder-17 commented Nov 16, 2023 •

edited

Loading

perminder-17 commented Nov 16, 2023 •

edited

Loading

perminder-17 commented Nov 21, 2023 •

edited

Loading

perminder-17 Nov 21, 2023 •

edited

Loading

perminder-17 commented Nov 21, 2023 •

edited

Loading

perminder-17 commented Nov 23, 2023 •

edited

Loading

perminder-17 Nov 23, 2023 •

edited

Loading

perminder-17 commented Nov 23, 2023 •

edited

Loading

perminder-17 commented Nov 23, 2023 •

edited

Loading