# Criticise my D3D code please

This topic is 4224 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

## Recommended Posts

Further to my recent 2D in D3D thread: If anyone has a couple of minutes, would you mind having a quick glance at my (very simple) D3D8 wrapper. All it does at the moment is batch drawing white quads, with a view to adding textures etc later on to make a nice, simple sprite library. I'm after any pointers on using D3D and batching more efficiently (although any other criticism is, of course, always welcome). I am aware of the lack of solid error checking code at this stage. [EDIT] I've also now noticed, after two hours of swearing last night when I came to add textures, that I'm not setting a value to the rhw's in the CVertexBuffer::Add function either [smile]. draw.h
#ifndef draw_H
#define draw_H

#include <windows.h>
#include <sdk\d3d8.h>

struct CVertex
{
float x,y,z,rhw;
D3DCOLOR color;
};

class CVertexBuffer
{
private:
friend class CDevice;

IDirect3DVertexBuffer8 *Ptr;
IDirect3DIndexBuffer8 *Ind;

int Max,No; CVertex *Vtx;

public:
CVertexBuffer() : Ptr(0),Ind(0),Max(0),No(0),Vtx(0) { }

bool Acquire(int N);
bool Release();

void Begin();
void End();

bool Add(int X,int Y,int W,int H);
};

class CDevice
{
public:
CDevice();

bool Acquire(HWND Hw,int W,int H);
bool Release();

void BeginScene();
void EndScene();

void Clear(D3DCOLOR C);
void Present();

void Draw(CVertexBuffer &B);
};

#endif


draw.cpp
#include "draw.h"

#define sqrtf (float)sqrt // this is a workaround for Borland compiler
#include <sdk\d3dx8.h>

namespace
{

IDirect3DDevice8 *Dev=0;

void CreateParams(D3DPRESENT_PARAMETERS &P,HWND Hw,int W,int H);
void CreateIndices(IDirect3DIndexBuffer8 *Ind,int Max);

const DWORD D3DFVF_CVERTEX=(D3DFVF_XYZRHW|D3DFVF_DIFFUSE);

}

bool CVertexBuffer::Acquire(int N)
{
HRESULT R; Max=N*4;

R=Dev->CreateVertexBuffer(Max*sizeof(CVertex),
D3DUSAGE_DYNAMIC|D3DUSAGE_WRITEONLY,
D3DFVF_CVERTEX,D3DPOOL_DEFAULT,&Ptr);

if(FAILED(R)){ Ptr=0; return false; }

R=Dev->CreateIndexBuffer(Max*3,D3DUSAGE_WRITEONLY,D3DFMT_INDEX16,
D3DPOOL_MANAGED,&Ind);

if(FAILED(R)){ Ptr->Release(); Ptr=0; Ind=0; return false; }

CreateIndices(Ind,Max);

return true;
}

bool CVertexBuffer::Release()
{
if(Ptr){ Ptr->Release(); Ptr=0; }
if(Ind){ Ind->Release(); Ind=0; }

return true;
}

void CVertexBuffer::Begin()
{
}

void CVertexBuffer::End()
{
Ptr->Unlock();
}

bool CVertexBuffer::Add(int X,int Y,int W,int H)
{
if(No==Max) return false;

Vtx[No].color=D3DCOLOR_ARGB(255,255,255,255);
Vtx[No].x=X;
Vtx[No].y=Y;
Vtx[No].z=1.0f;

Vtx[No+1].color=D3DCOLOR_ARGB(255,255,255,255);
Vtx[No+1].x=X+W-1;
Vtx[No+1].y=Y;
Vtx[No+1].z=1.0f;

Vtx[No+2].color=D3DCOLOR_ARGB(255,255,255,255);
Vtx[No+2].x=X+W-1;
Vtx[No+2].y=Y+H-1;
Vtx[No+2].z=1.0f;

Vtx[No+3].color=D3DCOLOR_ARGB(255,255,255,255);
Vtx[No+3].x=X;
Vtx[No+3].y=Y+H-1;
Vtx[No+3].z=1.0f;

No+=4; return true;
}

