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

Better legacy GLSL compatibility #42

Closed
tklajnscek opened this issue Mar 19, 2019 · 4 comments
Closed

Better legacy GLSL compatibility #42

tklajnscek opened this issue Mar 19, 2019 · 4 comments
Labels
bug Something isn't working

Comments

@tklajnscek
Copy link

Is your feature request related to a problem? Please describe.
My engine supports GLES2 / WebGL1 in addition to newer OpenGL versions and Vulkan/DX12/Metal. By default, the shaders generated don't play well with the legacy GLSL versions. Since it's still a major target, I suggest some improvements & conventions to make it work out of the box.

Describe the solution you'd like
I suggest two improvements using SPRIVCross's shader reflection API:

  • Make the vertex output names match the fragment input names (renaming out_var_X and in_var_X to varying_X)
  • Append descriptor binding info into uniform names so it can be easily extracted when uniforms are enumerated after compilation/linking

Describe alternatives you've considered
The minor changes I propose should cover most of the use cases, but there might be a way to do this more generically with callbacks etc, although I feel it would go against the super streamlined design of ShaderConductor and would leak implementation details into user code.

Additional context
This is the code I'm using currently in ConvertBinary():

case ShadingLanguage::Glsl:
case ShadingLanguage::Essl:
    compiler = std::make_unique<spirv_cross::CompilerGLSL>(spirvIr, spirvSize);
    combinedImageSamplers = true;
    buildDummySampler = true;

    // Legacy GLSL fixups
    if (intVersion <= 300)
    {
        auto&& vars = compiler->get_active_interface_variables();
        for (auto& var : vars)
        {
            auto varClass = compiler->get_storage_class(var);
            
            // Make VS out and PS in variable names match
            if (source.stage == ShaderStage::VertexShader && varClass == spv::StorageClass::StorageClassOutput)
            {
                auto name = compiler->get_name(var);
                if (name.find("out_var") == 0)
                {
                    name.replace(0, 7, "varying");
                    compiler->set_name(var, name);
                }
            }
            else if (source.stage == ShaderStage::PixelShader && varClass == spv::StorageClass::StorageClassInput)
            {
                auto name = compiler->get_name(var);
                if (name.find("in_var") == 0)
                {
                    name.replace(0, 6, "varying");
                    compiler->set_name(var, name);
                }
            }
            
            // Encode binding info into variable name for uniform buffers, textures, samplers
            if (varClass == spv::StorageClass::StorageClassUniform || varClass == spv::StorageClass::StorageClassUniformConstant)
            {
                auto space = compiler->get_decoration(var, spv::Decoration::DecorationDescriptorSet);
				auto reg = compiler->get_decoration(var, spv::Decoration::DecorationBinding);
            
                char buf[128];
                sprintf(buf, "_reg_%d_space_%d", reg, space);
            
				auto type = compiler->get_type_from_variable(var);
 				auto typeName = compiler->get_name(type.self);
 				typeName.append(buf);
 				compiler->set_name(type.self, typeName);
            
                auto name = compiler->get_name(var);
                name.append(buf);
                compiler->set_name(var, name);
            }
        }
    }

    break;
@WJsjtu
Copy link

WJsjtu commented Mar 25, 2020

I've met the same problem. And this works for me when I try to use it for es 300 in WebGL2 since Separate Shader Objects feature is not supported until 310.

@gongminmin
Copy link
Contributor

Thanks. I'll add your change. It looks like appending descriptor binding info into uniform names is a temporally workaround for extracting information. It can be replaced by a future reflection APIs.

@gongminmin
Copy link
Contributor

I have to remove the appending descriptor binding info into uniform names. Because the spv::Decoration::DecorationBinding is different on macOS. The test can't pass with it.

gongminmin pushed a commit that referenced this issue Apr 11, 2020
@gongminmin gongminmin reopened this Sep 25, 2020
@gongminmin
Copy link
Contributor

A regression happens after update the SPIRV-Cross. The "in_var_xxx" is changed to "in.var.xxx". Need to adopt this change.

@gongminmin gongminmin added the bug Something isn't working label Sep 25, 2020
gongminmin pushed a commit that referenced this issue Sep 26, 2020
…nges in SPIRV-Cross

Related work item: Github #42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants