Updated Stencil Test Case to prevent it from disable after every draw #7578

Vaivaswat2244 · 2025-02-26T14:01:45Z

Resolves #7554

Changes:

Introduced this.isStencilTestOn and this._userEnabledStencil to track stencil state.
Overrode WebGL's enable(), disable(), and getEnabled() methods to ensure:
- The stencil test is marked as user-enabled only when explicitly set outside clip().
- pop() restores the correct stencil state after clipping operations.
Modified stencil test logic:
- Prevent unnecessary disable(STENCIL_TEST) calls when it is user-enabled.
- Restore user-enabled stencil test state after pop().

Screenshots of the change:

Used this snippet in preview/index.html,

<body>
 <script type="module">
   import p5 from '../src/app.js';

   const sketch = function (p) {
     let gl;

     p.setup = function () {
       p.createCanvas(400, 400, p.WEBGL);
       p.noStroke();
       gl = p._renderer.GL;
       gl.enable(gl.STENCIL_TEST);
     };

     p.draw = function () {
       p.background(0);
       gl.clear(gl.STENCIL_BUFFER_BIT);

       // Set stencil value to 1
       gl.stencilFunc(gl.ALWAYS, 1, ~0);
       gl.stencilOp(gl.KEEP, gl.REPLACE, gl.REPLACE);

       p.fill(255, 0, 0);
       p.plane(200, 400);

       // Only draw where stencil value is 1
       gl.stencilFunc(gl.EQUAL, 1, ~0);
       gl.stencilOp(gl.KEEP, gl.KEEP, gl.KEEP);

       p.fill(0, 0, 255);
       p.plane(400, 200);

       p.noLoop();
     };
   };

   new p5(sketch);
 </script>
</body>
</html>

SCREENSHOT:

PR Checklist

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

… loop

davepagurek · 2025-02-27T23:36:04Z

src/webgl/p5.RendererGL.js