CDevice::CDevice()
{
}

bool CDevice::Acquire(HWND Hw,int W,int H)
{
IDirect3D8 *D=Direct3DCreate8(D3D_SDK_VERSION); if(!D) return false;

D3DPRESENT_PARAMETERS Pm; CreateParams(Pm,Hw,W,H);

D3DCREATE_SOFTWARE_VERTEXPROCESSING,&Pm,&Dev);

D->Release(); if(FAILED(R) || !Dev) return false;

return true;
}

bool CDevice::Release()
{
if(Dev){ Dev->Release(); Dev=0; } return true;
}

void CDevice::BeginScene()
{
Dev->BeginScene();
}

void CDevice::EndScene()
{
Dev->EndScene();
}

void CDevice::Clear(D3DCOLOR C)
{
Dev->Clear(0,NULL,D3DCLEAR_TARGET,C,1.0f,0);
}

void CDevice::Present()
{
Dev->Present(NULL,NULL,NULL,NULL);
}

void CDevice::Draw(CVertexBuffer &B)
{
Dev->SetStreamSource(0,B.Ptr,sizeof(CVertex));
Dev->SetIndices(B.Ind,0);

Dev->DrawIndexedPrimitive(D3DPT_TRIANGLELIST,0,B.No,0,B.No/2);
}

namespace
{

void CreateParams(D3DPRESENT_PARAMETERS &P,HWND Hw,int W,int H)
{
P.BackBufferWidth=W;
P.BackBufferHeight=H;
P.BackBufferFormat=D3DFMT_A8R8G8B8;
P.BackBufferCount=1;

P.MultiSampleType=D3DMULTISAMPLE_NONE;

P.SwapEffect=D3DSWAPEFFECT_FLIP;
P.hDeviceWindow=Hw;
P.Windowed=false;
P.EnableAutoDepthStencil=false;
P.AutoDepthStencilFormat=D3DFMT_UNKNOWN;
P.Flags=0;

P.FullScreen_RefreshRateInHz=D3DPRESENT_RATE_DEFAULT;
P.FullScreen_PresentationInterval=D3DPRESENT_INTERVAL_DEFAULT;
}

void CreateIndices(IDirect3DIndexBuffer8 *Ind,int Max)
{
int I=0; short *Ptr=NULL;

Ind->Lock(0,Max*3,(UCHAR**)&Ptr,0);

for(int V=0;V<Max;V+=4)
{
Ptr=V;
Ptr[I+1]=V+2;
Ptr[I+2]=V+3;
Ptr[I+3]=V;
Ptr[I+4]=V+1;
Ptr[I+5]=V+2;

I+=6;
}

Ind->Unlock();
}

}


And used in the main loop a bit like (example only):
CDevice Dev;
CVertexBuffer Ver;

bool CEngine::OnAcquire()
{
if(!Dev.Acquire(Hw,Wt,Ht)) return false;
if(!Ver.Acquire(100)) return false;

return true;
}

void CEngine::OnCycle()
{
Dev.Clear(D3DCOLOR_ARGB(255,0,0,64));
Dev.BeginScene();

Ver.Begin();

for(int i=1;i<=20;++i)
{
}

Ver.End();

Dev.Draw(Ver);

Dev.EndScene();
Dev.Present();
}


This is based on various tutorials from this site and a lot of good advice from the forums. If anyone can see anyway to improve basic performance of the D3D bits of this, I'd appreciate you letting me know. Thanks in advance. Paul [Edited by - EasilyConfused on July 27, 2006 8:37:22 AM]

##### Share on other sites
I have some other points of criticism.

1.
CVertexBuffer has a friend class CDevice. This is bad OO design. I always remember the phrase: "A friend is someone who may touch your private parts". I don't know about you, but I'm VERY carefull with that :) Conclusion is that you shouldn't use a 'friend'.

2.
Quote:
 int Max,No; CVertex *Vtx;

Always write a single statement on a single line:

int Max, No;CVertex *Vtx;

3.
Quote:
 CVertexBuffer() : Ptr(0),Ind(0),Max(0),No(0),Vtx(0) { }

You point out the fact that you want to extand the code, then this is not a good way to declare the constructor. Just write out the constructor in the cpp file and keep the declaration in the header file clear of any variables.

4.
You are abusing the namespace keyword for what apear to be global functions. It's nicer to make a util class which hold these functions.

Please know that I only want to give constructive feedback.

Greetings,

Vincent

##### Share on other sites
the code looks fine other than the fact that CEngine contains D3D variables and is not cross platform.

it is also too neat.

you have really got to let your hair down a bit and let the vectors start flying

##### Share on other sites
Thanks for the input. I was really looking for some D3D advice to be honest but all criticism is welcome.

##### Share on other sites
Quote:
 Original post by CGameI have some other points of criticism.1.CVertexBuffer has a friend class CDevice. This is bad OO design. I always remember the phrase: "A friend is someone who may touch your private parts". I don't know about you, but I'm VERY carefull with that :) Conclusion is that you shouldn't use a 'friend'.

Friend classes are perfectly acceptable in some cases, and aren't really "bad OO design" when used properly, since they're still enforcing encapsulation at the macroscopic level. Especially in an API like D3D with assloads of circular dependencies, it's sometimes the only way to keep everything encapsulated in your wrapper.

Even C#, which is about as OO as you can get, has the "internal" keyword which is similar in concept (they don't behave exactly the same, they just both are used for keeping encapsulation).

Also, the way I heard it was "Only friends can touch your private member"

Anyway, as far as the actual code goes, it looks fine to me. I'd drop the whole "CClass" naming convention, but that's just personal preference (at least you're not using the "cClass" convention)

##### Share on other sites
Fair point about the CClass thing. People keep telling me this. I've been doing it so long now it looks "wrong" to me if I don't, but it is definately a habit I should break myself of.

Weirdly, I only feel the need to do it when I am windows programming. If I'm writing console programs, I do everything in lowercase and wouldn't dream of prefixing anything.

I guess it is a habit I picked up from Borland Builder development, with that whole TEverything thing.

##### Share on other sites
Quote:
 Original post by nsto119Friend classes are perfectly acceptable in some cases, and aren't really "bad OO design" when used properly, since they're still enforcing encapsulation at the macroscopic level. Especially in an API like D3D with assloads of circular dependencies, it's sometimes the only way to keep everything encapsulated in your wrapper.Anyway, as far as the actual code goes, it looks fine to me. I'd drop the whole "CClass" naming convention, but that's just personal preference (at least you're not using the "cClass" convention)

The use of the "friend" keyword indicates a hack. Sometimes hacks are necessary, and sometimes they're very useful, but they're always hacks.

I could tell you why "cClass" is superior to "CClass", but people who argue that these things are just personal preference usually have just mother-ducked onto whatever they first got used to.

##### Share on other sites
Quote:
Original post by JasonBlochowiak
Quote:
 Original post by nsto119Friend classes are perfectly acceptable in some cases, and aren't really "bad OO design" when used properly, since they're still enforcing encapsulation at the macroscopic level. Especially in an API like D3D with assloads of circular dependencies, it's sometimes the only way to keep everything encapsulated in your wrapper.Anyway, as far as the actual code goes, it looks fine to me. I'd drop the whole "CClass" naming convention, but that's just personal preference (at least you're not using the "cClass" convention)

The use of the "friend" keyword indicates a hack. Sometimes hacks are necessary, and sometimes they're very useful, but they're always hacks.

I could tell you why "cClass" is superior to "CClass", but people who argue that these things are just personal preference usually have just mother-ducked onto whatever they first got used to.

