Solves issue #6502 #6503

perminder-17 · 2023-10-28T11:47:19Z

Resolves #6502

Changes:

Screenshots of the change:

PR Checklist

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

perminder-17 · 2023-10-28T11:48:48Z

Is this the desired result @davepagurek ?

davepagurek · 2023-10-28T12:34:11Z

Thanks for opening this PR!

I think this might still break if one doesn't translate before calling filter. Maybe after the push() we can call resetMatrix() to ensure the image() call doesn't accumulate any translations/rotations/scales from the sketch?

Also while we're at it, to fully resolve all the issues mentioned, can we clear() and blendMode(BLEND) before image(...)?

Garima3110 · 2023-10-28T13:02:32Z

hey @perminder-17 the problem still persists with this. Please let me know If I could also contribute to this issue in any way.
Thank you!!

Garima3110 · 2023-10-28T13:22:26Z

#6504
Hey @davepagurek , Considering this:

I think this might still break if one doesn't translate before calling filter. Maybe after the push() we can call resetMatrix() to ensure the image() call doesn't accumulate any translations/rotations/scales from the sketch?

Also while we're at it, to fully resolve all the issues mentioned, can we clear() and blendMode(BLEND) before image(...)?

Is this what you are trying to say ?

 this._pInst.push();
 this._pInst.resetMatrix(); // Reset the transformation matrix to identity
 this._pInst.clear(); // Clear the canvas
 this._pInst.blendMode(this._pInst.BLEND); // Set blend mode to BLEND

 this._pInst.noStroke();
 this._pInst.image(pg,-this.width/2,-this.height/2,this.width,this.height);

 this._pInst.pop();

Please let me know if some more changes need to be made !?
Thank you!

perminder-17 · 2023-10-28T23:50:29Z

Thanks for opening this PR!

I think this might still break if one doesn't translate before calling filter. Maybe after the push() we can call resetMatrix() to ensure the image() call doesn't accumulate any translations/rotations/scales from the sketch?

Also while we're at it, to fully resolve all the issues mentioned, can we clear() and blendMode(BLEND) before image(...)?

You were right @davepagurek I tested my previous code and I found it breaks when I doesn't translate before calling filter. Now, it's perfectly working for this one. But for this code:-

function setup() {
createCanvas(400, 400);
let gr=createGraphics(100,100);
gr.textAlign(CENTER,CENTER);
gr.textSize(80);
gr.text("🦊",50,50);
gr.filter(INVERT);
background(128);
image(gr,100,100);
}

It still breaking as shown in the issue #6502 . Is this a bug? If yes then I was thinking to make the backgroundColor of gr(gr.background) equal to the background color of the main background. Or something else, am I missing with? Idk.

inaridarkfox4231 · 2023-10-30T11:21:59Z

for example...

If you apply the INVERT filter to a gray with a transparency of 0.5, the gray will still be gray even if you apply INVERT, so it is natural that it will become a gray with a transparency of 0.5, and the version 1.7.0 filter (INVERT) achieves that.

However, the INVERT filter in version 1.8.0 seems to make this almost completely white.

function setup() {
    createCanvas(400,400);
    noStroke();
    fill(128);
    rect(0,0,400,200);
    fill(128,128);
    rect(0,200,400,200);
    filter(INVERT);
    // version 1.7.0: [128,128,128,128]
    // version 1.8.0: [255,255,255,224]
    console.log(get(200,300));
}

version 1.7.0

version 1.8.0

inaridarkfox4231 · 2023-10-30T12:20:49Z

I have decided to open a separate issue regarding transparency issues. Therefore, it seems that there is no need to handle this in this pull request.
#6514

inaridarkfox4231 · 2023-10-30T15:33:48Z

I felt that 2D examples were not enough, so I would like to add 3D examples as well. Because validation should consider various use cases.

FILTER_BUG_3D

