Material/Shader System - Design Review

Started by
7 comments, last by Krohm 11 years, 8 months ago
Hiya,

I've implemented a simple material/shader system for the first time, which seems to work fairly well. I've tried to focus on good separation of concerns and the relationship between classes. I'd really appreciate it if anyone can have a quick look through and help me think about what's good and what's bad.

It's not an enormous design but I don't want to drop a mountain of code, so I'll step through it and explain the important parts.

The first thing I've done is to make a distinction between shaders and materials. A shader is the actual program, and owns a set of uniforms. A material holds a reference to a shader program, and contains a collection of values for the shader uniforms. So several materials can reference the same shader, but with different parameters.

On the shader side I've split the functionality into ShaderProgram and ShaderUniform classes, where each type of uniform is represented by a derived class. The uniform classes are just a thin interface to call the correct glUniformxxx() function.

ShaderUniform.h
[source lang="cpp"]
class ShaderUniform
{
public:
virtual ~ShaderUniform() {}

enum Type { TypeFloat, TypeVec3 /* etc. */ };

// Allows safe casting to the correct uniform sub-type.
Type getType() const;
};

typedef std::shared_ptr<ShaderUniform> ShaderUniformPtr;

// An example type:
class FloatUniform : public ShaderUniform
{
public:
FloatUniform(GLint uniformLocation);

// Calls glUniform1f()
void setValue(float value);
};
[/source]
The ShaderProgram is essentially an interface to the uniforms exposed by the shader. The uniforms are enumerated in the constructor, when the program is linked.

ShaderProgram.h
[source lang="cpp"]
class ShaderProgram
{
public:
// Link the shader and enumerate the available uniforms.
ShaderProgram(VertexShader *vs, FragmentShader *fs);

// Get a reference to a named uniform.
ShaderUniformPtr getUniform(const std::string& name);

void bind(); // Calls glUseProgram();

private:
std::map< std::string, ShaderUniformPtr > _uniforms;
}
[/source]
The higher level material system comprises two classes - Material and MaterialParameter. Each parameter relates to a single shader uniform. Parameter values are stored locally, and the associated uniform is updated when bind() is called.

MaterialParameter.h
[source lang="cpp"]
class MaterialParameter
{
public:
virtual ~MaterialParameter() {}

enum Type { FloatType, Vec3Type /* etc. */ };

// Allows safe casting to the correct parameter sub-type.
Type getType() const;

void bind();
};

class FloatParameter : public MaterialParameter
{
public:
FloatParameter(ShaderUniformPtr uniform);

void setValue(float value);

void bind(); // Calls ShaderUniform::setValue().

private:
ShaderUniformPtr _uniform;

float _value;
};
[/source]
Finally, the material class holds a shader program and collection of parameters.

Material.h
[source lang="cpp"]
class Material
{
public:
Material(ShaderProgram *program);

void setParameter(const std::string& name, float value);
void setParameter(const std::string& name, const Vec3& value);
// etc.

// Calls ShaderProgram::bind() and MaterialParameter::bind() for each parameter.
void bind();

private:
ShaderProgram* _program;

std::map< std::string, MaterialParameter* > _parameters;

};
[/source]

The setParameter() methods first check if the parameter is cached already. If not, the associated ShaderUniform is requested from the program and used to construct a new parameter, which is then cached. This also involves some type checking:

Material.cpp
[source lang="cpp"]
void Material::setParameter(const std::string& name, float value)
{
MaterialParameter *parameter = 0;

// Is the parameter already cached?
std::map< std::string, MaterialParameter* >::iterator itr = _parameters.find(name);

if(itr != _parameters.end())
{
parameter = itr->second;
}
else
{
// Not cached - create a new one.
parameter = new FloatParameter(_program->getUniform(name));

_parameters.insert(std::make_pair(name, parameter));
}

// Check the type.
if(parameter->getType() != Parameter::FloatType)
{
// throw type-mismatch.
}

// Set the new value.
static_cast<FloatParameter*>(parameter)->setValue(value);
}
[/source]

Overall I think it's OK, but there are a couple of parts I'm not crazy about.
- Constant need for type checks when a uniform is updated.
- Static casts: I'm not sure if this is good usage?
- The ShaderUniform and MaterialParameter classes essentially represent the same thing.

Any comments or suggestions whatsoever are greatly appreciated. :)

Cheers!
Advertisement
I've implemented two types of shader/parameter systems before. My first implementation was very similar to yours. It has a very nice OO approach. You probably don't need a separate MaterialParameter class, your Material could hold a list of ShaderUniforms and provide an interface to set them. Don't worry too much about type checks - during development they will save your butt, and you can always #define them out in a release build. I don't see where you are doing static casts, but if your checking the type, then I don't see a problem with it.

