Criticise my D3D code please

Started by
18 comments, last by Aardvajk 17 years, 8 months ago
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()
{
    No=0; Ptr->Lock(0,Max*sizeof(CVertex),(UCHAR**)&Vtx,D3DLOCK_DISCARD);
}

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);

    HRESULT R=D->CreateDevice(D3DADAPTER_DEFAULT,D3DDEVTYPE_HAL,Hw,
                              D3DCREATE_SOFTWARE_VERTEXPROCESSING,&Pm,&Dev);

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

    Dev->SetVertexShader(D3DFVF_CVERTEX);

    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.Add(i*20,i*20,i*10,i*10);
        }

    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]
Advertisement
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
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
Thanks for the input. I was really looking for some D3D advice to be honest but all criticism is welcome.
Quote:Original post by CGame
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'.


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)
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.
Quote:Original post by nsto119

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.

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.
Quote:Original post by JasonBlochowiak
Quote:Original post by nsto119

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.

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.
Quote:Original post by EasilyConfused
Quote:Original post by JasonBlochowiak

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.


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.
On the matter of friends from the C++ FAQ. Friends can break encapsulation (and frequently do) but not necessarily.

tj963
tj963

This topic is closed to new replies.

Advertisement