function setup() {
  createCanvas(400, 400, WEBGL);
  noStroke();
}
function draw(){
  background(0);
  fill("red");
  lights();
  //push();
  rotateX(frameCount*TAU/240);
  rotateZ(frameCount*TAU/240);
  box(100);
  //pop();
  filter(INVERT);
}

2023-10-31.00-07-37.mp4

To be honest, I'm surprised that even this level of use case was not considered. Considering various kinds of use cases is a natural part of implementing a specification, so I would like contributors to use their imagination more.

Qianqianye · 2023-10-30T21:38:14Z

Hi @inaridarkfox4231, thanks for bringing up this problem. We understand your concerns about the new features not meeting your expectations. It's important for us to create an environment where all contributors, especially those who are new, feel encouraged and supported as they work on p5.js. We are continuously learning how to improve this collaborative process with all contributors, stewards, and maintainers.

If you have any ideas for improving existing features or if you'd like to request new features, please kindly open a new issue with your suggestions. In the meantime, please uphold a respectful tone when discussing the work of other contributors, whose efforts we deeply value and appreciate. Thank you!

davepagurek · 2023-10-30T22:07:17Z

I also think it might be a good idea to have a bit of a testing phase before releases so that we have a chance to catch anything that slips by, and reduce some of the pressure on contributors and reviewers to think of all possible uses cases. I opened a discussion here if you want to weigh in! #6517

davepagurek · 2023-10-30T22:18:42Z

Anyway @perminder-17 @Garima3110 I'll reply in the issue so I can keep feedback about this in one place.

perminder-17 · 2023-10-30T23:04:18Z

Anyway @perminder-17 @Garima3110 I'll reply in the issue so I can keep feedback about this in one place.

Thanks to you @davepagurek for your great suggestions. Your feedback means a lot to us.

inaridarkfox4231 · 2023-10-31T00:18:22Z

It is often very difficult to consider all use cases. It would be more easier to deal with the bug after it is found, and I think it will be put into practical use if there are no problems with normal use.

Examples of BLUR usage that have come across my timeline

But in this case, I don't think it was that difficult. Since the original filter handles all RGBA, I think they should have also considered processing when the background is transparent. It is natural to create examples in both 2D and 3D if we are introducing it to webgl. I felt like the pull request author only looked at the example in the reference.

Reference examples are often simple, so they are often not enough. Contributors must be both coders and users. I think we just have to keep that in mind. It's easier to fix bugs later, but we should at least use our imagination.

inaridarkfox4231 · 2023-10-31T10:02:33Z

However, the new filter() can be thought of as such a function...
BLUR_P5

2023-10-31.11-22-41.mp4

Putting that aside, the current filter() function works correctly if the following conditions are met:

In 2D,
transform is reset before use,
when fully opaque and blendMode is BLEND.

The reference example satisfies all of these conditions, so the bug was overlooked.
If we expect the same behavior as conventional filter() even in cases where this is not the case, we will need to change the specifications somehow.
If we want it to work in 3D, we need a 3D sample, and if we want it to be unaffected by the transform, we need a sample to manipulate the transform immediately before. If think about it calmly, it might not be that difficult.

inaridarkfox4231 · 2023-11-02T17:15:09Z

I'll give you a 3D example where the current commit doesn't work. Users have the right to freely configure their cameras.

3D_GRAY

function setup() {
  createCanvas(400, 400, WEBGL);
  camera(400,400,400,0,0,0,0,0,-1);
  noStroke();
}

function draw() {
  orbitControl();
  background(0);
  lights();
  fill("red");
  specularMaterial(64);
  plane(400);
  fill("orange");
  sphere(80);

  filter(GRAY);
}

2023-11-03.01-52-55.mp4

resetMatrix() is the process of returning to the view of the currently set camera, so if you want to draw the board, you must also consider the camera.
Another thing I noticed about the commit is the use of clear().

3D_DEPTH

function setup() {
  createCanvas(400, 400, WEBGL);
  camera(400,400,400,0,0,0,0,0,-1);
  noStroke();
}

