Critique/debug a renderstate manager?

Started by
8 comments, last by MaulingMonkey 18 years, 7 months ago
This is the first time I've posted a largeish amount of source, it's my complete renderstate manager for Direct3D. (~200 lines). It is causing memory leaks/errors (see this thread) but I don't currently have time to sieve through it. I plan to release a fixed version to the community, so if anyone is a bit handy about basic STL could you have a quick look. Basically PushRenderStateBlock and PushTextureStageStateBlock are the same, as are PopRenderStateBlock and PopTextureStageStateBlock so looking at either should suffice. Here's the source - I recommend cut'n'paste into your IDE for easier reading.
#pragma once
#pragma warning( disable : 4786)

/*RenderStateManager.h - John Dexter - 21/6/2005

  This class implements functionality to do 2 things:
  1)Maintain the D3D render & texture stage states in a reliable way
  2)Reduce the number of D3D Renderstate and TextureStagestate changes
*/

#define SIZE_D3DRS 174
#define SIZE_D3DTSS 29
#define NUM_TEXTURE_STAGES 8
//#define NOT_SET 0xffffffff

/*#ifdef _DEBUG
	#define RENDER_STATE_MANAGER_DBG
#else #ifdef RENDER_STATE_MANAGER_DBG
	#undef RENDER_STATE_MANAGER_DBG
#endif
*/
#include <vector>
#include <d3d.h>
#include "class logger.h"

class RenderState
{
public:
	D3DRENDERSTATETYPE m_State;
	DWORD m_Value;
	RenderState(D3DRENDERSTATETYPE state,DWORD value) : m_State(state),m_Value(value)
	{}
};
class TextureStageState
{
public:
	DWORD m_Stage;
	D3DTEXTURESTAGESTATETYPE m_State;
	DWORD m_Value;
	TextureStageState(DWORD stage,D3DTEXTURESTAGESTATETYPE state,DWORD value) : m_Stage(stage),m_State(state),m_Value(value)
	{}
};

typedef std::vector<RenderState> RenderStateVector;
typedef std::vector<TextureStageState> TextureStageStateVector;

class RenderStateManager
{
private:
	IDirect3DDevice8 *m_Device;
	//The D3D Device to set the states on
	DWORD m_RenderStates[SIZE_D3DRS];
	//the current set of D3D render states which are required
	DWORD m_TextureStageStates[NUM_TEXTURE_STAGES][SIZE_D3DTSS];
	//the current set of D3D texture stage states which are required for each stage

	std::vector<RenderStateVector> m_RenderStateStack;
	//the render states which have been pushed onto the stack
	std::vector<TextureStageStateVector> m_TextureStageStateStack;
	//the TextureStage states which have been pushed onto the stack
#ifdef RENDER_STATE_MANAGER_DBG
	unsigned m_NumRenderStateCalls,m_NumRenderRedundantStateCalls,m_NumTextureStageStateCalls,m_NumTextureStageRedundantStateCalls;
	unsigned m_NumConsecutiveRenderPushes,m_NumConsecutiveTextureStagePushes;
#endif
public:
	RenderStateManager(IDirect3DDevice8 *device) : m_Device(device)
	{
		int i,j;
		for(i=0; i<SIZE_D3DRS; ++i)
			m_Device->GetRenderState((D3DRENDERSTATETYPE)i,&m_RenderStates);
		
		for(i=0; i<SIZE_D3DTSS; ++i)
		for(j=0;j<NUM_TEXTURE_STAGES; ++j)
			m_Device->GetTextureStageState(j,(D3DTEXTURESTAGESTATETYPE)i,&m_TextureStageStates[j]);
	}
	//initialise with defaults
	