+            // When pop() disables the stencil test after clip(), 
+            // restore the user's stencil test setting
+            if (this._clipDepth === this._pushPopDepth) {
+                this.isStencilTestOn = this._userEnabledStencil;


It's a little unintuitive that calling disable() elsewhere in the code might not actually disable. How do you feel about making enable/disable only record the last value so that getEnabled is fast, and then place this logic of what value to revert to elsewhere?

The idea would be, write the code so that the behaviour would be the same even if we haven't overridden enable and disable: e.g. when we begin clipping, we can record this.isStencilTestOn = this.drawingContext.getEnabled(this.drawingContext.STENCIL_TEST), and then in pop where we turn off stencil testing currently, we would either turn it on or off based on the value of this.isStencilTestOn.

Then, this overriding of enable/disable just makes the getEnabled call run faster, without changing what it does or its side effects.

davepagurek · 2025-02-27T23:36:43Z

Thanks for taking this task on, it's looking good! I have some comments on how we might rearrange the implementation to make the code easier for other contributors to read.

davepagurek · 2025-02-27T23:42:34Z

One other thought: if stencil testing was on, after we finish clipping in pop, do we want to clear the stencil buffer? otherwise it effectively just continues clipping.

Vaivaswat2244 · 2025-02-28T21:13:19Z

One other thought: if stencil testing was on, after we finish clipping in pop, do we want to clear the stencil buffer? otherwise it effectively just continues clipping.

So, right now, I'm clearing the stencil buffer at the beginning of beginClip() but not when exiting the clip context in pop().
clearing the buffer while exiting makes sense.
I can add gl.clear(gl.STENCIL_BUFFER_BIT) in the pop() function after restoring the previous stencil test state to ensure a clean slate for future operations.
Would you recommend this, or maybe there are cases where preserving the stencil buffer contents across pop operations would be beneficial?

davepagurek

Some thoughts on the overall logic

Just looking again at the if statements in the drawingContext.enable override: Is the if statement in here trying to detect if the user is manually calling drawingContext.disable vs if p5 itself is doing it?

One idea to try to simplify this: Maybe we should take those prevEnable and prevDisable variable we have, and store it on the renderer with names like internalEnable and internalDisable. Calling those just enable/disable; and then we're free to make our overrides a lot simpler, just:

this.drawingContext.enable = (key) => {
  if (key === this.drawingContext.STENCIL_TEST) {
    this._userEnabledStencil = true
  }
  return prevEnable.call(key)

Then:

wherever we internally call GL.enable(GL.STENCIL_TEST), we'd call internalEnable(GL.STENCIL_TEST) instead so that it keeps _userEnabledStencil the same
in all the places where we currently just disable stencil testing, we'd only actually disable it if _userEnabledStencil is false too, and otherwise leave it on.

This way we also don't need to record what the value is right before we enable stencil testing, since we don't really need to know what it was right before if we've recorded what the user has set it to.

I think that would mean we don't need the isStencilTestOn check as well, and might simplify some of our logic. Do you think that would work?

Clearing the buffer on `pop`

Thinking a little more about the use case here, clearing the stencil buffer in pop would only make a difference if the user already manually enabled the stencil buffer, and additionally is using p5 clipping. I think that's actually not a combination we need to worry about, since the stencil buffer does clipping, if you're turning it on manually, the user won't be using p5's clipping. So maybe we take the simpler approach and don't do anything additional here.

Testing

Sorry for all the back-and-forth on this, it turns out the logic is complicated haha. That's maybe a good sign that we should make some unit tests involving manual calls to enable/disable the stencil test?

davepagurek · 2025-03-01T19:25:26Z

src/webgl/p5.RendererGL.js

@@ -1750,7 +1809,9 @@ class RendererGL extends Renderer {
        this.GL.enable(this.GL.STENCIL_TEST);
        this._stencilTestOn = true;
      } else {
-        this.GL.disable(this.GL.STENCIL_TEST);
+        if (!this.isStencilTestOn) {


Should this only disable if the user has not manually called disable? If so, maybe we need to be using the _userEnabledStencil for the if condition?

Vaivaswat2244 · 2025-03-02T12:40:43Z

Some thoughts on the overall logic

Just looking again at the if statements in the drawingContext.enable override: Is the if statement in here trying to detect if the user is manually calling drawingContext.disable vs if p5 itself is doing it?

One idea to try to simplify this: Maybe we should take those prevEnable and prevDisable variable we have, and store it on the renderer with names like internalEnable and internalDisable. Calling those just enable/disable; and then we're free to make our overrides a lot simpler, just:
this.drawingContext.enable = (key) => {
  if (key === this.drawingContext.STENCIL_TEST) {
    this._userEnabledStencil = true
  }
  return prevEnable.call(key)
Then:

wherever we internally call GL.enable(GL.STENCIL_TEST), we'd call internalEnable(GL.STENCIL_TEST) instead so that it keeps _userEnabledStencil the same

in all the places where we currently just disable stencil testing, we'd only actually disable it if _userEnabledStencil is false too, and otherwise leave it on.

This way we also don't need to record what the value is right before we enable stencil testing, since we don't really need to know what it was right before if we've recorded what the user has set it to.

I think that would mean we don't need the isStencilTestOn check as well, and might simplify some of our logic. Do you think that would work?

Clearing the buffer on pop

Thinking a little more about the use case here, clearing the stencil buffer in pop would only make a difference if the user already manually enabled the stencil buffer, and additionally is using p5 clipping. I think that's actually not a combination we need to worry about, since the stencil buffer does clipping, if you're turning it on manually, the user won't be using p5's clipping. So maybe we take the simpler approach and don't do anything additional here.

Testing

Sorry for all the back-and-forth on this, it turns out the logic is complicated haha. That's maybe a good sign that we should make some unit tests involving manual calls to enable/disable the stencil test?

Sure, got it.
I have made the changes required. Tests are not included rn, once you confirm these I can write tests involving manual calls to enable/disable the stencil test.

davepagurek

The updates are looking good! I think the general approach looks almost complete. There are a few cases being handled that I think we may not need to handle any more and can be omitted. But in general this looks almost done, feel free to start adding tests!

davepagurek · 2025-03-02T15:01:57Z

src/webgl/p5.RendererGL.js

@@ -1387,7 +1423,7 @@ class RendererGL extends Renderer {
    this.drawTarget()._isClipApplied = true;

    const gl = this.GL;
-    gl.clearStencil(0);
+    gl.clearStencil(0); 


I think there's a trailing space at the end here that we can take out.

davepagurek · 2025-03-02T15:04:04Z

src/webgl/p5.RendererGL.js

+      if (key === this.drawingContext.STENCIL_TEST) {
+        // When pop() disables the stencil test after clip(),
+        // preserve the user's stencil test setting
+        if (this._clipDepth === this._pushPopDepth) {


Do we need this condition here, since we're calling _internalDisable in pop? I think maybe this is only called now when users manually call disable, so we can set this._userEnabledStencil = true. The same may be true in enable as well if we update the spots where we turn on stencil testing to use _internalEnable

Right! I'll do this for enable as well.

davepagurek · 2025-03-02T15:04:16Z

src/webgl/p5.RendererGL.js

@@ -427,6 +427,39 @@ class RendererGL extends Renderer {
    this.drawShapeCount = 1;

    this.scratchMat3 = new Matrix(3);
+
+    this._userEnabledStencil = false;
+    this.isStencilTestOn = false;


Do we use this any more?

Right, we don't.

Vaivaswat2244 · 2025-03-02T21:58:51Z

So, I have added the tests for stencil testing which I thought were appropriate. Let me know to add or remove some cases. Also replaced the internalEnable for enable in the file.

Updated Stencil Test Case to prevent it from disable after every draw…

b42d2a1

… loop

davepagurek reviewed Feb 27, 2025

View reviewed changes

Vaivaswat2244 added 2 commits March 1, 2025 01:51

updated beginClip and pop

2e88c82

fixed breaking the tests

38abe63

davepagurek reviewed Mar 1, 2025

View reviewed changes

Added internal functions and some logic changes

bc4b5f8

davepagurek reviewed Mar 2, 2025

View reviewed changes

Vaivaswat2244 and others added 3 commits March 3, 2025 03:00

Minor changes and added tests for the stencil tests

ed26828

Merge branch 'dev-2.0' into stencil_test

f300db5

fixed the linting which happened because of merging conflicts mb

441468e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated Stencil Test Case to prevent it from disable after every draw #7578

Updated Stencil Test Case to prevent it from disable after every draw #7578

Vaivaswat2244 commented Feb 26, 2025

davepagurek Feb 27, 2025

davepagurek commented Feb 27, 2025

davepagurek commented Feb 27, 2025

Vaivaswat2244 commented Feb 28, 2025

davepagurek left a comment

davepagurek Mar 1, 2025

Vaivaswat2244 commented Mar 2, 2025

Some thoughts on the overall logic

Clearing the buffer on `pop`

Testing

davepagurek left a comment

davepagurek Mar 2, 2025

Vaivaswat2244 Mar 2, 2025

davepagurek Mar 2, 2025

Vaivaswat2244 Mar 2, 2025

davepagurek Mar 2, 2025

Vaivaswat2244 Mar 2, 2025

Vaivaswat2244 commented Mar 2, 2025

Updated Stencil Test Case to prevent it from disable after every draw #7578

Are you sure you want to change the base?

Updated Stencil Test Case to prevent it from disable after every draw #7578

Conversation

Vaivaswat2244 commented Feb 26, 2025

PR Checklist

Choose a reason for hiding this comment

davepagurek commented Feb 27, 2025

davepagurek commented Feb 27, 2025

Vaivaswat2244 commented Feb 28, 2025

davepagurek left a comment

Choose a reason for hiding this comment

Some thoughts on the overall logic

Clearing the buffer on pop

Testing

Choose a reason for hiding this comment

Vaivaswat2244 commented Mar 2, 2025

Some thoughts on the overall logic

Clearing the buffer on pop

Testing

davepagurek left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vaivaswat2244 commented Mar 2, 2025

Clearing the buffer on `pop`

Clearing the buffer on `pop`