In my second implementation, I steered away from this design, and tried to create something much simpler. My ShaderUniform class became a struct PoD with a type and union of parameters. I have methods to set them and bind them using a switch statement instead of hitting the vtable through virtual functions (I haven't profiled both implementations yet, so I don't know which is faster). Using a type and union may be simpler, but as I add more types, I need to touch several pieces of code to keep it updated. Using your OO approach allows you to easily create a new derived ShaderUniform and everything should just work.

Overall, I think your design is fine. As long as it's easy to use and extensible... which you'll find out pretty quick once you start using it as an interface.

Edit: It's probably not necessary to cache the ShaderUniforms into your Material class. Why not just create all of the uniforms and give them a default value on Init()?
[font=courier new,courier,monospace]MaterialParameter[/font] is bad.
Don't cast just to call [font=courier new,courier,monospace]SetValue(float)[/font]. Use a basic interface instead example: [font=courier new,courier,monospace]SetValue(HardwareRegisters &set)[/font].
How do you set textures? How do you link to runtime "live" data? Do you need those features? I cannot reasonably tell anything about those by seeing your snippets.
You might need to separate your data model ([font=courier new,courier,monospace]MaterialParameter[/font]) from your live runtime mode (dealing with uniforms).

Previously "Krohm"

I've seen almost this exact design used in more than one game engine, so you can let that be a confidence booster that you've independently invented a similar solution to a proprietary engine wink.png

However, the biggest bottleneck in my experience with these engines was the setting of shader values -- we shipped one game with 30% of the main-thread CPU time being consumed by this task... So now, I really prefer to keep these systems as simple as possible, which means more PoD and less OO-overhead.

Also, I'd recommend basing these systems around UBO's rather than individual uniforms, as it makes everything a lot simpler and more efficient.
You could use a template in the shader uniform to avoid having to redefine your set and get fucntions, as in the sample below.


class ShaderUniformBase
{
...
}

template<class T>
class ShaderUniform : public ShaderUniformBase
{
...
void setUniform(const T& value) { m_value= value; }
const T& getUniform() const { return m_value; }
private:
T m_value;
}

typedef ShaderUniform<float> FloatUniform;
typedef ShaderUniform<int> IntUniform;


But as Hodgman already pointed out both GL and DX have moved on from the setting of single uniforms, to a more block based setup, which often tends to avoid setting uniforms that haven't changed.

Worked on titles: CMR:DiRT2, DiRT 3, DiRT: Showdown, GRID 2, theHunter, theHunter: Primal, Mad Max, Watch Dogs: Legion

Wow, a lot of replies - thanks!


You probably don't need a separate MaterialParameter class, your Material could hold a list of ShaderUniforms and provide an interface to set them.


I actually tried that originally, and I agree that it seems more logical. I'm using the ShaderUniform class to represent the current state of the uniform, which naturally can only have one value. The problem I found was that if several materials use the same shader but with different uniforms, then they need a way to store their own uniform values - which is why I added MaterialParameter to do this.

It might be possible to roll the MaterialParameter and ShaderUniform classes into one. If uniforms are copied to each material rather than being shared between them, then there would be no need for the parameter class at all.


[font=courier new,courier,monospace]MaterialParameter[/font] is bad.
Don't cast just to call [font=courier new,courier,monospace]SetValue(float)[/font]. Use a basic interface instead example: [font=courier new,courier,monospace]SetValue(HardwareRegisters &set)[/font].
How do you set textures? How do you link to runtime "live" data? Do you need those features? I cannot reasonably tell anything about those by seeing your snippets.
You might need to separate your data model ([font=courier new,courier,monospace]MaterialParameter[/font]) from your live runtime mode (dealing with uniforms).


I'm sorry to sound clueless, but could you explain your [font=courier new,courier,monospace]SetValue(HardwareRegisters &set) [font=arial,helvetica,sans-serif]method a little further? Would that be similar to a constant buffer or UBO?[/font][/font]

Textures are set using a TextureUniform class derived from ShaderUniform, which have a setValue(TexturePtr texture) method. Per-frame modification of material parameters isn't necessary for this project, but I should be able to make a deep copy of a material (i.e. including parameters), and call setParameter() as needed.


I've seen almost this exact design used in more than one game engine, so you can let that be a confidence booster that you've independently invented a similar solution to a proprietary engine wink.png

