FilterRenderer2D for a 2d-Build

[perminder-17]

This PR makes a separate class which will handle filter() function in 2D mode using mini WEBGL renderer.
We can use this for future when we try to make a 2D-build for p5.js.

[perminder-17]

This PR is ready for review. It's working as expected. @davepagurek, any thoughts or suggestions on the code structure?

Many tests are still failing and will look into it very soon and also needs some code cleanups and proper testing. Thanks

[perminder-17]

Since we are dealing with 2D models here, do you think making the matrices identity works well? I tested them and results look good to me.

[davepagurek]

I think that makes sense!

[davepagurek]

This is looking good so far! I haven't looked into the tests just yet, but I left some initial thoughts on some things we might need.

[davepagurek]

If a different filter is rendered (e.g. a blur then a threshold) I think we'll need to update the addressee used here. Might need to add an update method, and maybe also cache the shaders by their type instead of just having one cached

[davepagurek]

And I guess if a custom filter shader is passed in, we don't need to cache that, since the user holds on to the shader instance themself in that case

[perminder-17]

If a different filter is rendered (e.g. a blur then a threshold) I think we'll need to update the addressee used here.

Thanks for catching that. I have fixed that now, do you think that looks good to you.

And I guess if a custom filter shader is passed in, we don't need to cache that, since the user holds on to the shader instance themself in that case

Okay, I thought custom filterShaders will be only used for webgl graphics and not in a 2d canvas. Added a logic for users using custom filter shaders as well. Do you think that looks good? Currently I need to test the whole filter but do you think that looks good to you or have any other suggestions for improving code quality? @davepagurek

[davepagurek]

It looks like once we set this._shader, it will never update. I think that will cause this scenario to apply the first filter twice instead of doing each one once:

function draw() {
  filter(BLUR)
  filter(THRESHOLD)
}

I think maybe we should structure this in a way where we have this.filterShaderSources (storing the fragment shaders) and this.filterShaders (storing the initialized p5.Shader, if it exists). So in _initializeShader, we could just return the custom shader if a custom shader has been provided, and if not, return the stored this.filterShaders[this.operation] if it already exists, and if not, initialize a shader and store it in this.filterShaders.

[davepagurek]

This would be instead of having separate filter renderers per shader as you have it right now. The browser has a pretty low limit for the number of active WebGL contexts it can maintain (it's something like 6, and lower on phones), so we'd want to have just one filter renderer per 2D context, and have it have multiple shaders.

[davepagurek]

Hm actually we might need to do a bit of refactoring here when we initialize custom filter shaders now that we won't have a filter graphics layer:

p5.js/src/webgl/material.js

Line 677 in 69580cf

shader.ensureCompiledOnContext(this._renderer.getFilterGraphicsLayer());

Maybe what we do is always pass in this, and then in the p5.Shader.ensureCompiledOnContext implementation, we do something like context._renderer._filterRenderer || context._renderer to grab the 2D filter renderer if it exists.

p5.js/src/webgl/p5.Shader.js

Line 776 in 69580cf

this._renderer = context._renderer;

[perminder-17]

btw @davepagurek, my custom filterShader works for 2d mode now but I need to add my filterRenderer here as well and in pixel.js as well

      if (!this.filterRenderer) {
        this.filterRenderer = new FilterRenderer2D(this);
        this._renderer.filterRenderer = this.filterRenderer;
      }

any idea why this works only when I have initialized filterRenderer in both the places and by removing any of them, it stops to work?

[davepagurek]

I think this represents different things in both cases: in this file, it's a p5.Renderer2D, but in the pixels.js file, it's aninstance of p5. In that file, to access the one on the renderer, I think you need to check for this._renderer.filterRenderer.

[perminder-17]

btw @davepagurek , the tests is passing for all 2d modes I think, the test which is currently failing is when we apply rectMode(), blendMode() or imageMode() in webgl modes. I think they are also failing in the actual dev-2.0 branch because I haven't changed the code for the webgl filter which uses the actual renderer.

Here's the result.

2D mode without blendMode

2D mode with blendMode (working correctly)

WEBGL mode without blendMode

WEBGL mode with blendMode (not correct)

Let me know if you have any thoughts, I think this is the main reason of the failing tests. What you think?

[davepagurek]

Awesome, this looks great! Agreed, it looks like the failing WebGL filter tests probably weren't caused by these changes, so I think this PR is good to go!

[perminder-17]

I think the issue is fixed now, but still tests doesn't passes for now :"). Also, resolved some merge conflicts, do you think that looks good to you @davepagurek

[davepagurek]

turns out what was missing was a thing to handle default parameters (e.g. filter(BLUR) and not filter(BLUR, 3)), so I factored the code in the WebGL module that does that into its own file where both can reference it.

I also cached the p5.Texture we create so we can keep reusing it, and added a visual test that I was using for debugging. I think we're good to go now! Thanks for your work on this @perminder-17, I'm happy to see how minimal you managed to get this class to be 🙂