HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Shader class implementation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
implementationclassshader

Problem

I'm trying to design a Shader class. For now, I'm just trying to get basic outline for handling uniforms. Some design goals I aimed for is to avoid passing a string to a function as this is error prone and has no type checking. There's really only one place this can go wrong: if you mistype the name of the uniform when defining it.

There is a lot of potential for code bloat. I could potentially move the Name out of the template parameters for Parameter and store it as a member variable, which would get assigned in the default constructor.

template
class Shader
{
public:
    template
    class Parameter
    {
        static const char* name;
        static GLuint location;
        static GLuint program;

        template friend class Shader;
    };

    template
    void SetParameter(const Parameter&, const Type& value )
    {
        // static_assert(!std::is_same::value, "Unsupported type, needs specialization.");
        typedef Parameter Param;

        if(program != -1 && Param::program != program)
        {
            // set Param::location
            Param::program = program;
        }

        std::cout 
template
const char* Shader::Parameter::name = Name();

template
template
GLuint Shader::Parameter::program = -1;

#include "shaderparameters.inl"


shaderparameters.inl:

#define MY_PARAMETER(shader, type, name) \
    private: static const char* ShaderParameterStr##shader##type##name##() { return #name ; }  \
    public:  typedef Shader::Parameter name;

struct ShaderParam
{
    struct Generic
    {
        MY_PARAMETER( Generic, Vec4, position )
        MY_PARAMETER( Generic, Mat4, projection )
    };

};
#undef MY_PARAMETER


Usage:

Shader s;

s.SetParameter(ShaderParam::Generic::projection(), Mat4());
s.SetParameter(ShaderParam::Generic::position(), Vec4());


What are you thoughts on this implementation? Do you have suggestions for improvements or perhaps an entirely different system?

I know I've seen people use hash m

Solution

What are you thoughts on this implementation?

It is simply awful. Besides code bloat, just look how you declare such objects, and how you call methods on such objects.


Do you have suggestions for improvements or perhaps an entirely different system?

So, to fix your implementation :

  • add an interface for your Shader class. Like that, you can have different shader classes for vertex, geometric and pixel shaders



  • remove all templates



  • pass the shader's program id to constructor, and remove it from the Parameter class



  • move 'class Parameter' out of the 'class Shader', and make it a structure. It's member variables do not need to be static. By the way, name should be std::string - not const char*

Context

StackExchange Code Review Q#30893, answer score: 2

Revisions (0)

No revisions yet.