	~RenderStateManager(){}

/*	pass in a set of states and their desired values. The current values for these
	set of states are recorded, and the new values applied */
	void PushRenderStateBlock(const RenderStateVector &block)
	{
		RenderStateVector currentState;
		//build a list of the current values for each state which is to change
		for(RenderStateVector::const_iterator I=block.begin();I!=block.end();++I)
		{
			const RenderState &rs = *I;
			if(rs.m_Value != m_RenderStates[rs.m_State])
			{
				currentState.push_back(RenderState(rs.m_State,m_RenderStates[rs.m_State]));
				//set and record the new value for this state
				m_RenderStates[rs.m_State]=rs.m_Value;
				m_Device->SetRenderState(rs.m_State,rs.m_Value);
			}
		#ifdef RENDER_STATE_MANAGER_DBG
			else
				++m_NumRenderRedundantStateCalls;
			++m_NumRenderStateCalls;
		#endif
		}
		//store the previous settings to restore later
		if(!currentState.empty())
			m_RenderStateStack.push_back(currentState);

	#ifdef RENDER_STATE_MANAGER_DBG
		++m_NumConsecutiveRenderPushes;
		if(m_NumConsecutiveRenderPushes > 1)
		{
			sprintf(Logger::buffer,"RenderStateManager::PushRenderStateBlock : %d consecutive pushes",m_NumConsecutiveRenderPushes);
			Logger::OutputLine();
		}
	#endif
	}

/*	gets the last set of states set, and restores the previous values */
	void PopRenderStateBlock()
	{
		if(m_RenderStateStack.empty())
			return;
		RenderStateVector &popState = m_RenderStateStack.back();
		for(RenderStateVector::iterator I=popState.begin();I!=popState.end();++I)
		{
			RenderState &rs = *I;
			//set and record the new value for this state
			m_RenderStates[rs.m_State]=rs.m_Value;
			m_Device->SetRenderState(rs.m_State,rs.m_Value);
		}
		m_RenderStateStack.pop_back();
	#ifdef RENDER_STATE_MANAGER_DBG
		m_NumConsecutiveRenderPushes = 0;
	#endif
	}


/*	pass in a set of stage states and their desired values. The current values for these
	set of states are recorded, and the new values applied */
	void PushTextureStageStateBlock(const TextureStageStateVector &block)
	{
		TextureStageStateVector currentState;
		//build a list of the current values for each state which is to change
		for(TextureStageStateVector::const_iterator I=block.begin();I!=block.end();++I)
		{
			const TextureStageState &tss = *I;
			if(tss.m_Value != m_TextureStageStates[tss.m_Stage][tss.m_State])
			{
				currentState.push_back(TextureStageState(tss.m_Stage,tss.m_State,m_TextureStageStates[tss.m_Stage][tss.m_State]));
				//set and record the new value for this state
				m_TextureStageStates[tss.m_Stage][tss.m_State]=tss.m_Value;
				m_Device->SetTextureStageState(tss.m_Stage,tss.m_State,tss.m_Value);
			}
		#ifdef RENDER_STATE_MANAGER_DBG
			else
				++m_NumTextureStageRedundantStateCalls;
			++m_NumTextureStageStateCalls;
		#endif
		}
		//store the previous settings to restore later
		if(!currentState.empty())
			m_TextureStageStateStack.push_back(currentState);
	#ifdef RENDER_STATE_MANAGER_DBG
		++m_NumConsecutiveTextureStagePushes;
		if(m_NumConsecutiveTextureStagePushes > 1)
		{
			sprintf(Logger::buffer,"RenderStateManager::PushTextureStageStateBlock : %d consecutive pushes",
				m_NumConsecutiveTextureStagePushes);
			Logger::OutputLine();
		}
	#endif
	}

/*	gets the last set of states set, and restores the previous values */
	void PopTextureStageStateBlock()
	{
		if(m_TextureStageStateStack.empty())
			return;
		TextureStageStateVector &popState = m_TextureStageStateStack.back();
		for(TextureStageStateVector::iterator I=popState.begin();I!=popState.end();++I)
		{
			TextureStageState &tss = *I;
			//set and record the new value for this state
			m_TextureStageStates[tss.m_Stage][tss.m_State]=tss.m_Value;
			m_Device->SetTextureStageState(tss.m_Stage,tss.m_State,tss.m_Value);
		}
		m_TextureStageStateStack.pop_back();
	#ifdef RENDER_STATE_MANAGER_DBG
		m_NumConsecutiveTextureStagePushes = 0;
	#endif
	}


