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

## Recommended Posts

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
//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

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
int id;
short width;
short height;
};

//this is only at the beginning of the file
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:
int  width;
int  height;
std::vector<CreatureData> creatures;
std::vector<TileData>     tiles;

};

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

}

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

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

public:
int NumLevels();
private:
std::vector<level::offset_struct> offset_structs;
std::fstream file;
//no copy, no assign
};

#endif

}

}

//(potentially) clear old data
file.close();
offset_structs.clear();
}
//setup
file.open(filename.c_str());
if(file.fail()){
//error
}
if(file.fail()){
//error
}
//error
}
if(file.fail()){
//error
}
}

//get the offset struct
level::offset_struct& offset = offset_structs.at(level_num);
//find the position of the level
file.seekg(offset.begin_offset);
if(file.fail()){
//error
}
//error
}
//I've heard you're supposed to use std::vectors for binary buffers, but is this how?
std::vector<level::mapCell> raw_data;
if(file.fail()){
//error
}
//stuff it in the LevelData
}

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?

##### Share on other sites
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*

##### Share on other sites
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 );

• 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 _fastcallIn 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?

• 18
• 11
• 16
• 9
• 49
• ### Forum Statistics

• Total Topics
631395
• Total Posts
2999780
×