However, the biggest bottleneck in my experience with these engines was the setting of shader values -- we shipped one game with 30% of the main-thread CPU time being consumed by this task... So now, I really prefer to keep these systems as simple as possible, which means more PoD and less OO-overhead.


This is interesting to me. Each time a material parameter is set in my current design, it involves a number of vtable lookups and static casts, which probably isn't ideal for something that happens very often. Is this what was causing the performance issue?

I'm guessing by 'more PoD and less OO', you mean something like this?

class ShaderUniform
{
public:
void setValue(float value)
void setValue(const Vec3& value);
void setValue(const Mat4x4& value);
void setValue(TexturePtr value);
// etc...

UniformType getType() const;
};


Thanks again for all your replies, this is very helpful.
Additionally, thanks for the hints about UBOs - I hadn't heard of them until now.

One of the things I like about the design here is that I can define material parameters in an XML-like file, e.g.:

<Material>
<VertexShader>shader.vert</VertexShader>
<FragmentShader>shader.frag</FragmentShader>
<Parameter name="uniformA" type="float">1.0</Parameter>
<Parameter name="uniformB" type="int">5</Parameter>
</Material>


Using UBOs, there doesn't seem to be a way to get the names of the uniforms inside a block - just the name of the block itself. Could anyone suggest a way of integrating UBOs with a material file format like this?

Cheers.

Additionally, thanks for the hints about UBOs - I hadn't heard of them until now.

One of the things I like about the design here is that I can define material parameters in an XML-like file, e.g.:

<Material>
<VertexShader>shader.vert</VertexShader>
<FragmentShader>shader.frag</FragmentShader>
<Parameter name="uniformA" type="float">1.0</Parameter>
<Parameter name="uniformB" type="int">5</Parameter>
</Material>


Using UBOs, there doesn't seem to be a way to get the names of the uniforms inside a block - just the name of the block itself. Could anyone suggest a way of integrating UBOs with a material file format like this?

Cheers.


Well you could use a block construct arround parameters like this

<Material>
<VertexShader>shader.vert</VertexShader>
<FragmentShader>shader.frag</FragmentShader>
<UBO name="uniform_block_1">
<Parameter name="uniformA" type="float">1.0</Parameter>
<Parameter name="uniformB" type="int">5</Parameter>
</UBO>
</Material>


The point is that you don't really need a name for the uniforms any more, they are just a memory area that maps to the UBO that has a structure the shader expects to have. But in the layout I used you could ignore the block tag and just use the uniforms inside if the card you were running on doesn't support UBO's.

Worked on titles: CMR:DiRT2, DiRT 3, DiRT: Showdown, GRID 2, theHunter, theHunter: Primal, Mad Max, Watch Dogs: Legion


I'm sorry to sound clueless, but could you explain your SetValue(HardwareRegisters &set) method a little further? Would that be similar to a constant buffer or UBO?

Textures are set using a TextureUniform class derived from ShaderUniform, which have a setValue(TexturePtr texture) method. Per-frame modification of material parameters isn't necessary for this project, but I should be able to make a deep copy of a material (i.e. including parameters), and call setParameter() as needed.
I'll try. We're talking about thin air here.
The whole point of programming is to make generalizations by recognizing similar behavior across a range of entities we need to handle. In OOP we push the common behavior to the base class and then use derived classes to manage the special cases. The whole point is to let the compiler select the correct thing to do for us.

Now, in your material.cpp you do:

// Check the type.
if(parameter->getType() != Parameter::FloatType)
{
// throw type-mismatch.
}

// Set the new value.
static_cast(parameter)->setValue(value);

In general the problem takes the form of a sequence of [font=courier new,courier,monospace]if[/font]s, each with a cast and executing a special code path. This is required because your setValue call is not a base interface, it is defined in each derived class. It buys you type safety but at quite some cost. We cannot move setValue to base class, unless we allow the base interface to accommodate all possible special behaviors of each derived class. Which takes us to the [font=courier new,courier,monospace]HardwareRegisters [/font]structure. It could be something like this:


struct HardwareRegisters {
BOOL bools[16];
INT int4s[16 * 4];
float fp[SOME_COUNT];
SamplerStruct *samplerState[16];
}; // nice for D3D9
struct HardwareRegisters {
SomeBufferStructure *buffers[16];
SamplerStruct*textures[128]
}; // nice for D3D11, uniform buffers supported
This allows you to only use the base interface and let the compiler figure out the correct thing to do by means of a [font=courier new,courier,monospace]virtual [/font]function call.

It's worth noticing that this might have some serious consequences on your perf, as Hodgman notes above. Personally I think it's a good solution for personal projects.

Previously "Krohm"

This topic is closed to new replies.

Advertisement