Opengl ShaderUniform is incompatible with GLDeferredCommandBuffer.

[raxvan]

It seems that if you want to set the uniforms are not stored in the deferred command buffer for opengl.

@LukasBanana help wanted ?

[LukasBanana]

Do you have a practical use case, or how did you found that bug?

The ShaderUniform interface was intended only to support setting up sampler slots in GLSL 4.10 and older, when explicit uniform locations are not supported.

I see two options here:

1. The ShaderUniform interface can only be used to set uniform locations outside of command recording (i.e. between CommandBuffer::Begin/End calls). Adding this specification to the documentation and the validation layer should prevent an invalid use of this interface.
2. The ShaderProgram::LockShaderUniform/UnlockShaderUniform functions should be moved from the ShaderProgram to the CommandBuffer interface. The GLDeferredCommandBuffer will than return a different implementation of the ShaderUniform interface that manages setting uniforms differently by storing the values in a local buffer.

   
   // Before:
   class ShaderProgram {
   ShaderUniform* LockShaderUniform();
   UnlockShaderUniform();
   };

// After: class CommandBuffer { ShaderUniform* LockShaderUniform(ShaderProgram&); UnlockShaderUniform(); };

I prefer option 1, because modern rendering APIs use constant buffers primarily and OpenGL 3.1+ supports it. Or maybe we stick with option 1, and option 2 is postponed for a later version. I don't see necessity of this interface right now. Although I generally like backwards compatibility, I think this feature is not worth the effort as LLGL is designed after the model of D3D12/Vulkan.

Did you have something in mind already?
Nevertheless, I appreciate your contributions and support for this project 👍 

[raxvan]

hello,

i was thinking exactly for option 2 because it's very a convenient option for prototypeing. i know its only opengl but this would make it easier to port existing code bases to llgl and eventually switch to uniform buffers. for implementation both options 1 and 2 are viable.

razvan.

raxvan. On Fri, Jul 5, 2019, 21:21 Lukas Hermanns notifications@github.com wrote:

Do you have a practical use case, or how did you found that bug?

The ShaderUniform interface was intended only to support setting up sampler slots in GLSL 4.10 and older, when explicit uniform locations https://www.khronos.org/opengl/wiki/Layout_Qualifier_(GLSL)#Explicit_uniform_location are not supported.

I see two options here:

1. The ShaderUniform interface can only be used to set uniform locations outside of command recording (i.e. between CommandBuffer::Begin/End calls). Adding this specification to the documentation and the validation layer should prevent an invalid use of this interface.
2. The ShaderProgram::LockShaderUniform/UnlockShaderUniform functions should be moved from the ShaderProgram to the CommandBuffer interface. The GLDeferredCommandBuffer will than return a different implementation of the ShaderUniform interface that manages setting uniforms differently by storing the values in a local buffer.

// Before: class ShaderProgram {

ShaderUniform* LockShaderUniform();

UnlockShaderUniform();

};

// After: class CommandBuffer {

ShaderUniform* LockShaderUniform(ShaderProgram&);

UnlockShaderUniform();

};

I prefer option 1, because modern rendering APIs use constant buffers primarily and OpenGL 3.1+ supports it. Or maybe we stick with option 1, and option 2 is postponed for a later version. I don't see necessity of this interface right now. Although I generally like backwards compatibility, I think this feature is not worth the effort as LLGL is designed after the model of D3D12/Vulkan.

Did you have something in mind already? Nevertheless, I appreciate your contributions and support for this project 👍

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/LukasBanana/LLGL/issues/38?email_source=notifications&email_token=AC2SACANWME6KHJD43QQKNDP56GMPA5CNFSM4H6MRFSKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZKC2QQ#issuecomment-508833090, or mute the thread https://github.com/notifications/unsubscribe-auth/AC2SACBJCOGRIOPVSUAGQMLP56GMPANCNFSM4H6MRFSA .

[LukasBanana]

I'm thinking about a complete refactor and removal of the ShaderUniform interface. My proposal for the new design looks like this:

class CommandBuffer {
    // Use glUniform* for OpenGL
    // Use vkCmdPushConstants for Vulkan
    // Use SetGraphicsRoot32BitConstants for D3D12
    void SetConstants(
        ShaderConstant shaderConstant,
        const void*    data,
        std::size_t    dataSize,
        long           stageFlags
    );
};
class ShaderProgram {
    ShaderConstant QueryShaderConstant(const char* name) const;
};
struct ShaderConstant {
    UniformType  type;
    std::int32_t location;
};

This is more conformant to the way Vulkan and D3D12 handle individual shader constants. It also drastically reduces the amount of interface functions by entirely replacing the ShaderUniform class. I didn't care much about that in the beginning, because it was only meant to be used with the OpenGL backend.

Back in the days when I programmed with D3D9, shader constants needed to be set every time a new shader was bound to the pipeline. In OpenGL, glUniform is constistent for its shader program but D3D12 and Vulkan are not. So it's probably reasonable to go the same way as the modern APIs do.

[raxvan]

Looks good. Removing ShaderUniform would simplify the code a bit by removing the need to have boundProgramStack and it increase safety since you only need to error check the bound pipeline in command buffer. I'm not that familiar with VK and DX12, i can only safely handle gl implementation.

[LukasBanana]

I started with the refactoring in an experimental branch (see feb1fa5).

[LukasBanana]

I did some refactoring today regards the shader uniforms:

• ShadeUniform interface has been removed and replaced by the SetUniform/SetUniforms functions in the CommandBuffer interface.
• The new internal class GLShaderBindingLayout is used to set uniform block binding, shader-storage binding, and sampler bindings in case the backend does not support explicit uniform locations in the shader (as with GLSL before version 420). A shared reference to an instance of this new class is stored in the GLGraphicsPipeline class.
• SetUniform/SetUniforms is supported with OpenGL for both immediate and deferrerd command buffers; Vulkan and D3D12 should be supported, too ... some day :)
• Adde name property to BindingDescriptor structure, which is only required for OpenGL before 4.20, or when explicit uniform locations in GLSL are not supported.
• Extended the specification and parser of PipelineLayoutDesc utility function to support the name property.

[LukasBanana]

I added the changes of the new interface for shader uniforms to the ChangeLog-v0.03 document.

[LukasBanana]

Fixed with 15025b4.