Game with voxels

Question

I started a hobby project which would be the first thing I do in C that is intended to be seen by someone else. I've been practicing for a while but I'm actually a beginner; I never wrote real software in C/C++ yet.

I wanted to know what is OK from my code, what is wrong and why, if it's clear/readable, and everything you can tell me about it would be fine. I have never shown any of my code to anyone and I have no clue if I'm doing well, or if it can't be worse--the latter being what I consider most likely. So I would like to know the reasons from people with experience with C/C++, and what you consider that I can improve/remove, etc.

The code is something I've just started, about a small voxel system for a Minecraft-like game.

The basic idea would be:

1. to have an array of voxel types
2. to be able to assign rules to the voxel types (like functions/scripts to be called.. somehow?)
3. to have an array of game relevant states for each voxel. The state data would be mostly lighting and visibility values.
4. to be able to save and load state data as necessary

etc... I have a lot of ideas but I still barely wrote some code and I would like to know other people's opinions before continuing, so I can improve the project instead of having to fix it later.

This is the link for the project files (only a .h and a .cpp, it's just started) at google code

And here are some of the contents of the .h so you can give yourself an idea without opening the link:

typedef struct _GVOXEL_RULE
{
        GVOXELRULE_ID   nRuleID;
        INT32                   nRuleValue; // this value is left for use by the rule function, not needed right now
} GVOXEL_RULE;

// here I'm planning to store the description of the voxel types
typedef struct _GVOXEL_DATA
{
        GVOXEL_TYPE             nVoxelType;     
        dword                   nRuleCount;
        GVOXEL_RULE*    pVoxelRules; // I'm thinking this should be separated to a "voxel rule manager" class or something similar
        dword                   nVoxelCount; // store here the amount of voxels applying this reference (so we stop looking when the limit is reached)
} GVOXEL_DATA;

typedef struct _GVOXEL_STATE_1_0
{
        GVOXEL_ID       nVoxelID;       // This id will be equal to the voxel index which may be handy
        GVOXEL_TYPE     nVoxelType;     // This number will reference the table with voxel descriptions
        word            nMetadata;      // This was described below.
        byte            nLightX;        // This stores how much light (from 0 to 255) is receiving the face pointing to +X 
        byte            nLightXN;       // Same from below but for -X face
        byte            nLightY;        // etc.
        byte            nLightYN;       // etc..
        byte            nLightZ;        // etc...
        byte            nLightZN;       // 
} GVOXEL_STATE_1_0, GVOXEL_STATE;

typedef struct _GVOXELCHUNK_DATA_1_0 // 
{
    volatile long       nRefCount; // <-- This gives me the chills
    dword               nChunkID;           // some chunk ID that makes sense in some other reference table. or not.
    dword               nWidth;             // size of the array in the X dimension
    dword               nHeight;            // size of the array in the Y dimension
    dword               nDepth;             // size of the array in the Z dimension
    GVOXEL_STATE_1_0*   pVoxelStateList;    // The count of voxel states will be nWidth*nHeight*nDepth
    dword               nExtendedDataSize;  // size of optional data, if found in the file, to be loaded to the "pExtendedData" pointer.
    void*               pExtendedData;      // If bExtendedData is 0, this variable should be set to 0 (null).
} GVOXELCHUNK_DATA_1_0, GVOXELCHUNK_DATA;

void gvCreateChunkData10( GVOXELCHUNK_DATA_1_0** ppChunkData );
void gvAcquireChunkData10( GVOXELCHUNK_DATA_1_0* pChunkData );
void gvFreeChunkData10( GVOXELCHUNK_DATA_1_0** ppChunkData );

#define gvCreateChunkData               gvCreateChunkData10 
#define gvFreeChunkData                 gvFreeChunkData10       
#define gvAcquireChunkData              gvAcquireChunkData10

// wchar_t* pFilename: Name of the source file to load chunk data from
// dword *nMaxChunks: if ppChunkData is NULL, the function returns here the number of chunks in the file. else stores the
// GVOXELCHUNK_DATA** ppChunkData: 
INT32 gvLoadChunkFromFileW( wchar_t* pFilename, dword *inout_nMaxChunks, GVOXELCHUNK_DATA** ppChunkData );
//
INT32 gvSaveChunkToFileW( wchar_t* pFilename, dword nChunkCount, GVOXELCHUNK_DATA** ppChunkData );
#ifdef _TODO
INT32 gvLoadChunkFromFileA( char* pFilename, dword nMaxChunks, GVOXELCHUNK_DATA** ppChunkData );
INT32 gvSaveChunkToFileA( char* pFilename, dword nChunkCount, GVOXELCHUNK_DATA** ppChunkData );
#endif

#define gvLoadChunkFromFile     gvLoadChunkFromFileW
#define gvSaveChunkToFile       gvSaveChunkToFileW  

Answer 1

Design

It looks like you have a design in mind, but it isn't all visible from the code posted. I'm anyway a little hazy on what you're really trying to model here, so let's step through it.

1. 3D array of voxel instances: this is your actual world state, got it.
2. nVoxelType is ... an index into some type array not shown? Well, we clearly need some connection between a voxel instance and it's type, however implemented
3. now, what properties does a voxel type need? You show an array of rules, but it isn't clear how they interact with the type. If you're intending that each rule should represent something like hit a voxel of type 7 with a pickaxe or set fire to a voxel of type 3, I'm not sure a linear array will cut it.

It might help to clarify things if you try sketching a game event: going from coordinates to a voxel instance is easy, but once you have that, what are you going to do to it?

Implementation

You say you probably want C rather than C++, but you can use similar polymorphism in either with a little effort, so designing the interaction is still more important than language choice.

Eg. C++

class VoxelType
{
public:
    // common interface for all types
    virtual void hitMeWithAPickaxe(Voxel *self) = 0;
    virtual void setFireToMe(Voxel *self) = 0;
};

class DirtVoxelType: public VoxelType
{
public:
    // specific implementation for this type
    void hitMeWithAPickaxe(Voxel *self);
    virtual void setFireToMe(Voxel *self);
};

or in C:

struct VoxelType {
    int type;
    void (*hitMeWithAPickaxe)(struct Voxel *self);
    void (*setFireToMe)(struct Voxel *self);
};

void hitDirtWithAPickaxe(struct Voxel *);
void setFireToDirt(struct Voxel *);

struct VoxelType VoxelTypeArray[] = {
  { DirtType, hitDirtWithAPickaxe, setFireToDirt },
  ...
};

Obviously you can also use the time-honoured massive switch/case statement in either language too - the point is just that the selection of C or C++ isn't the driving force behind the design, the design is the driving force behind the implementation.

volatile long nRefCount; // <-- This gives me the chills I don't blame you - even if you already know your game is going to be multithreaded, I can't see this working. Either design concurrency and synchronisation in up-front, and do it properly, or omit it entirely and be prepared to do some re-design work later if one thread really isn't good enough.

Style

In approximate order of increasing subjectivity

What is up with the #define gvSaveChunkToFile gvSaveChunkToFileW stuff? Is this intended to ease some cross-platform stuff later, or did you just copy it from elsewhere? IMO it's ugly and potentially confusing, so I'd remove it unless it adds something really worthwhile I can't see here.

Likewise with all the GVOXEL_STATE_1_0, GVOXEL_STATE stuff - unless you're really planning to have, and deal with, multiple incompatible versions of the same structure at runtime (which sounds horrible), you can isolate any backwards-compatibility logic to the (de)serialization layer and remove the duplicate names in your main code.

You're using lots of (what I think are) platform-specific typedefs, in case you care. Maybe byte, dword etc. are idiomatic on that platform so there is a big consistency benefit, but I'm not clear what other advantage they have over the standard types like char, int8_t or uint8_t.

Subjectively I'm not a big fan of Hungarian notation as used here: you're just replicating the type and not adding any semantic information.

Finally, and again subjectively, I don't like ALL_UPPER_CASE type names or _LEADING_UNDERSCORE_UPPER_CASE symbols, they get perilously close to the __RESERVED_NAMES and just tend to look ugly. I also don't see the need to typedef every struct; struct VoxelState * instead of GVOXEL_STATE_1_0 * seems clearer and easier to read