Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow shaders with disabled post processing pipeline #14338

Merged
merged 2 commits into from Feb 15, 2024

Conversation

lhofhansl
Copy link
Contributor

@lhofhansl lhofhansl commented Feb 2, 2024

This allows disabling the post-processing pipeline while leaving Shader enabled.

So "geometric" shader feature - shadows, waves, waving plants, etc - remain enabled, and all post-processing that relies to intermediary textures are disabled.

See also #14285. Currently post processing silently disables FSAA.
FSAA is the only (performance) option we have to avoid geometry aliasing (not texture aliasing).

In general it seems reasonable to allow shaders but disable post processing.

To do

This PR is Ready for Review.

How to test

Enable shaders, and disable post processing, make sure all non-post-processing shader features still work: shadows, waves, waving plants, smooth day-night change, etc., and that everything still renders OK.

Enable FSAA, try some different FSAA values.
Notice the absence of Moire effect caused by geometry aliasing. (see images on #14285).
(Remember changing FSAA requires a restart of MT)

@lhofhansl lhofhansl added @ Client / Audiovisuals WIP The PR is still being worked on by its author and not ready yet. Discussion Issues meant for discussion of one or more proposals labels Feb 2, 2024
@lhofhansl lhofhansl changed the title Allow disabling of the post processing pipeline Allow disabling the post processing pipeline Feb 2, 2024
@lhofhansl
Copy link
Contributor Author

lhofhansl commented Feb 2, 2024

@HybridDog Not sure whether you have any insights here.

I do not know enough about Irrlicht + Shaders. There should be a way to pass a muti-sample texture to the postprocessing pipeline, and then resolve it, so that at least the initial scene can benefit from fast FSAA (any post processed changes would not). But I do not understand how to do that with Irrlicht.

See: https://therealmjp.github.io/posts/msaa-overview/ and https://stackoverflow.com/questions/42878216/opengl-how-to-draw-to-a-multisample-framebuffer-and-then-use-the-result-as-a-n

For me MSAA works with NVidia and Intel drivers, and is quite a performance improvement over SSAA. (see link as to why... Essentially the fragment shader is not called for all fragments, only the interesting ones)

@lhofhansl
Copy link
Contributor Author

lhofhansl commented Feb 3, 2024

Maybe it was premature to move everything to the post processing stage.
@x2048 is gone now, so I can't ask him.

And actually it is not that unreasonable to disable post processing. I guess one could derive it by checking that all of tonemapping, saturation, dithering, bloom filters, and volumetric lighting are, but making it explicit is nice.
Maybe it's not as WIP as I thought.

Some more info on Irrlicht and MS textures here: https://irrlicht.sourceforge.io/forum/viewtopic.php?t=51397

@lhofhansl lhofhansl removed WIP The PR is still being worked on by its author and not ready yet. Discussion Issues meant for discussion of one or more proposals labels Feb 3, 2024
@lhofhansl
Copy link
Contributor Author

@devshgraphicsprogramming are you still around here?

You mention in #6976 that MSAA is arcane. I agree, yet, for the kind of geometry aliasing I am concerned with here, it is very effective (unlike FXAA and other post-processing AA techniques) and much faster than SSAA.
Curious if you have any insights into Irrlicht, MSAA, and post processing.

@lhofhansl lhofhansl changed the title Allow disabling the post processing pipeline Allow shaders with disabled the post processing pipeline Feb 5, 2024
@lhofhansl lhofhansl changed the title Allow shaders with disabled the post processing pipeline Allow shaders with disable post processing pipeline Feb 5, 2024
@lhofhansl lhofhansl requested a review from sfan5 February 5, 2024 19:07
@lhofhansl lhofhansl changed the title Allow shaders with disable post processing pipeline Allow shaders with disabled post processing pipeline Feb 5, 2024
@lhofhansl lhofhansl added the Trivial The change is a trivial bug fix, documentation or maintenance change, as per the Git Guidelines label Feb 6, 2024
@lhofhansl
Copy link
Contributor Author

And actually it's pretty trivial as it really just allows disabling post-processing, with no other changes.

@HybridDog
Copy link
Contributor

Not sure whether you have any insights here.

I also don't know much about Irrlicht and FSAA, but I can nonetheless try to give insights.

If multisampling should work, a so-called multisample resolve must happen.

I assume that with EGL and GLX, GL_TEXTURE_2D_MULTISAMPLE textures are automatically created because Irrlicht enables anti aliasing in the EGL or GLX attribute array configuration.
With post processing, however, the output of the fragment shader of the first stage is not some texture from EGL or GLX but a texture which Irrlicht creates using the configuration in the Irrlicht SMaterial object, which is passed from Minetest.
I could not find an explicit creation of GL_TEXTURE_2D_MULTISAMPLE textures in Irrlicht's code but a lot of GL_TEXTURE_2D texture creations. GL_TEXTURE_2D_MULTISAMPLE textures may only be implicitly created by EGL or GLX.

I think the multisample resolve needs to be implemented in Irrlicht (probably in COpenGLCoreRenderTarget.h), i.e. if FSAA is enabled in Minetest, the first stage should be rendered to a GL_TEXTURE_2D_MULTISAMPLE texture and this texture should then be converted (with a blit operation) to a GL_TEXTURE_2D texture for the next stages. Furthermore, as a cleanup, the EGL and GLX attribute array configuration should not contain enabled anti aliasing if post processing is enabled in Minetest.

Additional notes:

If I understand it correctly, in the OpenGL and OpenGL ES code, GL_MULTISAMPLE_ARB, respectively GL_MULTISAMPLE, is only enabled if anti aliasing is enabled both globally (AntiAlias) and in the material (material.AntiAliasing). The material may correspond to the output texture where the fragment shader renders to. If I change the code so that it does glEnable(GL_MULTISAMPLE_ARB) even if antialiasing is disabled, there is still no visible antialiasing. I assume there is no anti aliasing because the multisample resolve is missing.
https://github.com/minetest/irrlicht/blob/f1504093d148c89b9560a01a6bbeecd8aaf52ca3/source/Irrlicht/COGLESDriver.cpp#L1808-L1811
https://github.com/minetest/irrlicht/blob/f1504093d148c89b9560a01a6bbeecd8aaf52ca3/source/Irrlicht/COpenGLDriver.cpp#L2569-L2571

As far as I know, EGL and GLX provide the interface between OpenGL and the window. If MSAA is enabled, Irrlicht sets something in the "attribute array for the draw buffer", such as EGL_SAMLES with EGL:
https://github.com/minetest/irrlicht/blob/f1504093d148c89b9560a01a6bbeecd8aaf52ca3/source/Irrlicht/CGLXManager.cpp#L72
https://github.com/minetest/irrlicht/blob/f1504093d148c89b9560a01a6bbeecd8aaf52ca3/source/Irrlicht/CEGLManager.cpp#L204

Antialiasing in SDL-related code:
https://github.com/minetest/irrlicht/blob/f1504093d148c89b9560a01a6bbeecd8aaf52ca3/source/Irrlicht/CIrrDeviceSDL.cpp#L389-L398
https://github.com/minetest/irrlicht/blob/f1504093d148c89b9560a01a6bbeecd8aaf52ca3/source/Irrlicht/CIrrDeviceSDL.cpp#L460

OpenGL differentiates between the GL_TEXTURE_2D and GL_TEXTURE_2D_MULTISAMPLE texture types.
https://github.com/minetest/irrlicht/blob/f1504093d148c89b9560a01a6bbeecd8aaf52ca3/include/mt_opengl.h#L1609
https://github.com/minetest/irrlicht/blob/f1504093d148c89b9560a01a6bbeecd8aaf52ca3/include/mt_opengl.h#L2291

@lhofhansl
Copy link
Contributor Author

Thanks @HybridDog

I also have searched through the Irrlicht code to see how/where it does handles XXX_MULTISAMPLE. Do you have any leisure to dig a bit deeper into this. I'll also continue to dig.

if FSAA is enabled in Minetest, the first stage should be rendered to a GL_TEXTURE_2D_MULTISAMPLE texture and this texture should then be converted (with a blit operation) to a GL_TEXTURE_2D texture for the next stages.

Furthermore, as a cleanup, the EGL and GLX attribute array configuration should not contain enabled anti aliasing if post processing is enabled in Minetest.

Aren't these two at odds? Or do you mean if PP is enabled we should only enable it through the materials?

@lhofhansl
Copy link
Contributor Author

lhofhansl commented Feb 7, 2024

Storing some notes here for later:
The intermediary textures are created here: https://github.com/minetest/minetest/blob/master/src/client/render/pipeline.cpp#L117
Note the calls to createImage and addTexture. There's no option to pass a texturetype.

https://github.com/minetest/irrlicht/blob/master/include/ITexture.h#L159
There's no ETT_2D_MS (or something)

And ETT_2D is hardcoded here: https://github.com/minetest/irrlicht/blob/master/source/Irrlicht/COpenGLDriver.cpp#L1920

(I'm about to run out of time on this. It seems Irrlicht currently just does not support multisampling textures.)

@lhofhansl
Copy link
Contributor Author

lhofhansl commented Feb 7, 2024

In the extreme we can have an alternate shader that does tonemapping, saturation, and dithering - none of these actually need to be the PP pipeline, and only use the PP pipeline for bloom and volumetric lighting.
Then the PP pipeline can be turned off, and we can still get fast AA via MSAA.

Or... Someone figures out how to add MS texture support to Irrlicht proper.

Or... Do proper geometry LOD at a distance (300+), in that case pixel based (post processing) anti aliasing would work.

@rubenwardy
Copy link
Member

rubenwardy commented Feb 7, 2024

Tonemapping, saturation, and dithering are all optional effects, I think it's best to avoid duplicating implementations and keep them in post-processing. Tonemapping (filmic and saturation) does seem to be best implemented as a post-processing effect given that it maps the HDR output to colors to be rendered on the screen - we don't want to duplicate this for all types of rendered thing

Surely it's possible to have correct antialiasing with post processing enabled?

@lhofhansl
Copy link
Contributor Author

lhofhansl commented Feb 7, 2024

Surely it's possible to have correct antialiasing with post processing enabled?

Yep. Just needs some changes in Irrlicht - which I find somewhat obscure.

Allowing to optionally disable PP is not unreasonable in general, though.

@HybridDog
Copy link
Contributor

Aren't these two at odds? Or do you mean if PP is enabled we should only enable it through the materials?

Yes, if post processing is enabled, MSAA should be disabled in the EGL and GLX attribute array configuration. I think with post processing, in each stage except the first one we render a single square (two triangles) and there are no edges which need to be anti aliased. We need multisampling only in the first stage, where the rendering from the 3D world to a 2D image happens.
If post processing is disabled, the first stage is the last stage, and since the output texture of the last stage is somehow created by EGL or GLX, we need to enable anti aliasing in the EGL or GLX attribute array, which is what Irrlicht does in CGLXManager.cpp and CEGLManager.cpp, if I understand the code correctly.

It seems Irrlicht currently just does not support multisampling textures.

I also think so since I don't find anything useful when I ripgrep for TEXTURE_2D_MULTISAMPLE but I can find a lot for TEXTURE_2D. I assume multisampling textures are implicitly provided by GLX and EGL.

@grorp
Copy link
Member

grorp commented Feb 7, 2024

Postprocessing can make a small performance difference, e.g. see these "drawtime" values on my laptop:

without shaders: 8-9 ms
with shaders: 9-10 ms
with shaders + postprocessing (without any postprocessing effects): 13-14 ms

So I wonder why we'd even make this a manual setting. Can't we just automatically disable postprocessing if no postprocessing effects are enabled? Is there any reason for keeping it enabled in this case?

@lhofhansl
Copy link
Contributor Author

Can't we just automatically disable postprocessing if no postprocessing effects are enabled? Is there any reason for keeping it enabled in this case?

I thought about this too... The one part that is not manually enabled is saturation. So I thought instead of making that configurable it is better to just be able to disable PP wholesale.

@lhofhansl
Copy link
Contributor Author

Tonemapping (filmic and saturation) does seem to be best implemented as a post-processing effect given that it maps the HDR output to colors to be rendered on the screen

Right.
What I meant to say is that for all those we do not need a pipeline of step with a texture passed between them.
The texture-pipeline is only needed for bloom, volumetric light, FXAA, SSAA, undersampling.

@lhofhansl
Copy link
Contributor Author

lhofhansl commented Feb 7, 2024

With this one. I want everybody to try and see the staggering difference.

It may sounds like a trifle, but with slightly larger viewing_range (doesn't have to be 1000), as you move around the noise that comes from geometry aliasing is really grating and looks quite unprofessional.

You can compare FSAA with 4 samples, to FXAA. (SSAA is potentially better than FSAA but is far, far slower.)
To try FSAA you have to be able to turn off the PP pipeline. You could turn off shaders altogether, but then you pretty bad stuttering due to handling day-night difference without shaders.

@lhofhansl
Copy link
Contributor Author

lhofhansl commented Feb 9, 2024

BTW. FSAA does not work on Windows, even with the PP pipeline off. Seems that Irrlicht has not implemented that or we're missing something in the setup.

Update: And even on Linux, when Irrlicht is configured to use SDL2, FSAA does not work even with PP off.

@lhofhansl
Copy link
Contributor Author

Independently of all the FSAA discussion here. Is it reasonable to allow disabling the PP pipeline so that clients can control performance?

We could automatically disable the PP pipeline if all of:

  • bloom, volumetric lighting
  • saturation (would need to new config flag to allow disabling)
  • exposure compensation
  • upscaling
  • SSAA, FXAA
  • debanding
  • tonemapping

are disabled, but I think an overall switch is better, and easier to understand.

@sfan5
Copy link
Member

sfan5 commented Feb 10, 2024

@lhofhansl I can check that this works but not offer advice on graphical stuff. Is that what you wanted me to do?

@lhofhansl
Copy link
Contributor Author

@sfan5 Yeah, that (a) it works, and (b) it's reasonable to allow disabling the PP pipeline for performance reasons.

@sfan5
Copy link
Member

sfan5 commented Feb 12, 2024

Can confirm FSAA doing something when enable_post_processing=false.

thought: shouldn't we disable this on Android by default?

@sfan5 sfan5 added One approval ✅ ◻️ and removed Trivial The change is a trivial bug fix, documentation or maintenance change, as per the Git Guidelines labels Feb 12, 2024
@grorp
Copy link
Member

grorp commented Feb 12, 2024

thought: shouldn't we disable this on Android by default?

+1
Maybe this would even "fix" #13519? I have no affected device, so I can't know.

@lhofhansl
Copy link
Contributor Author

Should I default this to fault on Android in this PR (maybe different commit), or separate discussion on separate PR?

@sfan5
Copy link
Member

sfan5 commented Feb 13, 2024

This PR imo.

@lhofhansl
Copy link
Contributor Author

@sfan5 OK, done.

src/defaultsettings.cpp Outdated Show resolved Hide resolved
@lhofhansl
Copy link
Contributor Author

I assume this is still good to merge.

@lhofhansl lhofhansl merged commit c81e0b7 into minetest:master Feb 15, 2024
13 checks passed
@sfan5 sfan5 mentioned this pull request Feb 23, 2024
grorp pushed a commit to grorp/minetest that referenced this pull request Apr 15, 2024
- Allow disabling of the post processing pipeline while leaving shaders enabled
- Also disable post processing on Android by default
grorp pushed a commit to grorp/minetest that referenced this pull request Apr 15, 2024
- Allow disabling of the post processing pipeline while leaving shaders enabled
- Also disable post processing on Android by default
grorp pushed a commit to grorp/minetest that referenced this pull request Apr 15, 2024
- Allow disabling of the post processing pipeline while leaving shaders enabled
- Also disable post processing on Android by default
grorp pushed a commit to grorp/minetest that referenced this pull request Apr 15, 2024
- Allow disabling of the post processing pipeline while leaving shaders enabled
- Also disable post processing on Android by default
grorp pushed a commit to grorp/minetest that referenced this pull request Apr 21, 2024
- Allow disabling of the post processing pipeline while leaving shaders enabled
- Also disable post processing on Android by default
grorp pushed a commit that referenced this pull request May 6, 2024
- Allow disabling of the post processing pipeline while leaving shaders enabled
- Also disable post processing on Android by default
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants