Is this an acceptable way to abstract the pipeline stages in DirectX?

First of all, I wouldn't call it Model. I would also consider decoupling the shaders from their data (but that is to be evaluated in a bigger context).

Some concepts you might want to consider

• pipeline (I used this term to refer to a specific VS+GS+PS combination, if memory serves I also included ROP there)
• shader: instance of a pipeline with specific data (vertex bindings, textures etc)
• model: asset to be loaded and shared across…
• model instances: takes the common data from the unique model and replicates it multiple times reusing the same buffers.

Who holds ownership for your pointers (yes, I do recognize reference counting is a thing yet I think ownership must be designed). Since you use a std::vector of pointers I assume all your pointers are shared.

At the very least your pipeline could be a proper structure. In general less identifiers around = better.

@vanillasnake21 the model class you have declared is fine but what matters is how you're using it in your pipeline as you have found..

to avoid those redundant states then u need to think about how to design the pipeline such that those states are set “once per situation” for all or some models involved in this situation;

for example, let's say u want render a rainy scene where 10 models are running, if u followed what @krohm said, u would do this:

• pipeline: set rain_vs, gs, ps once
• rasterizer: set zbuffer once
• shader: set shader resources i.e. 1 texture array containing 10 textures, Vbuffers, Cbuffer…etc
• 1 draw indexed call for 10 models
• shader: unset resources
• rasterizer: unset
• pipeline: unset

u see, in this “rainy” situation (aka game state), u have set everything once and unset them once. That means, If you know that you are going to be in this game state for a while then you could set the pipeline and the rasterizer once and not unset them until when this situation changes to "sunny". That means while it is rraining you're only updating the animated data or if u like things that have changed in this situation/state. This will avoid redundancies.

That's it … all best ?

Krohm said:

First of all, I wouldn't call it Model. I would also consider decoupling the shaders from their data (but that is to be evaluated in a bigger context).

Ok so context I'm working in is this: I have a separate Shaders namespace in which I create all my shaders and their respective input layouts. Like this:

namespace Shaders
{
	//Shader collection
	extern ID3D11VertexShader* WorldTransformVertexShader;
	extern ID3D11VertexShader* LineVertexShader;
	extern ID3D11PixelShader* LinePixelShader;
	extern ID3D11PixelShader*  TexturePixelShader;
	extern ID3D11GeometryShader* LineGeometryShader;


	//Common input layout for majority of shaders and a seperate one for line renderer
	extern ID3D11InputLayout* CommonInputLayout;
	extern ID3D11InputLayout* LineInputLayout;


	//Vertex descriptions
	extern D3D11_INPUT_ELEMENT_DESC* VertexLayoutDescription;
	extern D3D11_INPUT_ELEMENT_DESC* LineVertexLayoutDescription;


	//These compile shaders and output their blobs if needed,

	//so far only using vertex, pixel and geometry shaders
	void CompileAndCreateShader(ID3D11Device* device, std::wstring filename, std::string entrypoint, 	ID3D11VertexShader** outShader, ID3DBlob** outBlob = NULL);
	void CompileAndCreateShader(ID3D11Device* device, std::wstring filename, std::string entrypoint, ID3D11PixelShader** outShader, ID3DBlob** outBlob = NULL);
	void CompileAndCreateShader(ID3D11Device* device, std::wstring filename, std::string entrypoint, ID3D11GeometryShader** outShader, ID3DBlob** outBlob = NULL);


	//General orchestrator functions that initializes all shaders the game is using
	void CreateShadersAndInputLayouts(ID3D11Device* device);
};

So all the shaders which are used are pretty much in a global namespace Shaders::. This namespace is the owner of the pointers to also answer your last question.

I then pass the shaders and other resources to the Model's constructor which looks like this:

	//Just pass in any shaders that the mesh is expected to use to render itself
	//GenericObject is just a structure that holds the actual vertex and index arrays and texture filenames
	Model(ID3D11Device* device, GenericObject LoadedObject, ID3D11InputLayout* input_layout, ID3D11VertexShader* vertex_shader, ID3D11PixelShader* pixel_shader, ID3D11GeometryShader* geometry_shader = NULL, ID3D11HullShader* hull_shader = NULL, ID3D11DomainShader* domain_shader = NULL);

The constructor only requires a vertex and pixel shaders, all other shaders are optional.

So that's just a bigger picture. And just a few questions about your comment:

When you say decouple shaders from their data, is what I'm doing above sufficient? The shaders themselves are in a separate namespace and just the data (textures, samplers) are associated with the Model, nothing else. Or is there an even cleaner way to decouple them?

By the way my constant buffers are also in a namespace of their own similar to that of shaders

namespace ConstantBuffers
{

	__declspec(align(16)) struct ProjectionVariables
	{

		DirectX::XMMATRIX View;
		DirectX::XMMATRIX Projection;

	};

	__declspec(align(16)) struct OrthographicProjectionVariables
	{

		DirectX::XMMATRIX ProjectionOrthographic;

	};

	__declspec(align(16)) struct WorldMatrices
	{
		DirectX::XMMATRIX World;
	};

	__declspec(align(16)) struct LineRendererVariables
	{
		DirectX::XMFLOAT3 CameraPosition;
		float  LineThickness;
	};


	void CreateConstantBuffers(ID3D11Device* device);

	extern ID3D11Buffer*  ViewProjBuffer;
	extern ID3D11Buffer*  WorldMatrixBuffer;
	extern ID3D11Buffer*  LineRendererBuffer;
	extern ID3D11Buffer*  OrthographicProjectionBuffer;

};

I then have methods in the Model class that set the respective shader's constant buffer list

class Model
{
public:

	void SetVSConstantBuffers(ID3D11Buffer** ConstantBuffers, UINT numBuffers);
	void SetPSConstantBuffers(ID3D11Buffer** ConstantBuffers, UINT numBuffers);
	void SetGSConstantBuffers(...);
	//and so on for all the shaders
	

So the shaders and constant buffers are kind of separate from the Model, which only is really responsibe for loading and creating the textures that the shaders will be using.

Also you say that “At the very least your pipeline could be a proper structure. In general less identifiers around = better.”. Can you an example of how that could be done? By reducing identifiers you mean that I should strive to completely remove references to DirectX API within the game code and use some abstractions to hide the implementations? So I should have like an effect system or something to that extent? Thanks for the feedback, by the way.

@ddlox

ddlox said:

for example, let's say u want render a rainy scene where 10 models are running, if u followed what @krohm said, u would do this:

• pipeline: set rain_vs, gs, ps once
• rasterizer: set zbuffer once
• shader: set shader resources i.e. 1 texture array containing 10 textures, Vbuffers, Cbuffer…etc
• 1 draw indexed call for 10 models
• shader: unset resources
• rasterizer: unset
• pipeline: unset

So I should focus more on scene centered approach? Something akin to what the old Effects framework in DirectX10 used to do? The reason I went with per object approach is because it's so intuitive for me. I define individually what each object will look like by assigning a shader (see my answer above about how I handle shader creation) that will handle that particular look. The way I was thinking about it was like this: a shader is anologous to a material an object is made of and how that material behaves. So it was natural for me to kind of couple that with the actual object. It's a bit more unnatural for me to think of a material as like an effect that I apply first and then pass the objects, and then unapply the effect and render the next batch of objects. I was actually thinking of fixing the redundant states by some simple solution like grouping models by the shaders they use and keeping track if a shader was set already and just not setting it was set previously.

So I should focus more on scene centered approach?

focus on what works for you ?;

what i showed you is an example of how you can also get rid of redundancies;

hope that's clarified it;

Until then ?

VanillaSnake21 said:
Also you say that “At the very least your pipeline could be a proper structure. In general less identifiers around = better.”. Can you an example of how that could be done? By reducing identifiers you mean that I should strive to completely remove references to DirectX API…

… that would be valuable at some point but in my experience you'll need a very advanced usage (i.e. being multi-API) to reap the benefits. In retrospect, I doubt I would do it myself nowadays.

I meant something much simpler than that. Instead of having all your structures packet together consider the following:

class Pipeline {
	ID3D11VertexShader* vertex;
	ID3D11HullShader* hull;
	ID3D11DomainShader* domain;
	ID3D11GeometryShader* geometry;
	ID3D11PixelShader* pixel;
	// Maybe also ROP
};

Then your bigger class can just bring those in by composition. Those objects are fairly well defined. The combination of VS/PS etc is a valuable resource - it can help you for example bucketize them. We might say this is a future optimization. It's more than this.

When I previously said to think at your code in the bigger picture I didn't mean how the code does things. I meant “think at your usage”. You should not let code imply “up” what you need it's vice versa. Good code is a good representation of your ideas. Ideas push "down” implementation.

Your code is… particular. To say the least. How exactly this namespace magic works reliably is beyond my understanding so to say but seeing your “ConstantBuffer” namespace approach I suspect you are going after a simple application. I expect you will need to rework this. Extensively.

Remeber: when you ask yourself how to improve the answer is often not in the code. It is in where you want to go. What are you trying to do?

1. Is this a prototype, explorative project? Hack your way to the end and gain all the possible knowledge.
2. Do you see yourself pulling forward this long time? Conceptual integrity is key. Tags: architecture, KISS, clean.
3. Bonus: you can be somewhere in-between (but I doubt).

@Krohm I see what you're saying. To elaborate on my intentions with this project - it's actually a bit circular but the goal was learn the pipeline in detail. I want to understand all the relationships in depth, like relationship of views to resources of managing buffers, managing shaders, best practices in the areas etc. The scope of the project is just to become really proficient in programming for this API. This is kind of my initial approach at it. I would like to have a clean, cohesive system that I could then use in a future project and along the way learn all the API internals.

Krohm said:

Your code is… particular. To say the least. How exactly this namespace magic works reliably is beyond my understanding so to say but seeing your “ConstantBuffer” namespace approach I suspect you are going after a simple application. I expect you will need to rework this. Extensively.

Can we talk about this some more? I've seen this approach used in Frank Luna books and it seemed very intuitive to me. I use it to manage my shaders and my constant buffers. Can you explain why it's not a reliable method? What would be an alternative?

VanillaSnake21 said:

@Krohm I see what you're saying. To elaborate on my intentions with this project - it's actually a bit circular but the goal was learn the pipeline in detail. I want to understand all the relationships in depth, like relationship of views to resources of managing buffers, managing shaders, best practices in the areas etc. The scope of the project is just to become really proficient in programming for this API. This is kind of my initial approach at it. I would like to have a clean, cohesive system that I could then use in a future project and along the way learn all the API internals.

Then I would encourage sticking to the most basic things.

Split your Model class into at least two. Maybe find another name. If you need to understand you need source code which maps well to your mind. Your mind will hold ~7 things. Try to have a similar amount of identifiers. Try to get your hands on as much art as you can.

I suspect it is intuitive and adeguate for explaining things but when you see double pointers… careful there! Take for example those:

void CompileAndCreateShader(ID3D11Device* device, std::wstring filename, std::string entrypoint, 	ID3D11VertexShader** outShader, ID3DBlob** outBlob = NULL);
	void CompileAndCreateShader(ID3D11Device* device, std::wstring filename, std::string entrypoint, ID3D11PixelShader** outShader, ID3DBlob** outBlob = NULL);
	void CompileAndCreateShader(ID3D11Device* device, std::wstring filename, std::string entrypoint, ID3D11GeometryShader** outShader, ID3DBlob** outBlob = NULL);

It is pretty obvious this wants to create a SomethingShader by returning it from outShader argument. This is common in the lower-level APIs because they return something already (HRESULT). Those functions return void. Just return the pointer. I'm not aware what the blobs are supposed to be.

I note there are no arguments mapping to the concept of ID3D11InputLayout for example. They seem to come from the extern identifiers/variables. This is shared state across calls. It is a well-known anti-pattern. Your calls should ideally be pure.

As far as I remember constant buffers were not supposed to work that way. Again, I would need to review the code to understand what exactly is going on but it seems to me the idea is to allocate some stuff in advance so you can… use them as input parameters to CreateConstantBuffers(ID3D11Device* device)?

Ok, let's say that's it. And it puts the results in extern ID3D11Buffer* {ViewProj,WorldMatrix,LineRenderer,OrthographicProjection}Buffer, how does your user code know which to bind?

Those are not fixed properties. They are stuff coming out from data and vertex shader (canonically). What if you want to write a shader being slightly different and expecting buffers in a certain way instead of another?

Try to devise a method (not in the sense of code) which doesn't let responsability ‘escape’.

As far as I remember constant buffers were not supposed to work that way. Again, I would need to review the code to understand what exactly is going on but it seems to me the idea is to allocate some stuff in advance so you can… use them as input parameters to CreateConstantBuffers(ID3D11Device* device)?

Ok, let's say that's it. And it puts the results in extern ID3D11Buffer* {ViewProj,WorldMatrix,LineRenderer,OrthographicProjection}Buffer, how does your user code know which to bind?

It's a bit different than what you describe. These structs

	__declspec(align(16)) struct ProjectionVariables
	{
		DirectX::XMMATRIX View;
		DirectX::XMMATRIX Projection;
	};

	__declspec(align(16)) struct OrthographicProjectionVariables
	{
		DirectX::XMMATRIX ProjectionOrthographic;
	};

	__declspec(align(16)) struct WorldMatrices
	{
		DirectX::XMMATRIX World;
	};

	__declspec(align(16)) struct LineRendererVariables
	{
		DirectX::XMFLOAT3 CameraPosition;
		float  LineThickness;
	};

are not used as input parameters for CreateConstantBuffers(), they are used when I call UpdateSubresource() on the buffer. Like this:

void InitializeApp(...)
{
	...
	//this call just creates the actual ID3D11Buffers, it does not initialize them with any values
	ConstantBuffers::CreateConstantBuffers(device);
	
	//then bind the buffers to the model
	model->SetVSConstantBuffer(ConstantBuffers::ViewProjectionBuffer)
}

void UpdateApp(float dt)
{
	//lets say I want to update my camera buffer
	
	//I use one of the predefined struct above and fill in the required values
	ConstantBuffers::ProjectionVariables viewProjBuffer;
	viewProjBuffer.View = XMMatrixTranspose(mPlayer.GetCamera().GetViewMatrix());
	viewProjBuffer.Projection = XMMatrixTranspose(mPlayer.GetCamera().GetPerspectiveProjectionMatrix());
	
	//then just I load the actual buffer with those values
	mContext->UpdateSubresource(ConstantBuffers::ViewProjBuffer, 0, NULL, &viewProjBuffer, 0, 0);
}

As to how the user code knows which constant buffer to pass to the shader, that's something that has to be hardcoded. The user must know which buffers the shader expects set, what order the buffers are in in the shader etc. Is there any way to decouple this? I thought maybe like having Shader class for each shader that explicitly requires certain buffers to be set and takes care of the order, but then the user must still write that class to begin with, for every shader they write so it didn't make much sense.

Krohm said:

It is pretty obvious this wants to create a SomethingShader by returning it from outShader argument. This is common in the lower-level APIs because they return something already (HRESULT). Those functions return void. Just return the pointer. I'm not aware what the blobs are supposed to be.

Yes, that seems pretty sound. I can definitely change these to just return a pointer. The reason I had it like that is simply because the function is just a wrapper for

ID3D11Device::CreateVertexShader(const void* pShaderByteCode, SIZE_T byteCodeLength, ID3D11VertexShader** ppOutShader);

So I'd still have to pass a double pointer in any case, but yes it is safer to just return it. The blob is the shader bytecode that is generated when the shader is compiled by D3DCompileFromFile, which the function above requires to create the interface. I don't have any particular reason for returning the blob itself because I don't think I even need it besides to the call to create the shader. I just decided to include that option just in case the user ever needs to keep it. But yeah, I'll probably remove this also since I don't think it's ever needed again.

Krohm said:

I note there are no arguments mapping to the concept of ID3D11InputLayout for example. They seem to come from the extern identifiers/variables. This is shared state across calls. It is a well-known anti-pattern. Your calls should ideally be pure.

Yes, both the Shader namespace variables and constant buffer variables are global, however I thought this was acceptable in this case since they're not really “states”. As in the code doesn't modify them but simply uses them as pretty much constant variables that are created and initialized at startup. But yes, the constant buffers are global states, hopefully I'll be able to find a better solution after our discussion.

Thanks for the help so far, it's been immensely useful!

VanillaSnake21 said:

As to how the user code knows which constant buffer to pass to the shader, that's something that has to be hardcoded. The user must know which buffers the shader expects set, what order the buffers are in in the shader etc. Is there any way to decouple this?

Yes, but it is quite involved. As long as you are aware of the limits of the system you're working with I am fine with it.

VanillaSnake21 said:
The reason I went with per object approach is because it's so intuitive for me. I define individually what each object will look like by assigning a shader (see my answer above about how I handle shader creation) that will handle that particular look. The way I was thinking about it was like this: a shader is anologous to a material an object is made of and how that material behaves. So it was natural for me to kind of couple that with the actual object. It's a bit more unnatural for me to think of a material as like an effect that I apply first and then pass the objects, and then unapply the effect and render the next batch of objects. I was actually thinking of fixing the redundant states by some simple solution like grouping models by the shaders they use and keeping track if a shader was set already and just not setting it was set previously.

Yes it is no wonder you find it more intuitive that way. I doubt there's anybody who's first approach is not to go “per object”. However the intricacies of the hardware make it more complicated. Per material is the way to go as that is how you can get performance out of your pipeline. For example check out pseudocode for how the Unreal Engine 4.22 rendering does it. The per material strategy works:

foreach( camera )
{
    foreach( material )
    {
        set_shader();
        upload_material_parameters()
        foreach( instanced_objects with that material )
        {
            set_mesh();
            // already uploaded----upload_object_parameters()
            // no need to upload data, just tell gpu to render at some index
            render_instanced( instanced_objects.idx, instanced_objects.count ) 
        }
    }
}

… and if your cameras data have no direct dependency on the materials used then u can mod your render code like so:

	// pseudo
    foreach( material )
    {
        set_shader();
        upload_material_parameters()
 		
 		foreach( camera )
		{
       		foreach( instanced_objects with that material )
        	{
            	set_mesh();
            	// already uploaded----upload_object_parameters()
            	// no need to upload data, just tell gpu to render at some index
            	render_instanced( instanced_objects.idx, instanced_objects.count ) 
        	}
        }
    }
}

this form has the added benefit that u only set your materials shaders, etc… once for all cameras (however abovementioned pre-condition must apply)

Until then ?

@ddlox True. That's better.

you're welcome ?