function draw() {
  orbitControl();
  background(64, 128, 96);
  lights();
  specularMaterial(64);
  fill("orange");
  sphere(60);

  filter(GRAY);

  fill("red");
  torus(120, 20, 48);
}

2023-11-03.02-01-35.mp4

Let's just ignore the fact that GRAY is not applied at all... (it can't be helped).
If the commit passes with the current policy, there is a concern that the depth will be cleared by clear(). This code draws the torus after filtering it, but if depth is cleared in that case, the torus may not wrap around behind the sphere (I might be thinking too much....).
For example, you can express that only certain objects are colored in a monochrome world.

2023-11-03.02-08-28.mp4

However, since the filter function in webgl has just been implemented, I think you are free to do whatever you want. So I think you should be free to decide.

davepagurek · 2023-11-02T18:17:09Z

I think it's ok to continue to ignore depth as filters mostly operate in image space for now. Filters like blur (and filters like these https://github.com/SableRaf/Filters4Processing, which are for Processing and not p5 but are the sorts of things we aim to support with custom filters) will distort the image and change what the depth mapping should be.

inaridarkfox4231 · 2023-11-03T00:14:43Z

I see, depending on the situation, it may be better to change the depth. I understand!

inaridarkfox4231 · 2023-11-03T00:39:22Z

It should be free to decide, I think. In the first place, in the community I belong to, they think the current filter() does not cause any bugs, so I will not be involved any further.
I saw some examples of that, and it makes sense that they were only looking at the filter function under the conditions listed earlier. The basic idea is to operate in 2D, I got it. But if that's the case, why does filter() work in 3D as well? The phrase "filter() cannot be used in webgl" has been removed already.
I don't know how p5 aims for filter() to function in 3D, so I'll refrain from getting involved with it any further. I'll leave everything to you.

davepagurek · 2023-11-03T00:48:43Z

I don't know how p5 aims for filter() to function in 3D

It's something new for us too, so it's still open for discussion! It would be great to discuss more details in a feature enhancement issue. For now this issue/PR can probably just keep things simple and not worry about it yet though.

davepagurek · 2023-11-03T16:49:15Z

src/webgl/p5.RendererGL.js

+    pg._pInst.blendMode(constants.BLEND);
+    this.clear();
+    this._pInst.push();
+    pg._pInst.setCamera(this.filterCamera);


I think this one should be this._pInst.setCamera(...), since we're using it for the this._pInst.image call below.

WhatsApp.Video.2023-11-03.at.21.06.48_d5719e17.mp4

this is the output.

let me use this._pInst.setCamera()

davepagurek · 2023-11-03T16:52:12Z

src/webgl/shaders/filters/invert.frag

