Skip to content

Conversation

VReaperV
Copy link
Contributor

This marks shaders correctly for materials that have specular w. r. t. reflection mapping settings.

@VReaperV VReaperV added T-Improvement Improvement for an existing feature A-Renderer labels Aug 30, 2025
@VReaperV VReaperV moved this to In Progress in Material system Aug 30, 2025
pStage->materialProcessor( &material, pStage, surface );

if ( material.enableSpecularMapping ) {
/* This will get the non-reflective version of the material if reflection mapping is enabled,
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't cubemaps being unavailable give you the version with specular mapping enabled but reflective specular disabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah? Reflective specular isn't used if there are no cubemaps available, so I'm not sure what the question is here.

Copy link
Member

Choose a reason for hiding this comment

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

Never mind, I thought there would be a compile option for enable specular but there isn't.

But in any case, the branch doesn't work for me. If I load Parpax with reflection mapping enabled, I get about 5 single shader builds after running buildcubemaps either with this branch or on master.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should work correctly now.

Copy link
Member

Choose a reason for hiding this comment

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

Still not working on my end - with the test in the previous comment I get the same results. The variants with reflective specular enabled are not pre-built.

@VReaperV VReaperV force-pushed the material-mark-shaders branch from eb32ff8 to 9feb755 Compare September 5, 2025 14:12
@VReaperV VReaperV force-pushed the material-mark-shaders branch from 9feb755 to 667222f Compare September 5, 2025 15:23
@slipher
Copy link
Member

slipher commented Sep 7, 2025

Context for NUKE bad assert? I believe it's correct - if padding is added into the uniform size it will be multiplied by the number of components resulting in a wrong pointer returned from WriteToBuffer.

@VReaperV
Copy link
Contributor Author

VReaperV commented Sep 7, 2025

Context for NUKE bad assert? I believe it's correct - if padding is added into the uniform size it will be multiplied by the number of components resulting in a wrong pointer returned from WriteToBuffer.

Because it's dereferencing a nullptr/random garbage.

@slipher
Copy link
Member

slipher commented Sep 7, 2025

Because it's dereferencing a nullptr/random garbage.

Oh right the iterator is not valid. It's supposed to say ASSERT( !materialSystemUniforms.back()->_components ).

@slipher
Copy link
Member

slipher commented Sep 13, 2025

I think I may have tested it wrong the previous time (without material system). I tested again and it does reduce the amount of lazily built shaders when cubemaps are not built. So the shader building commit LGTM.

@illwieckz
Copy link
Member

So to merge this I guess the ASSERT just has to be reintroduced but the ASSERT( !materialSystemUniforms.back()->_components ) way?

@slipher
Copy link
Member

slipher commented Sep 17, 2025

#1810 fixes the wrong assert.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Renderer T-Improvement Improvement for an existing feature
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants