Fixes tint in 2d canvas with webgl p5.Graphics

[ShenpaiSharma]

Resolves #5046

Changes:
Added condition while rendering webgl such that it reads a block of pixels from a specified shape of the current color framebuffer into an ArrayBufferView object.

Screenshots of the change:
After change:

PR Checklist

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

[ShenpaiSharma]

Hi, @montoyamoraga @stalgiag Can you please review my PR

[stalgiag]

Hi @ShenpaiSharma this is a good start! We might want to take a slightly different approach to make sure we don't repeat code unnecessarily. This will make it easier to fix problems that might arise in the future. Read my comment here where I outline why this might be better handled in a slightly different way.

[ShenpaiSharma]

I am sorry, @stalgiag. I had my mid-semester exams, so I thought before going on a break, I should try this quick fix first.
Well I thought of using const r = renderer || constants.P2D; instead of getContext to check whether context in 2D or 3D but realised in loadPixels there are various canvas property is utilised so this.drawingContext may do the trick for getContext but _toPixels maybe doesn't inherit property from RendererGL and also _getTintedImageCanvas which is used by Renderer2D but missing in RendererGL. I thought these were the reason why passing renderer maybe not a good idea here.
Please correct me if I am wrong and whether I am going in the right direction or not.

[stalgiag]

No worries! I am having a bit of trouble following. Can you try explaining in another way? Thank you!

[ShenpaiSharma]

Hi, @stalgiag well, I tried this code initially:

import * as constants from '../core/constants';

Filters._toPixels = function(renderer, canvas) {
  const r = renderer || constants.P2D;
  let gl = canvas.getContext('2d');
  if (r === constants.WEBGL) {
    gl = canvas.getContext('webgl');
  }
  if (canvas instanceof ImageData) {
    return canvas.data;
  } else {
    return gl.getImageData(0, 0, canvas.width, canvas.height).data;
  }
};

but it showed the same error as previously and the code that you have given :

What I feel is that instead, the tint has been made available in WebGL, but most of the tint property is not inherited by rendererGL class, which is used by renderer2D like _getTintedImageCanvas, etc...

[stalgiag]

Hi @ShenpaiSharma -- I think I understand. Have you tried passing only the renderer:

Filters._toPixels = function(renderer) {
  const data;
  if (!renderer.isP3D) {
    data = renderer.canvas.data;
  } else {
     data = new Uint8Array(len);
      gl.readPixels(
        0,
        0,
        canvas.width,
        canvas.height,
        gl.RGBA,
        gl.UNSIGNED_BYTE,
        data
      );
  }
  return data;
};

Note that this is very much untested and I wrote it quickly so it would be up to you to see if that general approach worked. Also this still repeats a lot of code from loadPixels but I think that could be okay.

[ShenpaiSharma]

Hi, @stalgiag. I have tried a completely different approach, and it worked perfectly. Can you please review it?
Chnage:

[ShenpaiSharma]

Well, I have tried passing renderer only too, but it was always showing some error. As I said earlier in my comment, maybe it could be due to that.
And with WebGL context, I think there is no other option than we have to use gl.readPixels().
So I tried this, which at least does not repeat code from loadPixels().

[stalgiag]

Hi @ShenpaiSharma I forgot to include that line. You can access gl as a property on the renderer when it is RendererGL with const gl = renderer.GL.

I don't think we need to create a new 2d canvas for getting the pixels.

[ShenpaiSharma]

Hi @stalgiag yeah I have seen that but problem is that whenever I am passing renderer it is showing bunch of error in console.

[stalgiag]

Hi @ShenpaiSharma I took some time to look at this in detail today. Changing things around so that a renderer is passed instead of a canvas object would require changing a number of different functions. This is too much work for what should be a fix. Thanks for trying anyway though!

I don't think we need to draw to the offscreen canvas. The first suggestion I made in the issue and the first commit that you made both work well enough. If you add a comment explaining that 'this approach is not ideal but put in place as a quick fix for a bug' then I think it will be okay to merge.

[ShenpaiSharma]

Hi @stalgiag, I tried to go through the whole rendering pipeline and tried to change the files according to it, but I still got some errors while passing renderer in the function.

[ShenpaiSharma]

Well, with a "quick fix," I have tried one more way, but it is with the use of 2d offscreen Canvas. But as you can see, it is encountering both of the problems that we have started with:

Filters._toPixels = function(canvas) {
  if (canvas instanceof ImageData) {
    return canvas.data;
  } else {
    const offCanvas = document.createElement('canvas');
    offCanvas.width = canvas.width;
    offCanvas.height = canvas.height;
    const gl = offCanvas.getContext('2d');
    gl.drawImage(canvas, 0, 0);
    const imageData = gl.getImageData(0, 0, offCanvas.width, offCanvas.height)
      .data;
    return imageData;
  }
};

And if it is '2d', we can use gl.getImageData(), while for 'webgl' we can use gl.readPixels() to get a specific target pixel region.
But also, we can't have two different contexts on the same canvas. So we can also read the pixels using this.

I have also tested it was working quite fine:

So I need your views on it. Otherwise, I think we are good with the first commit

[stalgiag]

@ShenpaiSharma I think the first commit is the better approach of the two. It is easier to understand what is happening. It is unfortunate to getContext so many times but I think this is preferable to creating another canvas. Also, readPixels is the preferred way to get pixel data from a webgl context.

[ShenpaiSharma]

Hi @stalgiag, I understand, then it will be okay to merge the first commit. Thanks

[ShenpaiSharma]

Oh, sorry forgot to revert the last commit. Now I think it should be ready to get merged

[stalgiag]

Thank you @ShenpaiSharma !