Shader parameters in managed OpenGL

Question

I'm working on a managed OpenGL game engine for C#, and here's how I'm managing shader parameters (uniforms). Is it "alright"? Could it be better? I'm a bit unsure about using generics in this case.

public abstract class ShaderParameter
{
    public readonly string Name;
    internal readonly int Location;
    internal readonly Shader Parent;

    internal ShaderParameter(string name, int location, Shader parent)
    {
        Name = name;
        Location = location;
        Parent = parent;
    }
    //Ensures that the value is set on the shader,
    //this should be called before using the parent shader
    internal abstract void EnsureSet();
    //Gives a value to this shader paramerter
    public abstract void SetValue<T>(T value);
}



//A shader paramater of type float
public sealed class ShaderParamFloat : ShaderParameter
{
    float value = 0;
    bool valueChanged = false; //If the value has changed since the last time it was set on the shader

    internal ShaderParamFloat(string name, int location, Shader parent)
        : base(name, location, parent)
    { }
    internal override void EnsureSet()
    {
        if (valueChanged)
        {
            //Hands over a single value to OpenGL.
            GL.Uniform1(Location, value);
            valueChanged = false;
        }
    }
    //Gives a value to this shader parameter
    public override void SetValue<T>(T value)
    {
        if (typeof(T) != typeof(float))
            throw new ArgumentException("Value is of type: " + typeof(T).ToString() + ", expected: float", "value");

        this.value = (float)(object)value;
        valueChanged = true;
    }
}

The reason for using inheritance is because other ShaderParameter classes behave differently in EnsureSet(). For example, ShaderParamVector3 uses GL.Uniform3(...), and ShaderParamTexture needs to ensure that the texture is valid and set on the graphics card.

Answer 1

The biggest thing here is that you should make the ShaderParam class itself generic, not just its SetValue method. This will get rid of your type check and double cast (any time you have a type check or a double cast, chances are you're using the type system incorrectly). It will also get rid of the ShaderParamFloat class completely.

For example:

public class ShaderParam<T>
{
    private T value;

    ... (other code here)

    public void SetValue(T value)
    {
        this.value = value;
        ...
    }
}

You can now use this for floats by instantiating a ShaderParam<float>, plus it works just as well for other types.

Some other comments:

• Don't write comments that just duplicate the code. A variable named valueChanged doesn't need a comment saying "If the value has changed" - that much is obvious from the name. More importantly in this code, the method named EnsureSet() should have a comment explaining its purpose and why it should be called, not just a comment that expands the name into a longer sentence. I'm not an OpenGL user so I don't know what GL.Uniform1() does, but that code is surprising to me and a comment to explain why it's written that way would be useful.
• I take it you have good reasons for making some things public and others internal. To me, those choices seem pretty arbitrary. If it works in your situation, consider making the class itself internal and all members public.
• I don't personally like making fields public/internal, though given that they are readonly, I could live with it. I might personally still change this to use properties and a readonly backing field. That gives you the added benefit of being able to set breakpoints later if you ever need to debug.
• Not relevant if you make the first change above, but why is ShaderParamFloat still abstract?
• The private fields in ShaderParamFloat should be explicitly marked private.
• There is no need to initialize value to 0 or valueChanged to false. Those are their default values.
• I would personally call the class ShaderParameter instead of ShaderParam. Your abbreviation is pretty obvious in this case, but I tend to stay away from all but the most commonly used abbreviations. In particular, if using the abbreviation improves the readability of the code, then it makes sense. I don't think that applies here.

EDIT:

Based on your updated question, I would still recommend a solution like I mentioned above, where the base ShaderParameter is a generic class. However, it does make sense for you to derive classes from that with different implementations of EnsureSet. For example:

public abstract class ShaderParameter<T>
{
    private T value;

    public abstract void EnsureSet();

    public void SetValue(T value)
    {
        this.value = value;
    }
}

public class FloatShaderParameter : ShaderParameter<float>
{
    public override void EnsureSet()
    {
        ...
    }
}

And you can then derive similar classes for the other types of parameters. Note that you only need a single implementation of SetValue in the base, and it works for all derived classes.