-  gl_FragColor = vec4(invertedColor, color.a);
+  vec3 invertedColor = vec3(1.0) - color.rgb; // Declare and calculate invertedColor
+  //checking if pixel is opaque
+  if (color.a == 1.0) { 


I think this will still break for colors that are semitransparent, since they'll go through the else branch.

To explain a bit about what's happening, the colors here are premultiplied, meaning color.r is actually the original r value multiplied by color.a.

So if we want to invert just the original colors but keep the alpha, maybe we need:

vec3 origColor = color.rgb / color.a; vec3 invertedColor = vec3(1.0) - origColor; gl_FragColor = vec4(invertedColor * color.a, color.a);

perminder-17 · 2023-11-03T22:03:09Z

Done with the semiTransparent invert. Thankyou @davepagurek for your suggestion.

perminder-17 · 2023-11-04T16:23:53Z

Some tests that were not working previously are now functioning perfectly. Here are images of the updated work.

20231104214219.mp4

20231104214446.mp4

davepagurek · 2023-11-05T01:25:06Z

src/image/pixels.js

@@ -614,7 +614,8 @@ p5.prototype.filter = function(...args) {
      // dest coords
      -this.width/2, -this.height/2, this.width, this.height
    );
-
+    //clearing the main canvas
+    this._renderer.clear();
    // filter it with shaders
    this.filterGraphicsLayer.filter(operation, value);


~~I think we might still need to put the image() call below in between a push(), resetMatrix(), and pop() similar to what we do for WebGL, since it looks like it still has an offset here.~~

Edit: ignore me, I was testing on the wrong p5 build!

Okay. No problem

davepagurek

Ok I've done a little testing myself and I think this is good to merge! Thanks for your work on this!

perminder-17 · 2023-11-05T01:46:15Z

Ok I've done a little testing myself and I think this is good to merge! Thanks for your work on this!

Thankyou for your support and suggestions.

limzykenneth · 2023-11-06T12:39:53Z

@davepagurek If the problem in this issue is fixed and there are no other immeidately pending fix, shall we arrange for a release soon @Qianqianye?

inaridarkfox4231 · 2023-11-06T13:30:16Z

Am I wrong in saying that OPAQUE doesn't seem to be fixed?
OPAQUE
If OPAQUE exhibits the same behavior as 1.7.0, the image should be opaque.

Excuse me if I am wrong.

davepagurek · 2023-11-06T13:36:38Z

Right, looks like OPAQUE has a separate issue, I'll open a new one for that.

davepagurek · 2023-11-06T14:50:28Z

@limzykenneth I made two PRs (#6529 and #6530) to address the open filter issues. I think after that we could be ready to go. Do we want to do a pre-release build for this one? While it's mostly bug fixes, we also have #6255 merged.

limzykenneth · 2023-11-09T13:10:31Z

I probably need to modify the release script to support pre-release tag to not break things. Depending on when I get time to do that if we merge the fixes before I'm done we can just to a patch release first and think about pre-release for next time.

Qianqianye requested a review from davepagurek October 30, 2023 01:31

inaridarkfox4231 mentioned this pull request Oct 30, 2023

Canvas becomes strange when using translate() and filter function in 2D mode #6502

Closed

17 tasks

perminder-17 closed this Nov 3, 2023

perminder-17 force-pushed the bluring branch from 5de6497 to 39b50ec Compare November 3, 2023 16:25

Update invert.frag

3948334

perminder-17 reopened this Nov 3, 2023

perminder-17 added 2 commits November 3, 2023 22:10

Update p5.RendererGL.js

44cf277

Update pixels.js

79a7360

davepagurek reviewed Nov 3, 2023

View reviewed changes

Update p5.RendererGL.js

14524a3

perminder-17 added 2 commits November 4, 2023 03:25

Update invert.frag

28a83c5

Update p5.RendererGL.js

2a50776

perminder-17 requested a review from davepagurek November 3, 2023 22:03

davepagurek reviewed Nov 5, 2023

View reviewed changes

davepagurek approved these changes Nov 5, 2023

View reviewed changes

davepagurek merged commit 17304ce into processing:main Nov 5, 2023
2 checks passed

perminder-17 deleted the bluring branch November 5, 2023 01:48

perminder-17 restored the bluring branch November 5, 2023 01:49

davepagurek mentioned this pull request Nov 5, 2023

webgl's filter function exhibits different behavior in handling transparency than in 1.7.0 #6514

Closed

17 tasks

limzykenneth mentioned this pull request Nov 9, 2023

Solves issue #6502 #6504

Closed

3 tasks

perminder-17 mentioned this pull request Feb 27, 2024

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Solves issue #6502 #6503

Solves issue #6502 #6503

perminder-17 commented Oct 28, 2023 •

edited

Loading

perminder-17 commented Oct 28, 2023

davepagurek commented Oct 28, 2023

Garima3110 commented Oct 28, 2023 •

edited

Loading

Garima3110 commented Oct 28, 2023 •

edited

Loading

perminder-17 commented Oct 28, 2023 •

edited

Loading

inaridarkfox4231 commented Oct 30, 2023

inaridarkfox4231 commented Oct 30, 2023

inaridarkfox4231 commented Oct 30, 2023 •

edited

Loading

Qianqianye commented Oct 30, 2023

davepagurek commented Oct 30, 2023

davepagurek commented Oct 30, 2023

perminder-17 commented Oct 30, 2023

inaridarkfox4231 commented Oct 31, 2023

inaridarkfox4231 commented Oct 31, 2023 •

edited

Loading

inaridarkfox4231 commented Nov 2, 2023

davepagurek commented Nov 2, 2023

inaridarkfox4231 commented Nov 3, 2023

inaridarkfox4231 commented Nov 3, 2023

davepagurek commented Nov 3, 2023

davepagurek Nov 3, 2023

perminder-17 Nov 3, 2023

perminder-17 Nov 3, 2023

davepagurek Nov 3, 2023

perminder-17 commented Nov 3, 2023

perminder-17 commented Nov 4, 2023

davepagurek Nov 5, 2023 •

edited

Loading

perminder-17 Nov 5, 2023 •

edited

Loading

davepagurek left a comment

perminder-17 commented Nov 5, 2023 •

edited

Loading

limzykenneth commented Nov 6, 2023

inaridarkfox4231 commented Nov 6, 2023

davepagurek commented Nov 6, 2023

davepagurek commented Nov 6, 2023

limzykenneth commented Nov 9, 2023

Solves issue #6502 #6503

Solves issue #6502 #6503

Conversation

perminder-17 commented Oct 28, 2023 • edited Loading

PR Checklist

perminder-17 commented Oct 28, 2023

davepagurek commented Oct 28, 2023

Garima3110 commented Oct 28, 2023 • edited Loading

Garima3110 commented Oct 28, 2023 • edited Loading

perminder-17 commented Oct 28, 2023 • edited Loading

inaridarkfox4231 commented Oct 30, 2023

version 1.7.0

version 1.8.0

inaridarkfox4231 commented Oct 30, 2023

inaridarkfox4231 commented Oct 30, 2023 • edited Loading

Qianqianye commented Oct 30, 2023

davepagurek commented Oct 30, 2023

davepagurek commented Oct 30, 2023

perminder-17 commented Oct 30, 2023

inaridarkfox4231 commented Oct 31, 2023

inaridarkfox4231 commented Oct 31, 2023 • edited Loading

inaridarkfox4231 commented Nov 2, 2023

davepagurek commented Nov 2, 2023

inaridarkfox4231 commented Nov 3, 2023

inaridarkfox4231 commented Nov 3, 2023

davepagurek commented Nov 3, 2023

davepagurek Nov 3, 2023

Choose a reason for hiding this comment

perminder-17 Nov 3, 2023

Choose a reason for hiding this comment

perminder-17 Nov 3, 2023

Choose a reason for hiding this comment

davepagurek Nov 3, 2023

Choose a reason for hiding this comment

perminder-17 commented Nov 3, 2023

perminder-17 commented Nov 4, 2023

davepagurek Nov 5, 2023 • edited Loading

Choose a reason for hiding this comment

perminder-17 Nov 5, 2023 • edited Loading

Choose a reason for hiding this comment

davepagurek left a comment

Choose a reason for hiding this comment

perminder-17 commented Nov 5, 2023 • edited Loading

limzykenneth commented Nov 6, 2023

inaridarkfox4231 commented Nov 6, 2023

davepagurek commented Nov 6, 2023

davepagurek commented Nov 6, 2023

limzykenneth commented Nov 9, 2023

perminder-17 commented Oct 28, 2023 •

edited

Loading

Garima3110 commented Oct 28, 2023 •

edited

Loading

Garima3110 commented Oct 28, 2023 •

edited

Loading

perminder-17 commented Oct 28, 2023 •

edited

Loading

inaridarkfox4231 commented Oct 30, 2023 •

edited

Loading

inaridarkfox4231 commented Oct 31, 2023 •

edited

Loading

davepagurek Nov 5, 2023 •

edited

Loading

perminder-17 Nov 5, 2023 •

edited

Loading

perminder-17 commented Nov 5, 2023 •

edited

Loading