shader-slang / slang

Making it easier to work with shaders
MIT License
1.78k stars 158 forks source link

GLSLForceScalarLayout doesn't seem to work as CompileOptionEntry #4300

Open chaoticbob opened 2 weeks ago

chaoticbob commented 2 weeks ago

I'm trying to force scalar block layout via the Slang API and I've gotten it to work by setting slang::TargetDesc::forceGLSLScalarBufferLayout to true. However when I try to do this with the compiler options via slang::SessionDesc using a slang::Compiler::Entry - it doesn't seem to work. I've walked through the code a bit but I'm not able to see where in the compiler options processing for a slang::CompilerOptionName::GLSLForceScalarLayout entry takes place.

Here's what my code looks like:

std::vector<slang::CompilerOptionEntry> compilerOptions;

compilerOptions.push_back(
    slang::CompilerOptionEntry{
        slang::CompilerOptionName::GLSLForceScalarLayout,
        slang::CompilerOptionValue{slang::CompilerOptionValueKind::Int, 1}
});  

slang::SessionDesc sessionDesc       = {};
sessionDesc.targets                  = &targetDesc;
sessionDesc.targetCount              = 1;
sessionDesc.defaultMatrixLayoutMode  = SLANG_MATRIX_LAYOUT_COLUMN_MAJOR;
sessionDesc.compilerOptionEntries    = compilerOptions.data();
sessionDesc.compilerOptionEntryCount = static_cast<uint32_t>(compilerOptions.size());

Was wondering what the correct way to force scalar block layout via the compiler options is.

csyonghe commented 2 weeks ago

The issue here is that there is a forceGLSLScalarBufferLayout field in TargetDesc that is overriding the option set in the session.

chaoticbob commented 2 weeks ago

Thanks. I now understand the behavior.

It was initially confusing for me because I was expecting to the target to inherit the session's flags if I didn't set anything in the target. It seems like having a bool for forceGLSLScalarBufferLayout could be potentially confusing if someone is expecting to map their command line flags to a list of compiler options.

csyonghe commented 2 weeks ago

Yes, forceGLSLScalarBufferLayout could cause confusion in this case, but it also has been there for a long time and we can't just remove it or we will break binary compatibility, so we will have to live with it for now.

I am closing this issue now that the problem has been clarified. If you would like us to implement any improvements, feel free to reopen.

swoods-nv commented 2 weeks ago

We should document this behavior.