Implement material system #1105
Conversation
|
I get a lot of those warnings: Unvanquished/daemon/src/engine/renderer/gl_shader.h:478:37: warning: '&&' within '||' [-Wlogical-op-parentheses]
if ( _shader->UseMaterialSystem() && _global || !_shader->UseMaterialSystem() ) {
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~ ~~Unvanquished/daemon/src/engine/renderer/Material.cpp:1503:70: warning: '&&' within '||' [-Wlogical-op-parentheses]
if ( tr.hasSkybox && ( fromSort <= shaderSort_t::SS_ENVIRONMENT_FOG && toSort >= shaderSort_t::SS_ENVIRONMENT_FOG
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ |
|
"Material" is sometimes used as a synonym or generic term for (q3)shader. Maybe we could call these metamaterials? |
This shouldbe fixed now. |
Yeah, I thought the name might cause some confusion, so a rename might be in order. |
Unvanquished/daemon/src/engine/renderer/gl_shader.h:399:3: warning: field '_global' will be initialized after field '_components' [-Wreorder-ctor]
_global( global ),
^~~~~~~~~~~~~~~~~
_std430Alignment( std430Alignment )Unvanquished/daemon/src/engine/renderer/tr_backend.cpp:136:7: warning: logical not is only applied to the left hand side of this comparison [-Wlogical-not-parentheses]
if ( !bundle->numImages == 0 ) {
^ ~~Unvanquished/daemon/src/engine/renderer/TextureManager.cpp:177:1: warning: non-void function does not return a value in all control paths [-Wreturn-type]
}
^Unvanquished/daemon/src/engine/renderer/Material.cpp:1524:74: warning: '&&' within '||' [-Wlogical-op-parentheses]
if ( tr.hasSkybox && ( ( fromSort <= shaderSort_t::SS_ENVIRONMENT_FOG ) && ( toSort >= shaderSort_t::SS_ENVIRONMENT_FOG )
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~Unvanquished/daemon/src/engine/renderer/Material.cpp:1525:64: warning: '&&' within '||' [-Wlogical-op-parentheses]
|| ( fromSort <= shaderSort_t::SS_ENVIRONMENT_NOFOG ) && toSort >= ( shaderSort_t::SS_ENVIRONMENT_NOFOG ) ) ) {
~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~Unvanquished/daemon/src/engine/renderer/tr_backend.cpp:124:18: warning: expression result unused [-Wunused-value]
bundle->image[0];
~~~~~~~~~~~~~ ~^It's a cherry-picking as I get dozens and dozens of other warnings like unused variables, const type qualifer on return having no effect. They are likely harmless but produces a lot noise making harder to spot the warnings that can be problematic for real. |
|
More details (GCC) on what is probably the same of the first one of the previous comment (Clang): Unvanquished/daemon/src/engine/renderer/gl_shader.h: In constructor ‘GLUniform::GLUniform(GLShader*, const char*, const char*, GLuint, GLuint, bool, int, bool)’:
/home/illwieckz/dev/buildme-unvanquished/Unvanquished/daemon/src/engine/renderer/gl_shader.h:387:20: warning: ‘GLUniform::_global’ will be initialized after [-Wreorder]
387 | const bool _global = false; // This uniform won't go into materials SSBO if true
| ^~~~~~~
Unvanquished/daemon/src/engine/renderer/gl_shader.h:381:19: warning: ‘const int GLUniform::_components’ [-Wreorder]
381 | const int _components;
| ^~~~~~~~~~~
Unvanquished/daemon/src/engine/renderer/gl_shader.h:392:9: warning: when initialized here [-Wreorder]
392 | GLUniform( GLShader *shader, const char *name, const char* type, const GLuint std430Size, const GLuint std430Alignment,
| ^~~~ |
|
Also this: diff --git a/src/engine/renderer/TextureManager.h b/src/engine/renderer/TextureManager.h
index d31892774..d28c78cb3 100644
--- a/src/engine/renderer/TextureManager.h
+++ b/src/engine/renderer/TextureManager.h
@@ -36,6 +36,7 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
#ifndef TEXTURE_MANAGER_H
#define TEXTURE_MANAGER_H
+#include <queue>
#include <vector>
#include <unordered_set>
#include "GL/glew.h"It is caught by older compilers (GCC9, ICC…): Unvanquished/daemon/src/engine/renderer/TextureManager.h:86:8: error: ‘queue’ in namespace ‘std’ does not name a template type
86 | std::queue<Texture*> texQueue;
| ^~~~~
Unvanquished/daemon/src/engine/renderer/TextureManager.h:42:1: note: ‘std::queue’ is defined in header ‘<queue>’; did you forget to ‘#include <queue>’?
41 | #include "GL/glew.h"
+++ |+#include <queue>Unvanquished/daemon/src/engine/renderer/TextureManager.h(86): error: qualified name is not allowed
std::queue<Texture*> texQueue;
^
Unvanquished/daemon/src/engine/renderer/TextureManager.h(86): error #77: this declaration has no storage class or type specifier
std::queue<Texture*> texQueue;
^ |
|
With AMDGPU-PRO OGLP 23.40: With Mesa radeonsi 23.2.1: |
|
Yeah, I'll fix most of those later, when everything is more ready. I'm not worried about the ones that don't actually change how the code is executed right now. |
It looks like |
|
With both drivers case I get this though: |
|
Most or all of these warns are caught by CI anyway, so they shouldn't be a problem. |
I'm pretty sure it works that way will all extensions that we use. But the shader never declares the extension as required in this case. |
|
Anyway with
With OGLP the computer hangs for several minute, then later when the computer becomes usable again the game window blinks forever with some textures painted in the whole window (but behind 2D elements), doing a screenshot just captures black. Edit: dmesg log: https://pastebin.com/pPpVWza8 |
|
Some graphics settings are currently broken on this branch, so might be that. |
|
I get the same with |
|
In fact it probably happened on |
|
With
The black lines around the 23 number are blinking (Edit: it's likely part of the 23 texture, probably missing transparency like the blue shield textures). |
Ah, I need to add that to Free(). It's just accessing memory for drawSurfs that isn't there anymore probably. |
I think it's because of polygon offset. I still need to add it as part of the material state. |
That looks about right though (ignore the incorrect textures in some places for now, need to fix a thing or two for that). Also, does the lighting look right to you? I had to invert the sign on u_InverseLightFactor, I'm not sure why it's not working correctly without that. |
VReaperV
force-pushed
the
material
branch
2 times, most recently
from
b08d9a5
to
32093f4
Compare
|
I got this with llvmpipe, maybe that explains why I get black textures with radeonsi? |
|
I have no idea how those line happened to be there: 161: void main()
162: {
163: sampler2D u_ColorMap = sampler2D(u_ColorMap_initial); // <---
164: vec4 color = texture2D(u_ColorMap, var_TexCoords); 174: #if defined(USE_DEPTH_FADE) || defined(USE_VERTEX_SPRITE)
175: sampler2D u_DepthMap = sampler2D(u_DepthMap_initial); // <---
176: float depth = texture2D(u_DepthMap, gl_FragCoord.xy / r_FBufSize).x;
177: float fadeDepth = 0.5 * var_FadeDepth.x / var_FadeDepth.y + 0.5; |
That's because it's lacking the bindless textures extension (it's currently required because there's too many textures to bind them separately, without packing a lot of them into a few larger textures). I'll need to add a check to not load the material shaders if not all of the required extensions are supported. |
That part is auto-generated in ShaderPostProcess(). Initially I just had it use |
|
I still need to fix a small part of the material rendering code to make textures resident, so that might be why you're seeing black textures there. |
src/engine/renderer/gl_shader.cpp
Outdated
| @@ -1139,6 +1184,120 @@ void GLShaderManager::CompileAndLinkGPUShaderProgram( GLShader *shader, shaderPr | |||
| LinkProgram( program->program ); | |||
| } | |||
|
|
|||
| // This will generate all the extra code for material system shaders | |||
| std::string GLShaderManager::ShaderPostProcess( GLShader *shader, const std::string& shaderText, const bool isVertexShader ) { | |||
There was a problem hiding this comment.
Why not writing a real .glsl file to be used to prefix the GLSL shaders with fragmentInline/vertexInline mechanism, using ifdef to select various code path and set defines based on the tests you need? Edit: This is basically an conditional “include” mechanism.
A good example of different shaders using a single source but different “inline” parts is generic and generic2D, that both uses generic_fp.glsl while using different compileMacros and vertexInlines. See GLShader_generic2D and GLShader_generic in gl_shader.cpp.
There was a problem hiding this comment.
Why not writing a real
.glslfile to be used to prefix the GLSL shaders withfragmentInline/vertexInlinemechanism, usingifdefto select various code path and set defines based on the tests you need? Edit: This is basically an “include” mechanism.
Because I don't want to add a macro for almost every uniform. It would also mean moving all those uniform definitions to a new file for each shader, which is bad. It would also mean having to align memory for every shader manually, and with any change to uniforms too.
Also, having 2 different inlines isn't gonna work here.
There was a problem hiding this comment.
Why not writing a real
.glslfile to be used to prefix the GLSL shaders withfragmentInline/vertexInlinemechanism, usingifdefto select various code path and set defines based on the tests you need? Edit: This is basically an conditional “include” mechanism.
Two more issues with that is you'd have to keep all uniforms in the exact same order everywhere, manually, and it probably won't work for textures.
There was a problem hiding this comment.
I suppose this is somewhat the case now since I did have to put some things into separate shader code files.
VReaperV
force-pushed
the
material
branch
3 times, most recently
from
94fc72e
to
7dfac81
Compare
|
|
Great! |
|
The remaining Mesa radeonsi error has a pending fix: I seen the proposed piglit test has been updated at the same time so it's probably for this too. Once I merge the two fixes, everything works again. I guess we will have to blacklist the broken Mesa versions for bindless textures the day we enable the feature. |
|
Good, once that is fixed I'll enable the extension but default. Yeah, a blacklist for driver versions and print a warning about a driver update required to run the extension properly. |
|
Here is an example of exhaustive search reporting the buggy driver version and disabling the extension when it's detected: index a5dc70924..d9f7f9796 100644
--- a/src/engine/sys/sdl_glimp.cpp
+++ b/src/engine/sys/sdl_glimp.cpp
@@ -2023,8 +2023,45 @@ static void GLimp_InitExtensions()
// made required in OpenGL 4.3
glConfig2.computeShaderAvailable = LOAD_EXTENSION_WITH_TEST( ExtFlag_NONE, ARB_compute_shader, r_arb_compute_shader.Get() );
- // not required by any OpenGL version
- glConfig2.bindlessTexturesAvailable = LOAD_EXTENSION_WITH_TEST( ExtFlag_NONE, ARB_bindless_texture, r_arb_bindless_texture.Get() );
+ {
+ bool foundMesa241 = false;
+ std::string mesaVersion = "";
+
+ static const std::vector<std::pair<std::string, std::vector<std::string>>> versions = {
+ { "1.0", { "1.0-devel", "1.0-rc1", "1.0-rc2", "1.0-rc3", "1.0-rc4", "1.0", } },
+ { "1.", { "1.1", "1.2", "1.3", "1.4", } },
+ { "2.0", { "2.0-devel", "2.0-rc1", } },
+ };
+
+ for ( auto& vp : versions )
+ {
+ mesaVersion = Str::Format( "Mesa 24.%s", vp.first );
+
+ if ( Q_stristr( glConfig.version_string, mesaVersion.c_str() ) )
+ {
+ for ( auto& v : vp.second )
+ {
+ mesaVersion = Str::Format( "Mesa 24.%s", v );
+
+ if ( Q_stristr( glConfig.version_string, mesaVersion.c_str() ) )
+ {
+ foundMesa241 = true;
+ break;
+ }
+ }
+
+ break;
+ }
+ }
+
+ if ( foundMesa241 )
+ {
+ logger.Notice( "...found buggy %s driver", mesaVersion );
+ }
+
+ // not required by any OpenGL version
+ glConfig2.bindlessTexturesAvailable = LOAD_EXTENSION_WITH_TEST( ExtFlag_NONE, ARB_bindless_texture, r_arb_bindless_texture.Get() && !foundMesa241 );
+ }
// made required in OpenGL 4.6
glConfig2.shaderDrawParametersAvailable = LOAD_EXTENSION_WITH_TEST( ExtFlag_NONE, ARB_shader_draw_parameters, r_arb_shader_draw_parameters.Get() );This does: The idea of being exhaustive is to make sure to not trigger that known bug when bisecting something else in Mesa. With some hope, the fixes will be merged before |
|
I was thinking of just a warning print, but I guess it's better to disable it as well on those versions since we know it doesn't work properly there. |
|
I edited the patch to make it more performant. |
|
If it's working properly on your end with buggy/non-buggy versions I'll add it to the pr later, unless you wanna add that yourself. |
|
We already do such kind of disablement for other buggy drivers / hardware, you can look for Running the game with bindless textures and material features enabled on that broken Mesa can crash the whole computer, so we better disable the feature.
So this is the failure with the most negative impact we are implementing a workaround for, we better avoid the bug if we can. With this patch the engine runs properly with both buggy and non-buggy driver (the engine properly disabling the broken feature when needed). It means by merging such patch it makes possible to also enable Before enabling it by default it would be good to have some feedback from tests with other drivers, especially the Nvidia one and the AMD proprietary drivers. I believe @slipher has some Nvidia. Current status (taking in account the above workaround is merged), it doesn't mean the feature is finally used, it means the game doesn't crash and renders properly with
|
|
AMD OGLP on Linux produces a black screen with bindless textures. The problem is that both AMD OGLP on Linux and AMD Adrenalin on Windows has |
I'll look at those, thanks. Nvidia works fine with it on Windows, I can maybe test it on an Intel iGPU later as well. |
Implement material system, set world BSP surfaces to use it. Material system works by generating enough materials (a distinct GL state that has to be set for the whole draw call) for each stage of every drawSurf_t. All of the per-surface data that was previously set through uniforms for each drawSurf_t is now sourced from a buffer. The remaining (global) uniforms like u_ViewUp and u_ViewOrigin are still set as usual. Updates for surfaces whose parameters change with time are done by mapping the corresponding part of the memory and copying all of the data into it, then flushing it all at once. Implement bindless texture support.
I've now added this patch. Also added a warn if |
That intel iGPU apparently doesn't support bindless textures, so I'll be unable to test that. |
|
In Khronos wiki: https://www.khronos.org/opengl/wiki/Bindless_Texture I read this:
But I don't know if that information is obsolete or not (that mention is from 2015, the page has not been updated since 2021). |
|
If it's ready to merge I can make a PR enabling bindless and detecting drivers for selective disablement after this is merged. Is there something remaining to do? |
|
According to the Intel Xe on Windows 10 supports bindless texture: https://opengl.gpuinfo.org/displayreport.php?id=13101 I don't own any Intel Xe. I have some Intel HD (from UHD generation) but I doubt it supports it, for example this public listing for an HD 520 doesn't list the bindless texture extension: https://opengl.gpuinfo.org/displayreport.php?id=13055 neither this one for an UHD 620: https://opengl.gpuinfo.org/displayreport.php?id=12782 |
|
gpuinfo.org is the one to check for hardware capabilities/extension availability yes.
It's the report for OpenGL ES, probably emulated or something there. The version for OpenGL has it - https://opengl.gpuinfo.org/displayreport.php?id=12781. |
Sure, that'd work well.
No, I'm just waiting for any more reviews or an LGTM :D |
|
Thx, biggest part of the changes done now finally. |
Base changes for implementing a material system.
The goal of this system is to improve performance by reducing the amount of state changes and data uploads from CPU->GPU.
The system works by generating materials for draw surfaces/shaders/shader stages after the map is loaded, as well as all the necessary data to draw each material in one drawcall. Currently this only works for world BSP surfaces, everything else is drawn the normal way.
Material
A material in this case represents a distinct global OpenGL state: this includes state bits (as used by GL_State(), however without the alphaTest bits), culling mode, shader program, material ordering and VBO/IBO bindings. Each material is also allocated 2 blocks of memory in a buffer, this memory holds the data for each surface.
Material system
The material system holds material packs, which map materials to various shader sorts: depth pre-pass, opaque pass, and translucent pass.
It also contains sky shaders and a list of dynamic surfaces.
A dynamic surface is a surface which can have its data changed after the map is loaded. This includes non-constant color, tex matrices, animated and video images, as well as any active expressions used by RB_EvalExpression().
All surface data is held in a shader storage buffer object. This buffer consists of 2 blocks which have the following memory layout:
Material0_surface0_stage0: uniform0 // Start of material 0 data
..
Material0_surface0_stage0: uniformn
Material0_surface0_stage0: optional padding
Material0_surface0_stage1: uniform0
..
Material0_surface0_stagex: uniformn
Material0_surface0_stagex: optional padding
..
Material1: optional padding
Material1_surface0_stage0: uniform0
etc.
The optional padding for per-surface data is needed for it to align with std430 layout. The optional padding for materials is needed because each material can have a different data size per surface, but the offset in the shader has to be an integer multiple of its materials size. The shader accesses the buffer using the offset that equals to:
bufferOffset / paddedSize,bufferOffsetis the offset in components i. e. multiples of 4 bytes because that's the minimum accessible memory in std430,paddedSizeis the size of this shaders Material struct in components, aligned under std430.The buffer is split into a static surface block, followed by a dynamic surface block. The dynamic surface block is updated once per frame. The whole block has to be updated because the BSP is entirely skipped for world surfaces.
There are 4 distinct steps done by the material system:
The command buffer contains data for multiDrawIndirect drawcalls. This contains the indexes into vertex and index array buffers, as well as baseInstance, which is used to access the buffer block in the shader. This data is static.
When rendering, all of the surfaces of a rendered material will be rendered with one such call.
While this means that all of the map surfaces are rendered every frame, it ends being much faster than walking the BSP tree and incurring all the state changes for maps with worse vis. Some maps that are good for testing this are https://users.unvanquished.net/~sweet/pkg/map-station12-b7_0.dpk and https://users.unvanquished.net/~kai/map-example_0.1.dpk.
The impact may vary depending on the device, but because currently at least on some devices perf can be worse with this system enabled depending on map, it is disabled by default. I expect that to work better or on par with what we currently have once GPU culling is added.
It can also later be extended to entities etc.
Some example performance differences:
There are a few issues that are out of scope of this pr: proper portal culling, heatHaze stages (the only map that I know of that use heatHaze is
usstremor, but there the heatHaze stage is on a non-world entity, so it works fine anyway), quakeFog stage.To use the material system, do
/r_arb_bindless_texture 1; r_useMaterialSystem 1; vid_restart.