	void BeginScene()
	{
#ifdef RENDER_STATE_MANAGER_DBG
		m_NumRenderStateCalls=0;
		m_NumRenderRedundantStateCalls=0;
		m_NumTextureStageStateCalls=0;
		m_NumTextureStageRedundantStateCalls=0;
		m_NumConsecutiveRenderPushes = 0;
		m_NumConsecutiveTextureStagePushes = 0;
#endif
	}
	void EndScene()
	{
#ifdef RENDER_STATE_MANAGER_DBG
//		sprintf(Logger::buffer,"RenderStateManager::EndScene : Render %u/%u, TextureStage %u/%u",
//				m_NumRenderRedundantStateCalls,m_NumRenderStateCalls,m_NumTextureStageRedundantStateCalls,m_NumTextureStageStateCalls);
//		Logger::OutputLine();
#endif
	}
};

Advertisement
It seems like nobody wants to know about this. Thought I'd bump it and give it another go. Is there a better way to go about posting code - I'd normally expect at least a couple of people to enjoy publicising my errors and bugs?!
Prehaps we're mostly too busy sieveing through our own code to sieve through those of others?

Nothing obviously inheritly wrong with the code except the total lack of assert checks when accessing arrays via POD data indicies. It's not suprising you're running into problems.

A few lines which could benifit...:

In PushRenderStateBlock: if(rs.m_Value != m_RenderStates[rs.m_State]) //rs is PODesque, rs.m_State could be anything - unchecked before this point
Before, add: assert( rs.m_State < SIZE_D3DRS ); //we'll find out if this is a problem.

In PopRenderStateBlock: m_RenderStates[rs.m_State]=rs.m_Value; //rs is PODesque, rs.m_State could be anything - unchecked before this point
Before, add: assert( rs.m_State < SIZE_D3DRS ); //we'll find out if this is a problem.

In PushTextureStageStateBlock: if(tss.m_Value != m_TextureStageStates[tss.m_Stage][tss.m_State]) //tss is PODesque, tss.[m_Stage/m_State] could be anything - unchecked before this point
Before, add: assert( tss.m_Stage < NUM_TEXTURE_STAGES );
Before, add: assert( tss.m_State < SIZE_D3DTSS );

In PopTextureStageStateBlock: m_TextureStageStates[tss.m_Stage][tss.m_State]=tss.m_Value; //tss is PODesque, tss.[m_Stage/m_State] could be anything - unchecked before this point
Before, add: assert( tss.m_Stage < NUM_TEXTURE_STAGES );
Before, add: assert( tss.m_State < SIZE_D3DTSS );

Chances are, you've been having buffer overflow issues exactly like SiCrane said in the first post of your other thread. The best way to catch these is to use assert() judiciously throughout your code. This may be as simple as O-B-ONE.
O-B-ONE being "Overflow by one", right?

So you reckon I'm trying to access an element in a vector which doesn't exist? Doesn't std::vector throw exceptions in such circumstances, or is it very unsafe?

Thanks for taking the time to wade through it by the way.
std::vector only performs bounds checking if you use the at(size_t) function.
Quote:Original post by d000hg
O-B-ONE being "Overflow by one", right?

Yep. Or Off By ONE.
Quote:So you reckon I'm trying to access an element in a vector which doesn't exist? Doesn't std::vector throw exceptions in such circumstances, or is it very unsafe?

When you use operator[], it will attempt to directly access the element without any safety checks. In this regard it is like a normal array - fast, but prone to misuse. There is a member function called at which will throw in such a circumstance. To take an example line, you would switch:

if(rs.m_Value != m_RenderStates[rs.m_State])

Into:

if(rs.m_Value != m_RenderStates.at( rs.m_State ))

..assuming that m_RenderStates was a std::vector.

However, both it and m_TextureSageStates are normal arrays, which don't have this function. These are the only elements I saw an unchecked or unbounded random access to, so no, I'm not saying you're trying to access an element in a vector which doesn't exist - I'm saying you're trying to access an element in a plain array which doesn't exist. The same basic issue applies either way, however.

