Improved: locking the stencil buffer now also locks the test and operations

[ProPuke]

The way WebGLState locking works isn't currently useful for making custom use of the stencil buffer as it only locks whether stencil operations are active and which bits are allowed to be read from or written to, but not what is actually being tested, and what operations are to occur.

This means that if you set up a stencil operation and lock the buffer, objects with custom material stencil settings can still modify the tests and operations mid-render and affect all later rendered objects, despite the buffer being locked.

Note that the lock did originally lock just the bits, and not even if stencil operations were active. This was previously amended to include activation state by #17136, who did also mention that func and op should likely also be locked, but decided not to at the time unless it was later mentioned.

This PR now locks those as well.

[mrdoob]

@gkjohnson what do you think?

[mrdoob]

Seems like this PR breaks webgl_postprocessing_advanced:
https://github.com/mrdoob/three.js/pull/21052/checks?check_run_id=1678908046

[ProPuke]

I'm not seeing any probs with that test on my end, unless it's perhaps for a single frame at the beginning?

I would be a bit sus of that test, as the film shader it uses applies random noise to the image (based on the shader time). Is this guaranteed to be deterministic in tests?

[gkjohnson]

@gkjohnson what do you think?

It looks like EffectComposer + MaskPass rely on the current behavior in dev so I'm not sure if this is necessarily such a simple change. It doesn't look like there's a lot of consistency between the the locking mechanisms in the different buffer states (the DepthBuffer locks the depth mask but not depth func) so I'm not sure if there's a rationale behind which fields should get locked and which shouldn't other than it's been found to be useful to do it in the current way.

@ProPuke do you mind explaining your use case for locking the other stencil fields?

I would be a bit sus of that test, as the film shader it uses applies random noise to the image (based on the shader time). Is this guaranteed to be deterministic in tests?

The screenshot comparison tests should be designed to be deterministic otherwise all of the other PRs would be failing, as well.

[ProPuke]

@gkjohnson It doesn't look like there's a lot of consistency between the the locking mechanisms in the different buffer states

So the colour/depth buffers and stencil buffers are used a little differently.

For colour and depth buffers the lock simply controls whether it can be written to. This makes sense as what they represent and how they're used is consistent.

For stencil buffers the lock controls whether they can be written to AND if materials are currently writing to it. This allows you to set up an operation and lock it as active (which was the intent with #17136). When locked, materials are currently blocked from toggling it on or off, or modifing the bitmask that tests can read/write. However, currently if a single material has custom stencil options set, it will change the test and operation being performed, and not change it back after, leaving the settings modified and ruining later tests (so defeating the point of the initial lock).

In the case of colour and depth buffers what they represent and how they're used is consistent, so just locking it makes sense. But with stencil buffers they're just custom bits, and the importance is more in how they're tested and what operation is being performed on them, so it makes sense to lock everything.

If the intent of locking the stencil buffer is just to stop the buffer from being modified, then the lock should only be for the mask, and not the activation step, but if the intent is to lock an operation in progress (which I'd argue is usually the common case with stencil operations) then the func and op should also be locked. Currently we're in the middle with a bit of both, which I don't think serves either circumstance.

@gkjohnson The screenshot comparison tests should be designed to be deterministic otherwise all of the other PRs would be failing, as well.

Maybe I'm really unlucky, hah. I would be curious to see what the difference is though, as it does look fine here. It is the only shader to make use rand(... time ...) which does make me suspicious. Hopefully though, time is increased in a deterministic fashion and not just based on render time.

@gkjohnson @ProPuke do you mind explaining your use case for locking the other stencil fields?

I'm currently redoing my portal rendering PR #12033 against the latest three.js. I see three.js has matured a little with regard to recursive rendering and I can hopefully add in support with fewer fundamental changes this time (so hopefully it's less of a mess for you guys to look over). I'm looking to trickle in some of the required changes as smaller PRs first of all to make things smoother, this being one of them.

[gkjohnson]

I'm currently redoing my portal rendering PR #12033 against the latest three.js. I see three.js has matured a little with regard to recursive rendering and I can hopefully add in support with fewer fundamental changes this time (so hopefully it's less of a mess for you guys to look over). I'm looking to trickle in some of the required changes as smaller PRs first of all to make things smoother, this being one of them.

