DirectX Mesh Manager critique

112

Author

March 03, 2010 01:09 PM

OK, I'm in a stage of my schooling were I'm trying to figure out if I'm programming correctly. I can make programs work and run well; however, I sometime wonder if there is a better, more efficient way to program. So I have a request. In my Junior project, I worked with a group on developing a very simple graphics engine that handles 3dXMeshes. The game worked, but I've come to believe it may be inefficient. I've already decided I need to reprogram it, as there were a few features I couldn't get to work right (such as my shaders wouldn't work properly, so I removed the functionality all together.) I'm hoping the pros can tell me if I'm headed in the right direction with my mesh manager, and/or any thing I may need to consider that I didn't have in my mesh manager. Note: This class runs off of .x files. Note2: This file runs as a singleton and the class I have my directx in is a singleton as well ModelCtrl.h


#ifndef MODELS_H
#define MODELS_H

#include	<vector>
#include	<string>
#include	<deque>

#include "D3D9.h"
#include "D3DX9.h"

#include	"D3DApp.h"
#include	"Vertex.h" // contains my mesh vertex deffinitions
#include	"Camera.h"  // does exactly what it's called

using	std::vector;
using	std::pair;
using	std::string;
using	std::deque;

struct Mtrl
{
	Mtrl()
		:ambient(D3DXCOLOR(1.0f, 1.0f, 1.0f, 1.0f)), diffuse(D3DXCOLOR(1.0f, 1.0f, 1.0f, 1.0f)), spec(D3DXCOLOR(1.0f, 1.0f, 1.0f, 1.0f)), emmisive(D3DXCOLOR(1.0f, 1.0f, 1.0f, 1.0f)), specPower(8.0f){}
	Mtrl(const D3DXCOLOR& a, const D3DXCOLOR& d, 
		 const D3DXCOLOR& s, const D3DXCOLOR& e, float power)
		:ambient(a), diffuse(d), spec(s), emmisive(e), specPower(power){}

	D3DXCOLOR ambient;
	D3DXCOLOR diffuse;
	D3DXCOLOR spec;
	D3DXCOLOR emmisive;
	float specPower;
};

struct	MeshData{
	int	ID;
	int Parent;  //location of the mesh it originated from, and its materials
	ID3DXMesh*	mMesh;
	D3DXVECTOR3	mPosition;
	D3DXVECTOR3	mRotation;
	float		mScale;

	D3DXMATRIX	mtxPosition;
	D3DXMATRIX	mtxRotationX;
	D3DXMATRIX	mtxRotationY;
	D3DXMATRIX	mtxRotationZ;
	D3DXMATRIX	mtxScale;

	bool	Active;	//whether the model should/can be drawn

	MeshData(int pID, int pParent):mPosition(D3DXVECTOR3(0.0f,0.0f,0.0f)),mRotation(D3DXVECTOR3(0.0f,0.0f,0.0f)),mScale(1),ID(pID), Parent(pParent)
	{
	D3DXMatrixTranslation(&mtxPosition,0.0f,0.0f,0.0f);
	D3DXMatrixRotationX(&mtxRotationX, 0.0f);
	D3DXMatrixRotationY(&mtxRotationY, 0.0f);
	D3DXMatrixRotationZ(&mtxRotationZ, 0.0f);
	D3DXMatrixScaling(&mtxScale,1.0f,1.0f,1.0f);
	}
	~MeshData(){}

	D3DXMATRIX	CalcTransform(){return mtxRotationX * mtxRotationY * mtxRotationZ * mtxScale * mtxPosition;}
};

class ModelCtrl
{
private:
//This class is a singleton
	static	ModelCtrl*	instance;

//Models are loaded into originals for effecient copying later on
	vector<ID3DXMesh*>	Originals;

//Stores textures and material data for models
	vector<vector<Mtrl>*> Mtrls;
	vector<vector<D3DMATERIAL9>*> MtrlsAll;
	vector<vector<IDirect3DTexture9*>*>	modelTextures;

//Basic material for models without texture data
	IDirect3DTexture9*	Blank;

//Vector of all Mesh Instances
	vector<MeshData>	MeshInstances;

//queue of all empty MeshInstance data
	deque<int>	emptyIndxs;

//Handles color and light direction
	D3DXVECTOR3 mLightVecW;
	D3DXCOLOR   mAmbientMtrl;
	D3DXCOLOR   mAmbientLight;
	D3DXCOLOR   mDiffuseMtrl;
	D3DXCOLOR   mDiffuseLight;
	D3DXCOLOR   mSpecularMtrl;
	D3DXCOLOR   mSpecularLight;
	float       mSpecularPower;

//Points to a WVP matrix
	D3DXMATRIX*	mWVP;

	
	D3DMATERIAL9 mat;

private:
	ModelCtrl();
	~ModelCtrl();

	void	SetUpLight();

public:
	static	ModelCtrl*	Instance();
	static	void	Destroy();

//Loads Mesh into the originals, called only once for each model
//Location is file location relative to directory
//IdxPtris the Int variable that the indx loacation of the mesh will be stored to.
	bool	LoadMesh(string location, int& IdxPtr);

//Adds a Mesh to the world
//	Return id to new model
	int		AddMeshToWorld(int MeshIdx);

//Removes a Mesh from the active world, adding it's ID to an empty ModelData table (not incorporated yet)
//    ModelData is zeroed out for future use
	void	RemoveMeshFromWorld(int Idx);

//Sets a models draw status
	void	ToggleMesh(int id, bool pActive);
	bool	GetDrawStatus(int id);

//set model tranforms
	void	SetPosition(int id, D3DXVECTOR3 pPosition);
	void	SetRotation(int id, D3DXVECTOR3 pRotation);
	void	SetScale(int id, float pScale);

//set world/view/transform matrix
	void	SetWVP(D3DXMATRIX*	pWVP){mWVP = pWVP;}

//get transform of model 'id'
	D3DXVECTOR3	GetPosition(int id);
	D3DXVECTOR3	GetRotation(int id);
	float	GetScale(int id);

//Runs through and draws all Active model instances
	void	Draw();

	void	OnLostDevice();
	void	OnResetDevice();

//called externally and performs a check to make sure mesh data exists
	void	DrawMesh(int id);
//Called by the Draw() function which runs it's own check.
	void	DrawMesh(MeshData&	pData);

};

#define	MdlCtrl	ModelCtrl::Instance()

#endif

ModelCtrl.cpp


#include "ModelCtrl.h"

ModelCtrl*	ModelCtrl::instance = 0;

ModelCtrl*	ModelCtrl::Instance()
{
	if(instance == 0)
		instance = new ModelCtrl();
	return instance;
}

void	ModelCtrl::Destroy()
{
	delete instance;
	instance = 0;
}

void	ModelCtrl::OnLostDevice()
{
}
void	ModelCtrl::OnResetDevice()
{
}

ModelCtrl::ModelCtrl()
{
	SetUpLight();

	gd3dDevice->SetRenderState(D3DRS_AMBIENT, D3DCOLOR_XRGB(255,255,255));
	D3DLIGHT9 light;
	ZeroMemory(&light, sizeof(D3DLIGHT9));
	light.Type		= D3DLIGHT_DIRECTIONAL;
	light.Diffuse.r = 1.0f;
	light.Diffuse.g	= 1.0f;
	light.Diffuse.b	= 1.0f;
	light.Diffuse.a	= 1.0f;
	light.Range	= 1000.0f;

	D3DXVECTOR3 vecDir(0.0f,1.0f,1.0f);
	D3DXVec3Normalize((D3DXVECTOR3*)&light.Direction, &vecDir);

	gd3dDevice->SetLight(0,&light);
	gd3dDevice->LightEnable(0, TRUE);

	D3DXCreateTextureFromFileA(gd3dDevice, "blank.bmp",&Blank);

	ID3DXBuffer* errors = 0;
	if( errors )
		MessageBoxA(0, (char*)errors->GetBufferPointer(), 0, 0);

//This describes the direction the light is coming from
	mLightVecW     = D3DXVECTOR3(0.0, 0.0f, -1.0f);

//These variables determine the light color
	mDiffuseMtrl   = D3DXCOLOR(1.0f, 1.0f, 1.0f, 1.0f);
	mDiffuseLight  = D3DXCOLOR(1.0f, 1.0f, 1.0f, 1.0f);
	mAmbientMtrl   = D3DXCOLOR(1.0f, 1.0f, 1.0f, 1.0f);
	mAmbientLight  = D3DXCOLOR(1.0f, 0.6f, 0.6f, 1.0f);
	mSpecularMtrl  = D3DXCOLOR(0.8f, 0.8f, 0.8f, 1.0f);
	mSpecularLight = D3DXCOLOR(0.2f, 1.0f, 0.2f, 1.0f);
	mSpecularPower = 8.0f;
}

ModelCtrl::~ModelCtrl()
{
	for(int i = 0; i<(int)MeshInstances.size();i++)
	{
		if(MeshInstances.mMesh != 0)
			ReleaseCOM(MeshInstances.mMesh);
	}

	for(int i = 0; i<(int)Originals.size();i++)
	{
		ReleaseCOM(Originals);
	}

	for(int i= 0; i<(int)Mtrls.size();i++)
	{
		delete Mtrls;
	}

	for(int i = 0; i<(int)modelTextures.size();i++)
	{
		for(int c = 0; c<(int)modelTextures->size();c++)
			ReleaseCOM(modelTextures->at(c));
		if(modelTextures != 0)
			delete modelTextures;
	}

	for(int i = 0; i<(int)MtrlsAll.size();i++)
	{
		delete MtrlsAll;
	}
	ReleaseCOM(Blank);
}

bool	ModelCtrl::LoadMesh(string location, int &IdxPtr)
{
	IdxPtr = (int)Originals.size();
	ID3DXMesh*	meshSys	= 0;
	ID3DXBuffer*	adjBuffer	= 0;
	ID3DXBuffer*	mtrlBuffer	= 0;
	DWORD	numMtrls	= 0;

	D3DXLoadMeshFromXA(location.c_str(), D3DXMESH_SYSTEMMEM, gd3dDevice, &adjBuffer, &mtrlBuffer, 0, &numMtrls, &meshSys);

	D3DVERTEXELEMENT9	elems[MAX_FVF_DECL_SIZE];
	meshSys->GetDeclaration(elems);

	bool hasNormals = false;

	for(int i = 0; i<MAX_FVF_DECL_SIZE; i++)
	{
		if(elems.Stream == 0xff)
			break;
		if(elems.Type == D3DDECLTYPE_FLOAT3 && elems.Usage == D3DDECLUSAGE_NORMAL && elems.UsageIndex == 0)
		{
			hasNormals = true;
			break;
		}
	}

	D3DVERTEXELEMENT9 elements[64];
	UINT numElements = 0;
	VertexPNT::Decl->GetDeclaration(elements, &numElements);

	ID3DXMesh* temp = 0;

	meshSys->CloneMesh(D3DXMESH_SYSTEMMEM, elements, gd3dDevice, &temp);

	ReleaseCOM(meshSys);
	meshSys = temp;

	if(!hasNormals)
	{
		D3DXComputeNormals(meshSys, 0);
	}

	ID3DXMesh*	OptMesh;
	meshSys->Optimize(D3DXMESH_MANAGED | D3DXMESHOPT_COMPACT | D3DXMESHOPT_ATTRSORT | D3DXMESHOPT_VERTEXCACHE, (DWORD*)adjBuffer->GetBufferPointer(), 0, 0, 0, &OptMesh);

	Originals.push_back(OptMesh);
	ReleaseCOM(meshSys);

	//Now to load materials

	Mtrls.push_back(new vector<Mtrl>);
	modelTextures.push_back(new vector<IDirect3DTexture9*>);
	MtrlsAll.push_back(new vector<D3DMATERIAL9>);

	if( mtrlBuffer != 0 && numMtrls !=0)
	{
		D3DXMATERIAL*	d3dxmtrls = (D3DXMATERIAL*)mtrlBuffer->GetBufferPointer();

		for(DWORD i=0;i<numMtrls;i++)
		{
			Mtrl m;
			m.ambient	= d3dxmtrls.MatD3D.Ambient;
			m.diffuse	= d3dxmtrls.MatD3D.Diffuse;
			m.spec		= d3dxmtrls.MatD3D.Specular;
			m.specPower	= d3dxmtrls.MatD3D.Power;
			m.emmisive = d3dxmtrls.MatD3D.Emissive;
			Mtrls.back()->push_back(m);
			
			D3DMATERIAL9 temp;
			temp.Ambient = m.ambient;
			temp.Diffuse = m.diffuse;
			temp.Emissive = m.emmisive;
			temp.Power = m.specPower;
			temp.Specular = m.spec;
			MtrlsAll.back()->push_back(temp);

			if(d3dxmtrls.pTextureFilename != 0)
			{
				IDirect3DTexture9* tex = 0;
				char* texFN = d3dxmtrls.pTextureFilename;
				D3DXCreateTextureFromFileA(gd3dDevice, texFN, &tex);

				modelTextures.back()->push_back(tex);
			}else
			{
				modelTextures.back()->push_back(0);
			}
		}
	}

	ReleaseCOM(mtrlBuffer);
	ReleaseCOM(adjBuffer);

	return true;
}

int		ModelCtrl::AddMeshToWorld(int MeshIdx)
{
	if(!((MeshIdx >=0) && (MeshIdx < (int)Originals.size())))
		return -1;
	
	ID3DXMesh*	tempMesh;

	D3DVERTEXELEMENT9	elements[64];
	UINT	numElements = 0;
	VertexPNT::Decl->GetDeclaration(elements, &numElements);

	Originals[MeshIdx]->CloneMesh(D3DXMESH_SYSTEMMEM,elements,gd3dDevice,&tempMesh);

	if(emptyIndxs.empty())
	{
		int idx = (int)MeshInstances.size();
		MeshInstances.push_back(MeshData(idx, MeshIdx));
		MeshInstances.back().mMesh = tempMesh;

		return idx;
	}else{
		int idx = emptyIndxs.front();
		emptyIndxs.pop_front();

		MeshInstances[idx].mMesh = tempMesh;
		MeshInstances[idx].Parent = MeshIdx;

		return idx;
	}
	return 0;
}

void	ModelCtrl::RemoveMeshFromWorld(int Idx)
{
	//Make sure Idx is within scope and the MeshInstance has an active mesh
	if(!((Idx >= 0) && (Idx < (int) MeshInstances.size()) && (MeshInstances[Idx].mMesh != 0)))
		return;

	ReleaseCOM(MeshInstances[Idx].mMesh);
	MeshInstances[Idx].Active = false;

	emptyIndxs.push_back(Idx);
}

void	ModelCtrl::SetPosition(int id, D3DXVECTOR3 pPosition)
{
	MeshInstances[id].mPosition = pPosition;
	D3DXMatrixTranslation(&MeshInstances[id].mtxPosition,pPosition.x,pPosition.y,pPosition.z);
}
void	ModelCtrl::SetRotation(int id, D3DXVECTOR3 pRotation)
{
	MeshInstances[id].mRotation = pRotation;
	D3DXMatrixRotationX(&MeshInstances[id].mtxRotationX, -pRotation.x);
	D3DXMatrixRotationY(&MeshInstances[id].mtxRotationY, -pRotation.y);
	D3DXMatrixRotationZ(&MeshInstances[id].mtxRotationZ, pRotation.z);
}
void	ModelCtrl::SetScale(int id, float pScale)
{
	MeshInstances[id].mScale = pScale;
	D3DXMatrixScaling(&MeshInstances[id].mtxScale,pScale,pScale,pScale);
}
void	ModelCtrl::ToggleMesh(int id, bool pActive)
{
	MeshInstances[id].Active = pActive;
}

D3DXVECTOR3		ModelCtrl::GetPosition(int id){return MeshInstances[id].mPosition;}
D3DXVECTOR3		ModelCtrl::GetRotation(int id){return MeshInstances[id].mRotation;}
float			ModelCtrl::GetScale(int id){return MeshInstances[id].mScale;}
bool			ModelCtrl::GetDrawStatus(int id){return MeshInstances[id].Active;}


//modifies a material to make all of the texture appear without the use of light.
void	ModelCtrl::SetUpLight()
{
	
	// Set the RGBA for the Diffuse reflection
	mat.Diffuse.r = 0.0f;
	mat.Diffuse.g = 0.0f;
	mat.Diffuse.b = 0.0f;
	mat.Diffuse.a = 1.0f;

	// Set the RGBA for ambient reflection.
	mat.Ambient.r = 1.0f;
	mat.Ambient.g = 1.0f;
	mat.Ambient.b = 1.0f;
	mat.Ambient.a = 1.0f;

	// Set the color and sharpness of specular highlights.
	mat.Specular.r = 0.0f;
	mat.Specular.g = 0.0f;
	mat.Specular.b = 0.0f;
	mat.Specular.a = 0.0f;
	mat.Power = 50.0f;

	// Set the RGBA for emissive color.
	mat.Emissive.r = 1.0f;
	mat.Emissive.g = 1.0f;
	mat.Emissive.b = 1.0f;
	mat.Emissive.a = 0.0f;

}

void	ModelCtrl::Draw()
{
	//SetEffectVars();
	
	UINT numPasses = 0;
	//mEffect->Begin(&numPasses, 0);

	for(int i = (int)MeshInstances.size()-1;i>=0;i--)
	{
		if(MeshInstances.Active)
			DrawMesh(MeshInstances);
	}
	//mEffect->End();

	int t = 0;
}

void	ModelCtrl::DrawMesh(int id)
{
	DrawMesh(MeshInstances[id]);
}

void	ModelCtrl::DrawMesh(MeshData &pData)
{
	
	gd3dDevice->SetTransform(D3DTS_WORLD,&pData.CalcTransform());

	for(int j=0; j<(int)Mtrls[pData.Parent]->size(); j++)
	{
		
		if(modelTextures[pData.Parent]->at(j) !=0)
		{
			
			gd3dDevice->SetTexture(0,modelTextures[pData.Parent]->at(j));
			gd3dDevice->SetMaterial(&MtrlsAll[pData.Parent]->at(j));
		}else
		{
			gd3dDevice->SetTexture(0,Blank);
			gd3dDevice->SetMaterial(&mat);
			//set texture to default
		}

		pData.mMesh->DrawSubset(j);
	}
}

Any support is appreciated as I'm trying to better my skils before the time comes in December when I graduate.

jpetrie

13,220

March 03, 2010 01:57 PM

Quote:
I'm hoping the pros can tell me if I'm headed in the right direction with my mesh manager, and/or any thing I may need to consider that I didn't have in my mesh manager.

From the top:

Quote:
Note2: This file runs as a singleton and the class I have my directx in is a singleton as well

They don't need to be. See if you can refactor that away.

What's with your inconsistent whitespacing for preprocessor statements? What are "D3D9.h" and "D3DX9.h"? If they are supposed to be the standard D3D headers, you should be using the angle-bracket style of inclusion.

using directives in headers are bad news, they pollute the global namespace. Don't do that; fully-qualify your references to std::vector, et cetera.

Names like ModelCtrl and Mtrl seem annoyingly concise.

MeshData has a lot of public data, most of it seems redundant -- a position member, plus a transformation matrix? This is precisely what encapsulation is for. Store position, scale and rotation and compute the net transformation matrix on demand in CalcTransform.

The MeshData destructor is not needed as is, you can remove it. Although you may need it if you want to implement propery copying for MeshData objects (right now, they probably don't copy correctly).

Your member names are inconsistent as well (casing, et cetera).

You use a lot of bare pointers; you may want to consider boost::shared_ptr and/or CComPtr, both to handle raw memory ownership and to help with COM reference counting issues.

mWVP seems... weird. First, it's a pointer, pointing to somewhere unspecified. Second, models generally only need a world transform. View and projection come from elsewhere.

You ModelCtrl class looks like its managing textures as well, especially with that nasty looking vector-of-vectors. This seems to be conflating responsibilities.

LoadMesh should take its string by const reference. Having the user indicate the index to store the mesh is pretty ugly.

It's unclear what "AddMeshToWorld" is really for.

ToggleMesh() and such seems like it should just be forwarding to the public visibility bool on the mesh; no need for the method itself.

Overall, the interface of the "mesh manager" feels clunky and very poorly designed, from an OO perspective. There's lot of indirection through bare integers used as "handle" versus giving the client a rich interface (that looks like a mesh) to call on. There's a lot of functionality rolled into the interface that feels like it should be elsewhere -- on the mesh objects themselves, or in some kind of texture cache.

I didn't look at your implementation as a result, because I want to hear your responses to the discussion of your interface first.

Ncyphe

112

Author

March 03, 2010 02:50 PM

As I did mention, this was from a first time approach I used in my Junior Project.

Quote:Original post by jpetrie
Quote:
I'm hoping the pros can tell me if I'm headed in the right direction with my mesh manager, and/or any thing I may need to consider that I didn't have in my mesh manager.

From the top:

Quote:
Note2: This file runs as a singleton and the class I have my directx in is a singleton as well

They don't need to be. See if you can refactor that away.

What's with your inconsistent whitespacing for preprocessor statements? What are "D3D9.h" and "D3DX9.h"? If they are supposed to be the standard D3D headers, you should be using the angle-bracket style of inclusion.

Ah my white-spacing, I've been trying to develop a style so that when I declare includes, functions, and variables that I use Tabs instead of spaces, so that it looks more even. I'm still trying to fall into that habit and still have a few occurrences with the old habit.

Also, those are the D3D references. The book I was learning from used "" in their includes, not sure if I tried <> before. Will have to remember that next time.

Quote:
using directives in headers are bad news, they pollute the global namespace. Don't do that; fully-qualify your references to std::vector, et cetera.

So, instead of: "using namespace std::vector" I should simple created each variable like "std:vector<int> intVector"?

Quote:
Names like ModelCtrl and Mtrl seem annoyingly concise.

I take it that's a good thing?

Quote:
MeshData has a lot of public data, most of it seems redundant -- a position member, plus a transformation matrix? This is precisely what encapsulation is for. Store position, scale and rotation and compute the net transformation matrix on demand in CalcTransform.

I've had had this bad problem about worrying about he processor from past project. I had thought that what if I stored the rotation, position, and scale values for reference outside of the class, but only calculated the tranform matrices when needed I would be conserving CPU. Aparently it doesn't use that much CPU, as I originally thought.

Quote:
The MeshData destructor is not needed as is, you can remove it. Although you may need it if you want to implement propery copying for MeshData objects (right now, they probably don't copy correctly).

Oops, left a little garbage in there

Quote:
Your member names are inconsistent as well (casing, et cetera).

A habit I'm trying to build is to maintain consistency, that I've not yet fully built. This mostly shows the timeline of my program from due. Nice, consistent naming was from the early build, rushed naming comes from working the night before it was finally due.

Quote:
You use a lot of bare pointers; you may want to consider boost::shared_ptr and/or CComPtr, both to handle raw memory ownership and to help with COM reference counting issues.

I started reading about that somewhere. I'm now going to have to give that some more reading.

Quote:
mWVP seems... weird. First, it's a pointer, pointing to somewhere unspecified. Second, models generally only need a world transform. View and projection come from elsewhere.

Once again, another one of those cases where I thought I would be conserving CPU by merely passing a pointer to the WVP matrix, rather than passing it each time it was updated. Also, this is garbage left over from my failed shader attempt. I'm sure it had something to do with my camera class.

Quote:
You ModelCtrl class looks like its managing textures as well, especially with that nasty looking vector-of-vectors. This seems to be conflating responsibilities.

I forgot to mention, the project only used 3D and had no GUI, so I ended up implementing that at the last second into the manager (You can see my newbieness). I will be implementing a texture class this next time.

Quote:
LoadMesh should take its string by const reference. Having the user indicate the index to store the mesh is pretty ugly.

It's unclear what "AddMeshToWorld" is really for.

I had learned that copying meshes was a lot less processor intensive than reloading meshes, so I built the program to initially load a mesh to memory, and then copy the mesh to new memory for each instance needed of the mesh, that's what this function does. I'm sure I am doing my instancing wrong, one of the reasons I wanted a critique.

Quote:
ToggleMesh() and such seems like it should just be forwarding to the public visibility bool on the mesh; no need for the method itself.

Haven't heard anything about the "public visibility pool" I'll have to read up on that. What it did in my code was simply stored whether my DrawMesh function should be called for the model.

Quote:
Overall, the interface of the "mesh manager" feels clunky and very poorly designed, from an OO perspective. There's lot of indirection through bare integers used as "handle" versus giving the client a rich interface (that looks like a mesh) to call on. There's a lot of functionality rolled into the interface that feels like it should be elsewhere -- on the mesh objects themselves, or in some kind of texture cache.

I didn't look at your implementation as a result, because I want to hear your responses to the discussion of your interface first.

A far as interface, this was my first attempt at a model manager. It was for my Junior Project, in which we were split into 3 teams, Graphics, A.T., and Physics. I built this with what little knowledge I had to make it as easy as possible for other groups to use to implement their code into a final product.

(We made a space pirates vs traders simulation were pirates hunted for the wondering trader vessels, while dodging in and out of asteroid fields)

Thank you for your critique, it was very enlightening.

jpetrie

13,220

March 03, 2010 03:23 PM

Quote:
So, instead of: "using namespace std::vector" I should simple created each variable like "std:vector<int> intVector"?

Yes.

Quote:
Quote:
Names like ModelCtrl and Mtrl seem annoyingly concise.

I take it that's a good thing?

No. It's a bad thing. "Material" isn't that hard to type (especially with Intellisense/autocomplete features in modern code editors) and it's far more descriptive. "Mtrl" looks stupid.

Quote:
I've had had this bad problem about worrying about he processor from past project. I had thought that what if I stored the rotation, position, and scale values for reference outside of the class, but only calculated the tranform matrices when needed I would be conserving CPU. Aparently it doesn't use that much CPU, as I originally thought.
...
Once again, another one of those cases where I thought I would be conserving CPU by merely passing a pointer to the WVP matrix, rather than passing it each time it was updated.

These are micro-optimizations, and not worth concerning yourself with until you have a demonstrable performance problem -- performance is extremely fickle, especially at such a small scope (for example, you could just as easily be making your code slower by changing the size of the structure and impacting caching and paging).

Futhermore, many of these attempted "optimization" attempts are negatively impacting the integrity of the interface to your class. In other words, you're making the code more brittle. You should always focus on making code work, and work cleanly before you worry about making it fast.

Quote:
You ModelCtrl class looks like its managing textures as well, especially with that nasty looking vector-of-vectors. This seems to be conflating responsibilities.

I forgot to mention, the project only used 3D and had no GUI, so I ended up implementing that at the last second into the manager (You can see my newbieness). I will be implementing a texture class this next time.
I realize that class projects have time constraints, but real projects do as well. You really want to focus and refine your code-writing abilities such that things like consistency in symbol name selection are not impacted by deadline pressure. It is the equivalent to developing muscle memory in sports.

Quote:
I had learned that copying meshes was a lot less processor intensive than reloading meshes, so I built the program to initially load a mesh to memory, and then copy the mesh to new memory for each instance needed of the mesh, that's what this function does. I'm sure I am doing my instancing wrong, one of the reasons I wanted a critique.

Your implementation is too convoluted for me to want to really dig into. I think you need to read about passing by reference versus passing by value.

Quote:
Haven't heard anything about the "public visibility pool" I'll have to read up on that. What it did in my code was simply stored whether my DrawMesh function should be called for the model.

I'm referring, specifically, to "bool Active; //whether the model should/can be drawn" in the MeshData class. If you were passing around / manipulating actual mesh instances versus these opaque integers with no interfaces, ToggleMesh() would be unnecessary, because you could simply set active (or not) on the mesh interface.

Ncyphe

112

Author

March 03, 2010 09:37 PM

Thank you for your assistance.

I'm gonna take the information you pointed out to me and try to rewrite this in a more appropriate manner. I've found some example code from my Software Engineering book that I'm gonna use as a baseline to build a lot of my classes.

So, I guess I'll be back in a couple of weeks; hopefully with a much better piece of code.

Oh yeah, the one I posted is a garbled mess of code because I had only 4 weeks to actually program it with little to no knowledge of what I was doing.

DirectX Mesh Manager critique

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

DirectX Mesh Manager critique

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

Reticulating splines