boost::array has a member function which behaves the same way, you may be interested in checking it out. AFAIK both throw std::range_error when you attempt to access out of bounds data.

[Edited by - MaulingMonkey on September 6, 2005 2:26:56 PM]
Well I put in those asserts, but they never got triggered. However I can't get it to crash at this location any more so it's a bit hard to tell!

I am getting it crashing out sometimes elsewhere sometimes though. I'll have something like:
Camera *pCamera = new Camera;
But every now and then if I run it through the debugger it'll crash and tell me that pCamera=0x00000000 when I access it. Puzzling since it is only rarely, and apparently only when run through the debugger. This happens in a couple of places with different things - it must be a sysematic problem but I just don't know where to look!
Necro post, of doom! Realized I hadn't followed up on this thread.

Quote:Original post by d000hg
Well I put in those asserts, but they never got triggered. However I can't get it to crash at this location any more so it's a bit hard to tell!


I'm going to guess it's highly unlikely that your bug is here then. With the asserts I can be fairly confident of that.

Quote:I am getting it crashing out sometimes elsewhere sometimes though. I'll have something like:
Camera *pCamera = new Camera;
But every now and then if I run it through the debugger it'll crash and tell me that pCamera=0x00000000 when I access it. Puzzling since it is only rarely, and apparently only when run through the debugger. This happens in a couple of places with different things - it must be a sysematic problem but I just don't know where to look!


So far, you've shown me a large section of code, and although not well safeguarded, I failed to see other problems within it. I'm guessing the rest of your code is probably similarly badly safeguarded.

With only a vauge descripion of the current situation and bugs, all I can really suggest is learning to use assert() (and I mean when to use it as well, not just "assert( true )"), trying to plug any potential buffer overflows... and to possibly look higher up on the debug backtrace.


It may be worth attempting to focus on the next "higher up" section of code - e.g. does a section of code use (even indirectly) both the function with that camera as well as these classes? They're both obvious graphics related... a problem high up isn't allways noticable until you reach the down low section, if you get my drift.
Yeah I see what you're getting at.
I've never really got into ASSERT, probably because it's not defined by default for non-MFC projects. I suppose I work on the thinking that a memory error will cause a crash immediately for me to debug - which evidently isn't the case.

I don't recall if you replied to it, but I created a newer thread on my problems in which I raised the opinion that this is a multithreaded issue. All my projects use the single-threaded project settings (for standard libraries I guess) so I'll have to try changing that! But when I stopped using a thread to do stuff, the problems went away completely.
Would you know if I must make ALL my projects set to use MT - I have quite a few which generate .libs - or just the one which generates the .exe? I use the .lib projects in other applications where I don't need to MT-enable them.
Quote:Original post by d000hg
Yeah I see what you're getting at.
I've never really got into ASSERT, probably because it's not defined by default for non-MFC projects. I suppose I work on the thinking that a memory error will cause a crash immediately for me to debug - which evidently isn't the case.


Nope, if the prolem is small enough it may never even crash at all. #include <assert.h> to get the macro (or <cassert> for C++, although as a macro it won't be namespaced).

Quote:I don't recall if you replied to it, but I created a newer thread on my problems in which I raised the opinion that this is a multithreaded issue. All my projects use the single-threaded project settings (for standard libraries I guess) so I'll have to try changing that! But when I stopped using a thread to do stuff, the problems went away completely.


Hmm, didn't notice it...

Quote:Would you know if I must make ALL my projects set to use MT - I have quite a few which generate .libs - or just the one which generates the .exe? I use the .lib projects in other applications where I don't need to MT-enable them.


I'd try making them consistant - I know you run into major allocation issues if you mix MT and non-MT DLLs and EXEs, as it causes multiple versions of the CRT to be linked under VC++ compilers which in turn causes multiple free stores to be allocated - causing problems whenever memory is alloc()/new/new[]ed in one and free()/delete/delete[]ed in another.

At worst, it can't hurt.

This topic is closed to new replies.

Advertisement