Can you elaborate on how exactly you'll be using this? I imagine the portal will set and lock the stencil in something like onBeforeRender, render the scene again, and then unlock it, is that right? I just want to make sure I understand.

I'm not against adding this change it should just be made sure that it doesn't break anything. The webgl_postprocessing_advanced example uses MaskPass which relies on stencil buffer locking behavior. EffectComposer sets the stencil parameters directly in that case which won't behave the same after this PR. It's not clear to me if other postprocessing passes rely on the stencil behavior maskActive in EffectComposer but that's likely the source of the test failure.

In the future it would be nice to be able to specify material override parameters on the renderer so users don't have to use the undocumented renderer.state buffers but that's another issue.

[ProPuke]

Can you elaborate on how exactly you'll be using this? I imagine the portal will set and lock the stencil in something like onBeforeRender, render the scene again, and then unlock it, is that right? I just want to make sure I understand.

It is a bit more complicated and does modify the main flow I'm afraid, but only if there are portals present in the scene.

I've added an additional function to specify how many bits are reserved for custom use and how many for portal rendering. By default all 8 bits are assigned to custom use, but users can opt in to allocate some for portal use, which will allow portals to work (more bits mean support for more and deeper portals)

Portal rendering occurs between opaque and transparent object rendering, and recursively calls render() again for each supported depth. The first pass looks like this:

render all opaque objects (the stencil buffer is not locked here, so custom stencil operations can be performed)

for each portal in the scene:
    render the portal geometry to the stencil buffer using a unique stencil id for each

for each portal in the scene:
    lock the stencil buffer to only write within this stencil region
    clear the depth for just the portal region
    recursively call render() with a custom camera config, rendering the contents within this portal region
    "seal" the portal by writing flat depth values over the portal surface to stop rendering within the area
    unlock the stencil buffer

render all transparent objects (the stencil buffer is again unlocked here in this first pass)

Inside the deeper calls to render() the stencil buffer is instead locked (although it is briefly unlocked if rendering deeper portal geometry before being restored to previous state and relocked again).

This allows custom stencil effects to be used alongside portals, but only on the first pass. Further depths within the portals do not allow custom stencil effects on top (as this would nullify the test that limits rendering to the stenciled portal regions). Thus we need to support rendering of the same content multiple times - initially allowing stencil operations to be changed, and later fully disabling such things.

You can see the end result of this in this old demo here: https://www.31i73.com/three.js/examples/webgl_geometry_portals.html
Zooming out with the mousewheel makes the effect more apparent. The mirrors use the same rendering technique. They are just portals that project to themselves, with an axis mirrored.

[Mugen87]

Sorry, but this PR can't be merged in this form. The visuals of webgl_postprocessing_advanced are broken and it's possible that applications which rely on the current implementation break in the same way.

[ProPuke]

That example now builds correctly. I've fixed the EffectComposer handling now (the change was just ProPuke@5965c11). Apologies there.

Relying on old implementations may be an issue, though I wouldn't expect this to be common.

I'd commonly assume rendering techniques that lock the stencil buffer for a particular task to have set it up and to be unlikely to rely on materials being able to modify its configuration mid-pass. Particularly as that modification lingers for later rendering objects without restoring state after.

I'd still consider this modified behaviour an improvement for future techniques (I need this capability, for instance, to lock out stencil modifications to protect against custom stencil effects breaking rendering within portals).

If that's still too concerning, we could look at something like a masking parameter when locking, with the default mask matching existing behaviour (locking just the stencil activation, but not the operation on it). Although I personally think the amended behaviour makes more sense, and the existing locking behaviour to be more of an odd smell.

If there's definitely no interest, then it is what it is.