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

Improve material system data packing #1473

Merged
merged 16 commits into from
Jan 28, 2025

Conversation

VReaperV
Copy link
Contributor

@VReaperV VReaperV commented Dec 25, 2024

Partially implements #1448.

Stage data is allocated for stages rather than surface * stage, which greatly reduces the amount of required data. Texture handles and texture matrix are moved into a different buffer, since stage data ends up being often under 1kb this way. Lightmap/deluxemap handles are moved into a different UBO, because they depend on the drawSurf rather than on shader stage. All 3 are encoded into baseInstance (12 bits for shader stage, 12 bits for textures, and 8 more for lightmaps).

Optimised u_Color by changing it from a vec4 to a uint (the colors we use only have 8-bit precision in engine).
Also added u_ColorModulateColorGen for use with shaders other than cameraEffects: these only use 12 distinct states, encoded as 5 bits, which also allows using a uint here instead of a vec4. Added unpackUnorm4x8() define for devices that don't support ARB_gpu_shader5.

Changed u_TextureMatrix from a mat4 to mat3x2, reducing its size from 16 to just 6 components since only these 6 components have any actual effect, which allows making the TexData struct be exactly 64 bytes.

All of the above combined reduces the memory usage of material system by ~50-100x, and improves its performance and load times.

Also added r_materialSystemSkip cvar, which allows instantaneously switching between core and material renderer (as long as the latter is enabled in the first place), for much faster debugging and performance comparison.

@VReaperV VReaperV added T-Improvement Improvement for an existing feature A-Renderer T-Performance labels Dec 25, 2024
@VReaperV VReaperV force-pushed the material-stages-pack branch 6 times, most recently from 1332e02 to 981f302 Compare December 26, 2024 08:06
@slipher
Copy link
Member

slipher commented Dec 26, 2024

Is there any way to split this up into multiple PRs?

@VReaperV
Copy link
Contributor Author

I believe so, yes.

@VReaperV
Copy link
Contributor Author

Is there any way to split this up into multiple PRs?

Parts of it are now in: #1475, #1476, #1477, and #1478. I think that's as far as I can split it (u_TextureMatrix change would end up with a bunch of merge conflicts here).

@VReaperV VReaperV force-pushed the material-stages-pack branch 2 times, most recently from f4a4702 to 3045e47 Compare January 8, 2025 10:20
@slipher
Copy link
Member

slipher commented Jan 11, 2025

Light styles are broken, e.g. map metro-b1-2 at setviewpos -1172 627 107.

unvanquished_2025-01-10_192237_000

@VReaperV
Copy link
Contributor Author

Thought I fixed those, gonna need to recheck it then.

@VReaperV
Copy link
Contributor Author

Light styles are broken, e.g. map metro-b1-2 at setviewpos -1172 627 107.

unvanquished_2025-01-10_192237_000

The black spot is a shader caching issue, deleting the glsl folder should fix it.

@VReaperV VReaperV force-pushed the material-stages-pack branch from 3045e47 to 217028e Compare January 11, 2025 11:48
@VReaperV
Copy link
Contributor Author

Nevermind, that does not fix it. It's an issue with dynamic lights though.

@VReaperV VReaperV force-pushed the material-stages-pack branch from f7c1c76 to 92ceaa8 Compare January 11, 2025 14:17
@VReaperV
Copy link
Contributor Author

Both issues fixed now.

@VReaperV VReaperV force-pushed the material-stages-pack branch 3 times, most recently from 60acd47 to f4cc9bd Compare January 22, 2025 22:26
Copy link
Member

@slipher slipher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests look good. Fixes #1499 and doesn't alter anything else.

Currently I have reviewed all commits except the first one.

src/engine/renderer/gl_shader.h Outdated Show resolved Hide resolved
@VReaperV
Copy link
Contributor Author

That is very good, thanks.

