Fix incorrect enabling of post process in OpenGL

[BastiaanOlij]

A recent change (commit 9000a9d) resulted in post processing to always happen in a separate step even when unwanted. This resulted in a 20-30% performance drop on Quest 3 resulting in proper frame rates being unobtainable. It also results in a number of errors as some of the code was not expecting the separate post processing step to happen.

An easy mistake, a call to ensure_internal_buffers was performed to easy in our check if glow was enabled.

However there was also a flaw in the implementation as once enabled, you couldn't disable it.

So if a level allowed for glow (or any other post process) to be enabled, and a later level would turn it off, or if this had been turned into a user setting because we can enable it on desktop, it would always be running the separated post process logic.

I've thus done a little extra work to clean this up and make it work consistently.

[dsnopek]

I tested this with a couple of projects on the Quest 3.

Unfortunately, I wasn't really able to verify the performance improvement in any of the test projects I tried, using the methods I was using to measure. This could certainly be a weakness in my projects or the way I was measuring though?

I can say that it does seem to work fine, including enabling/disabling glow.

[BastiaanOlij]

@dsnopek it's fairly easy to test with XR Tools, just open it up in Godot 4.3 dev1 (yeah our first dev release) and it runs at a nice 90fps
Then open the same project in 4.3 beta2 and the performance drops to around 75-80 fps. Enabling/disabling glow doesn't matter (much) here because the post processing pass is introduced either way.

With renderdoc you can quickly see the difference, this is dev1, 3 passes, 1st is the 2D UI on our pedestal, 2nd is the wrist UI and 3rd is our actual 3D output:

Then in beta1 we see those same 3 passes plus our post processing pass:

That post processing pass results in all our tiles having to be rendered and flushed out to texture memory in pass 3, then in pass 4 being loaded back into tile memory to apply post processing.

Obviously seeing we don't have proper timing info in OpenGL you need to use a test case that is sufficiently big to start noticing a drop in FPS, which is why we didn't catch this earlier because most of our test scenes are simple enough they run on 90fps, especially with foveated rendering enabled.

Note btw that we're about to make the minimum version for XR Tools Godot 4.2 and will have foveated rendering enabled, so it will be harder to notice a regression like this unless you go to some of the more complex demos.

[BastiaanOlij]

Hmm, just realised the note I added isn't completely valid, it would probably still struggle because post processing requires the use of an intermediate buffer, and we can't apply foveated rendering to that buffer as it's only added to the destination buffer that we set up in the OpenXR module. This is a design limitation of our (current) rendering engine and why for Vulkan we're doing our own VRS implementation which can be applied to intermediate buffers.

[clayjohn]

The code here looks good. It solves the problem of post processing getting enabled even though glow/BCS are not enabled. However, if Glow or BCS are enabled and then disabled later, the post process will continue running because the post process FBO is not freed.

I prefer that we do not free the post process FBO/textures whenever post processing is disabled. But we should stop doing the post processing pass when post processing is disabled

[BastiaanOlij]

The code here looks good. It solves the problem of post processing getting enabled even though glow/BCS are not enabled. However, if Glow or BCS are enabled and then disabled later, the post process will continue running because the post process FBO is not freed.

I prefer that we do not free the post process FBO/textures whenever post processing is disabled. But we should stop doing the post processing pass when post processing is disabled

I agree this could use a rework, I remember when originally making the system (before we even separated post processing out) this had some real issues from us not providing any dependency management. On the Vulkan side things would nicely be cleaned up automatically.

It is important to realise that the texture aren't just used for post processing, they are also used when scaling, and they are used when certain extensions aren't available that keep logic in tile memory and allow us to never write stuff out to textures but write out straight to our render buffer.

We're also trying to not allocate these buffers unless they are really needed, especially in VR where we're rendering at high resolution with double layers for stereo. Maybe I'm being to conservative, but that is the reasoning behind it cleaning up after itself.

[akien-mga]

Thanks!