I'd actually be interested to hear it.

##### Share on other sites
Quote:
Original post by EasilyConfused
Quote:
 Original post by JasonBlochowiakI could tell you why "cClass" is superior to "CClass", but people who argue that these things are just personal preference usually have just mother-ducked onto whatever they first got used to.

I'd actually be interested to hear it.

Basically, the name is the most important thing most of the time. We (english speakers) are used to scanning left to right. The lower case letter "gets in the way" less when scanning l->r, letting you more easily focus on the class name. If you're interested in the prefix, you can "go back" to it.

##### Share on other sites
On the matter of friends from the C++ FAQ. Friends can break encapsulation (and frequently do) but not necessarily.

tj963

##### Share on other sites
Quote:
 Original post by Anonymous Poster[...]it is also too neat. you have really got to let your hair down a bit and let the vectors start flying

I agree with this. But it does depend from person to person on how much you want to balance cool code versus cool output.

An immediate improvement is to call CVertex something else. Perhaps CDefaultVertex. Also, the float x,y,z should be encapsulated within a vector of three floats, D3DXVECTOR3 or your own, possibly named "position". I say this because you'll eventually have more than one vertex type and possibly multiple positions in the programmable pipeline. But, I guess it's perfectly fine for an engine of this size.

##### Share on other sites
I would make some methods inline like the Begin or End for instance.

##### Share on other sites
@ dx elliot - thanks for the advice. I doubt this wrapper will ever be used much beyond experimentation but I will bear in mind your points about the vertex structure in the future.

Quote:
 Original post by dx elliotBut it does depend from person to person on how much you want to balance cool code versus cool output.

Bit confused by this. Where is the tradeoff, exactly?

##### Share on other sites
Quote:
 Original post by tj963On the matter of friends from the C++ FAQ. Friends can break encapsulation (and frequently do) but not necessarily.tj963

Ugh, their example is terrible - if you break a class into two parts, and then you make them friends so that they can get at each others' guts, then you haven't really broken the class into two parts. I would only deem that argument as acceptable during an early refactoring phase - something to be corrected in a subsequent pass. Otherwise, bad programmer - no cookie.

They do have a valid point w.r.t. friend being not worse then adding accessors for everything, but that's like saying that it's not much worse to be poked in the left eye versus the right eye.

##### Share on other sites
Quote:
 Original post by JasonBlochowiakThey do have a valid point w.r.t. friend being not worse then adding accessors for everything, but that's like saying that it's not much worse to be poked in the left eye versus the right eye.

Adding accessors to everything would allow anyone to access the private members, whereas only a friend could access otherwise non-accessible private members. One thing to watch out for, though, and this is an argument for friending being a hack, is that friends aren't inherited.

One roundabout, ugly way of doing a safer friend-like thing is to declare those members as protected, then have a subclass that has private accessor functions to those protected members, and friend someone to that class, who can then access that data only through the subclass's private accessors, which no one else can use. But, then again, you could accomplish the same thing by creating that sublass class privately inside what would have been the friend, and making the accessors public. I dunno, I'm just musing here, and don't know if any of this would work, or be worth doing.

##### Share on other sites
Well, you are all absolutely right and I agree that from a strict theoretical programming point of view my wrapper has broken some fundamental rules. Were this designed to be used by other people, or as library code, I would certainly consider a serious refactoring to address these issues.

Can I assume though, as per my original question, that the actual use of the D3D stuff is as efficient as is possible, since no-one has posted any criticism of that?

Sorry to keep badgering about this but I only started out with D3D a couple of weeks ago and I'm keen to get the early stages right.

Thanks.

##### Share on other sites
Quote:
 Original post by EasilyConfusedCan I assume though, as per my original question, that the actual use of the D3D stuff is as efficient as is possible, since no-one has posted any criticism of that?
I assumed somebody else would pick this up, but I guess not.