@VReaperV VReaperV force-pushed the material-stages-pack branch from df287f7 to 685684f Compare January 26, 2025 01:13
src/engine/renderer/Material.cpp Outdated Show resolved Hide resolved
src/engine/renderer/tr_local.h Outdated Show resolved Hide resolved
src/engine/renderer/Material.h Outdated Show resolved Hide resolved
for ( shaderStage_t* pStage : stages ) {
Material& material = materialPacks[pStage->materialPackID].materials[pStage->materialID];

/* Stage variants are essentially copies of the same stage with slightly different values that
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what's the ultimate purpose of variants? If I trace e.g. mayUseVertexOverbright/VERTEX_OVERBRIGHT, I can't find any decisions ever being made based on the variant, other than segregating it from other variants.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once some of the pStage members are set in MaterialSystem::GenerateMaterialsBuffer(), they are then used in pStage->surfaceDataUpdater() to set the correct surface data. Essentially, for each unique surface data all of the active variants are stored consecutively, with the differences dictated by variant.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems wrong to store them on pStage since if there is more than one draw surf using the same shader (which is the whole point of having variant bit masks, right?) the different values would clobber each other.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The offset stored in drawSurf_t includes the offset to the specific variant used for it (

const uint32_t SSBOOffset =
), computed and set in ProcessStage() and AddStage().

Copy link
Contributor Author

@VReaperV VReaperV Jan 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mapping between drawSurfs, shader stages, and material buffer can be thought of like this:

shaderStage_t* pStage = drawSurf->shader->stages[stage];

uint32_t baseSurfaceDataIndex = pStage->bufferOffset;
uint32_t variant = drawSurf->shaderVariant[stage]; // Always in the range of [0; active stage variants]
uint32_t variantOffset = pStage->variantOffsets[variant];
void* material = materials + baseSurfaceDataIndex + variantOffset; // materials is mapped to the start of the range for either static or dynamic stages
uint32_t materialOffset = pStage->materialOffset // Different from bufferOffset for dynamic stages
    + variantOffset; // materialOffset is what's then used in the GLSL shaders to index the material array 

lightMode_t lightMode;
deluxeMode_t deluxeMode;
SetLightDeluxeMode( drawSurf, pStage->type, lightMode, deluxeMode );
pStage->mayUseVertexOverbright = pStage->type == stageType_t::ST_COLORMAP && drawSurf->bspSurface && pStage->shaderBinder == BindShaderGeneric3D;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This data should be stored somewhere else. It's hacky to store drawsurf-specific data on the shader.

Copy link
Contributor Author

@VReaperV VReaperV Jan 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps, though I don't really see where else it could be stored. There could be a wrapper around shaderStage_t passed to GenerateMaterialsBuffer instead, and it could store all the variants, I suppose.

Copy link
Contributor Author

@VReaperV VReaperV Jan 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it's not truly stored in the shader, the shaderStage_t is just used as a proxy, so that surfaceDataUpdater() no longer needs a drawSurf_t as input. When the materials are initially generated, these use drawSurf to set the value to avoid using up memory and doing processing for stage variants that are never used.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I see that they are just temporary; used during one iteration of GenerateMaterialsBuffer.

If it's too burdensome to add an argument to those "updater" functions, it would still be better to store them in a global variable, instead of misleadingly them with shader stages.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made it work with propagating it through UpdateSurfaceData*().

@VReaperV VReaperV force-pushed the material-stages-pack branch 2 times, most recently from 6003e22 to 65e465b Compare January 27, 2025 01:17
Allocate memory for each stage of used shaders rather than for each stage in all used shaders for each surface that uses them. Change some of the material system functions to use `shaderStage_t*` instead of `drawSurf_t*` etc as input.

Textures are put into a different storage buffer, following the structure of `textureBundle_t`. This allows decreasing the total amount of memory used, and changing the material buffer to be a UBO instead. The lightmap and deluxemap handles are put into another UBO, because they're per-surface, rather than per-shader-stage.

Also moved some duplicate code into a function.
Only these 6 components (0, 1, 4, 5, 12, 13) actually have an effect. This largely reduces the amount of data being transferred for texture matrices, as well as some useless computations.

.
Also removed some now useless code.
Also fixes an incorrect matrix constructor in shaders.
The UBO bindings were conflicting with light UBO.
Also make it in line with other usages of `lightMap`.
@VReaperV VReaperV force-pushed the material-stages-pack branch from efe9430 to 6431517 Compare January 28, 2025 01:46
@slipher
Copy link
Member

slipher commented Jan 28, 2025

LGTM

@VReaperV VReaperV merged commit e9c9324 into DaemonEngine:master Jan 28, 2025
9 checks passed
@VReaperV VReaperV deleted the material-stages-pack branch January 28, 2025 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Material system: plat23 heat haze does not scroll
2 participants