Please review my level-loading code

Started by
3 comments, last by theOcelot 14 years, 8 months ago
Well, my old level-loading code was an awful mess, and I'm glad it's gone. This time, I tried to use better practices, but I'm not sure how well I did. So I wanted to get all your opinions. Keep in mind that all this is supporting what's effectively a legacy file format, the one that was written and read by my old, ugly code. I don't think it's too bad, but it's still not great. I'm open to suggestions, but it's not likely to change. It's a binary file format, which was written using the fstream::write'ing structs into the file. Here's the header that basically defines the file format.
#ifndef LEVELFILE_H_INCLUDED
#define LEVELFILE_H_INCLUDED

//File format is as follows:
//1. a level::file_header struct, with id field set to correct value.
//2. a series of offset_structs with length numLevels. These allow
//    random  access to each level without a ton of overhead
//3. after this, the actual level data commences. Each level consists of
//    a header (with valid id), followed by an array of mapCells, of length
//    header.width * header.height. Repeat as necessary

namespace level{
    //these are the valid id's for the file header and level header, totally arbitrary
    const int valid_file_id = 0xabcd;
    const int valid_id = 0x1234;
    //there is one of these for each tile in the level
    struct mapCell{
        char t_type;
        char t_arg;
        char s_type;
        char s_arg;
    };

    //there is one of these for each level
    struct header{
        int id;
        short width;
        short height;
    };

    //this is only at the beginning of the file
    struct file_header{
        int id;
        short numLevels;
        int fileSize;
    };

    //these tell the program where to find each level in the file
    struct offset_struct{
        int begin_offset;//from beginning of file
        int length;      //length of the level, including its header
    };
}

#endif
I created this class to ease the passage of level data to the places that actually need it. The original idea was for it to be completely independent.
#ifndef LEVELDATA_H
#define LEVELDATA_H

#include "levelfile.h"
#include <vector>

struct CreatureData{
    char type;
    char argument; //special initialization data from the file
    int xpos, ypos;
    CreatureData(char t, char a, int x, int y):type(t), argument(a), xpos(x), ypos(y){}
};

struct TileData{
    char type;
    //do we need pos?
    char argument;
    TileData(char t, char a):type(t), argument(a){}
};

class LevelData{
    public:
    const std::vector<CreatureData>& GetCreatures();
    const std::vector<TileData>&     GetTiles();
    int Width(){return width;}
    int Height(){return height;}
    LevelData();
    void LoadFromMemory(level::mapCell* begin, level::mapCell* end, int width_, int height_);
    private:
    bool loaded;
    int  width;
    int  height;
    std::vector<CreatureData> creatures;
    std::vector<TileData>     tiles;

};

//LevelData.cpp
#include "LevelData.h"


LevelData::LevelData():loaded(false){
}

const std::vector<CreatureData>& LevelData::GetCreatures(){return creatures;}
const std::vector<TileData>&     LevelData::GetTiles(){return tiles;}

void LevelData::LoadFromMemory(level::mapCell* begin, level::mapCell* end, int width_, int height_){
    //error checking
    if((end - begin) != width_*height_){
        //error
    }
    //prep
    if(loaded){
        creatures.clear();
        tiles.clear();
    }
    width = width_;
    height = height_;
    tiles.reserve(width*height);
    //foreach mapCell
    int xpos = 0; int ypos = 0;
    for(level::mapCell* it = begin; it != end; ++it){
        //
        tiles.push_back(TileData(it->t_type, it->t_arg));
        if(it->s_type){
            creatures.push_back(CreatureData(it->s_type, it->s_arg, xpos, ypos));
        }
        //move xpos, ypos
        xpos < width ? ++xpos: xpos = 0;
        ypos < height ? ++ypos: ypos = 0;
    }
    //done
}
#endif
The only thing I really regret about this class the dependency on levelfile.h, But I couldn't think of a better way to do it that preserves the way that the tile data is packed contiguously, except making LoadFromMemory templated on an Iterator, which would probably always just be a level::mapCell* anyway. Here's the workhorse. It actually does the loading of the files. An object that wants to load level files either instantiates or accepts one of these, and uses it to generate LevelData's. without further ado:
#ifndef LEVELFILELOADER_H
#define LEVELFILELOADER_H

#include <fstream>
#include "LevelData.h"
#include "levelfile.h"

class LevelFileLoader{
    public:
    LevelFileLoader();
    explicit LevelFileLoader(const std::string& filename);
    void LoadFile(const std::string& filename);
    void LoadLevel(int level_num, LevelData& data);
    int NumLevels();
    private:
    bool loaded;
    level::file_header file_header;
    std::vector<level::offset_struct> offset_structs;
    std::fstream file;
    //no copy, no assign
    LevelFileLoader(const LevelFileLoader&);
    void operator=(const LevelFileLoader&);
};

#endif

//LevelFileLoader.cpp
#include "LevelFileLoader.h"

LevelFileLoader::LevelFileLoader():loaded(false){}

LevelFileLoader::LevelFileLoader(const std::string& filename){
    LoadFile(filename);
}

int LevelFileLoader::NumLevels(){
    if(!loaded) return 0;
    return file_header.numLevels;
}

void LevelFileLoader::LoadFile(const std::string& filename){
    //(potentially) clear old data
    if(loaded){
        file.close();
        offset_structs.clear();
    }
    //setup
    file.open(filename.c_str());
    if(file.fail()){
        //error
    }
    //read the file header, check it
    file.read(reinterpret_cast<char*>(&file_header), sizeof(file_header));
    if(file.fail()){
        //error
    }
    if(file_header.id != level::valid_file_id){
        //error
    }
    //read the offset structs
    offset_structs.resize(file_header.numLevels);
    file.read(reinterpret_cast<char*>(&offset_structs[0]), sizeof(level::offset_struct)*file_header.numLevels);
    if(file.fail()){
        //error
    }
}

void LevelFileLoader::LoadLevel(int level_num, LevelData& data){
    //get the offset struct
    level::offset_struct& offset = offset_structs.at(level_num);
    //find the position of the level
    file.seekg(offset.begin_offset);
    //get the level header
    level::header header;
    file.read(reinterpret_cast<char*>(&header), sizeof(header));
    if(file.fail()){
        //error
    }
    if(header.id != level::valid_id){
        //error
    }
    //read the level data
    //I've heard you're supposed to use std::vectors for binary buffers, but is this how?
    std::vector<level::mapCell> raw_data;
    raw_data.resize(header.width * header.height);
    file.read(reinterpret_cast<char*>(&raw_data[0]), sizeof(level::mapCell)*header.width*header.height);
    if(file.fail()){
        //error
    }
    //stuff it in the LevelData
    data.LoadFromMemory(&raw_data[0], &raw_data[0] + header.width*header.height, header.width, header.height);
}
It's all working right now, but it hasn't been rigorously tested, partly because I just wouldn't know how. So do you have any ideas about how to do things better? Subtle bugs that will make my computer explode when I least expect it? How do I make it better? PS: would it be better to put this code on pastebin or something?
Advertisement
Wow, my code is perfect? Amazing.

Seriously, I'm just looking for the obvious stuff, that's not obvious to me because of my lack of experience. Even a brief lecture from lofouque about RAII would be welcome.
*bump*
Here are my two cents:

1) Remember that the compiler might add padding to struct or class to sacrifice memory for speed, however this padding might mess up the alignment stored in the file.

In order to ensure consistency, you can either ensure that the padding for each class or struct exists in with the file, or you can disable the padding:
// Tells the (Microsoft Visual C++) compiler to pack structs and classes as tightly as possible:#pragma pack( push, 1 )// // Structs...//// Restores the pack setting#pragma pack( pop )


2) To prevent yourself from accidentally misreading data from the file, you can use a helper function to do the reading for you:

// For all types:template<class Type>void ReadBinary( std::ifstream& in, Type& out ){	in.read( reinterpret_cast<char*>( out ), sizeof( Type ) );}// Specialize for std::string:template<>void ReadBinary( std::ifstream& in, std::string& out ){	unsigned len = 0;	ReadBinary<unsigned>( in, len );	// And stuff ...}


And use it like so:

	level::header header;	// file.read(reinterpret_cast<char*>(&header), sizeof(header));	ReadBinary<level::header>( file, header );


<a href=">

[Edited by - _fastcall on July 27, 2009 5:28:18 PM]
Thanks _fastcall.

1. I tried to remember padding. I made sure that the sizes of all the structs were multiples of 4, and having looked at the resulting files in a hex editor I'm pretty sure it's working right, but I realize that that's not a very substantial guarantee. ... Okay, it's not a guarantee at all. I'm actually using GCC, so I guess I'd need to look up the instructions for giving packing instructions.

Quote:paraphrased quote from _fastcall
In order to ensure consistency, you can ensure that the padding for each class or struct exists in with the file.


What exactly does that mean? And which of your options is demonstrated in the code snippet?

2. I've seen those before, but never thought to apply them here. That also might be a good place to stick the error handling. All that if(file.fail()){... stuff got on my nerves.

Should I also replace the builtin types with fixed-size ones from boost?

This topic is closed to new replies.

Advertisement