From a D3D standpoint, your wrapper negates many benefits of using VBs, with nothing in return. You force a specific vertex format, which is completely inappropriate for any but the most trivial apps. You force a function call to write each and every quad, which is a performance nightmare. You appear to provide no facilties whatsoever for handling device lost. And why are all VBs forced to be dynamic? The acquire and release functions should be part of the class constructors and destructors. There's no support for only modifying sections of a VB (particularly useful in combination with no-overwrite locks).

That's just at a quick glance. I don't see any of this code providing real benefits, just removing flexibility in an attempt at simplicity. If you want to make solid base classes, you're going to need to start over from design phase.

##### Share on other sites
Thanks promit. I really wanted someone to address this kind of thing.

To deal with these points in random order, in the hopes you or someone else can suggest some better ideas:

It was suggested to me elsewhere on gamedev that dynamic vertex buffers were the best solution for a 2D sprite library, since I would be regenerating all the vertex data every frame, which is also why I've used D3DLOCK_DISCARD. If I was mis-advised, I'd appreciate that advice being corrected.

I didn't consider that a function call for writing each quad would produce any kind of noticable overhead, since the calls are not locking and unlocking the buffer, but just writing to an already locked buffer. Could you explain the potential overhead here, which I assume from your post must be more than the normal overhead involved in a function call, since that is pretty negligable on a modern computer, and also perhaps suggest an alternative that would avoid the code that uses this wrapper having to manipulate the vertex data directly?

I am aware that this code is not checking for device lost, or even testing that the hardware supports the formats and capabilities I am using and had planned to add this later, but I appreciate you pointing this out.

I did not put the acquire and release code into constructors and destructors since this would force these actions to occur at the beginning and end of the scope of the device object, buffer object and so on, which was not what I wanted, but I suppose I could re-wrap all the objects into another class in the main engine that would place these actions in the appropriate place. I don't really see how this presents any performance issues though.

I have not attempted to add support for modifying sections of buffers since the strategy for drawing with a 2D sprite based game as I see it is generally to recreate ALL the vertex information each frame. I wanted to create an interface that presented the facility for a similar approach to using something like DirectDraw but would be glad to hear suggestions for how this could be improved, bearing in mind that there would be very little or no quads that would remain fixed from frame to frame.

This isn't meant to be a flexible, 3D-capable wrapper. The idea is to present an interface specifically for drawing 2D sprites with the flexibility of DirectDraw but with as little D3D performance overhead as possible within that flexibility.

With this in mind, could you advise on the disadvantage of having a specific vertex format, since I don't want to be able to draw anything but textured quads with this wrapper.

Any suggestions for improvement would be gratefully recieved.

Thanks. Paul

##### Share on other sites
Alright, so let's see how would be best to set this up, keeping your actual usage in mind.

First, CVertexBuffer -- not a good name. Doesn't really describe what this class does. (Also, ditch the capital C. It's pointless.) What we actually need is some kind of list of sprite objects. These sprites will all be combined into a buffer every frame and then rendered. For now, we'll keep it simple:
class Sprite{public:    int X, Y, Width, Height;};typedef std::vector<Sprite> SpriteList;typedef SpriteList::iterator SpriteIterator;

Now, we'll need some kinda object to keep a set of buffers cached.
struct Vertex{    float x, y, z;    D3DCOLOR diffuse;    float tu, tv;};class SpriteBuffer{private:    IDirect3DVertexBuffer9* m_Vb;    IDirect3DIndexBuffer9* m_Ib;    unsigned int m_SpriteCount;public:    SpriteBuffer( unsigned int maxSprites );    ~SpriteBuffer();    void Begin();    void End();    void Write( const SpriteList& spriteList );    void Clear();};

The exact functions you use may vary somewhat, and I leave the implementation up to you, but that should get the idea across.

##### Share on other sites
That is a very good idea that I appreciate you taking the time to post. My thanks.