level loading code causes error

Started by
10 comments, last by Diton9000 18 years, 10 months ago
My level loading code causes a windows error. Can you help?

//LevelLoad.h
#ifndef LEVEL_LOAD_H
#define LEVEL_LOAD_H

#include <fstream>
#include <string>
using namespace::std;

#define NORTH 1;
#define EAST 2;
#define SOUTH 3;
#define WEST 4;

#define ERROR 1;
#define OK 0;

class Node
{
public:
	string NorthFacingBMP, EastFacingBMP, SouthFacingBMP, WestFacingBMP;
	int NorthNode, EastNode, SouthNode, WestNode;
	void Draw(int direction);
};

class Level
{
public:
	Node *Map;
	ifstream levelFile;
	int numberOfNodes;
	int LoadLevel(const char *filename);
};

#endif



//levelload.cpp
#include "LevelLoad.h"
#include <fstream>
#include <string>
using namespace::std;

int Level::LoadLevel(const char *filename)
{
	levelFile.open(filename);
	levelFile >> numberOfNodes;
	if(levelFile.fail())
	{
		return ERROR;
	}
	Map = new Node[numberOfNodes]; 
	int counter = 0;
	while (counter <= numberOfNodes)
	{
		levelFile >> Map[counter].NorthFacingBMP;
		levelFile >> Map[counter].NorthNode;
		levelFile >> Map[counter].EastFacingBMP;
		levelFile >> Map[counter].EastNode;
		levelFile >> Map[counter].SouthFacingBMP;
		levelFile >> Map[counter].SouthNode;
		levelFile >> Map[counter].WestFacingBMP;
		levelFile >> Map[counter].WestNode;

		counter++;
	}
	return OK;
}


//main.cpp
#include "LevelLoad.h"




int main (/*int argc, char *argv[]*/)
{
	Level test;
	test.LoadLevel("test.txt");
	delete [] test.Map;
	return 0;
}

test.txt is this:

1
asdf.asdf
2
asfd
2
asdf
4
asdf
3

I hate signatures.
Advertisement
I would recommend using a for loop instead of that while
it looks like the problem is caused by the line

while (counter <
I would recommend using a for loop instead of that while
it looks like the problem is caused by the line

while (counter <= numberOfNodes)

try changing it to

while (counter < numberOfNodes)

because the counter increments to 1 and it tries to run through another iteration of the loop but it only creates an array with one element so Map[1] doesn't exist.
Thanks. That fixed it.
I hate signatures.
- Classes aren't supposed to behave the way that "Level" does. In particular, the "LoadLevel" looks like it really wants to be a constructor, and the class should handle deallocation of the Map itself - i.e. in a destructor. Anything less is asking for hard-to-find bugs later on - put code in the right places (encapsulation).

- The Level also doesn't need to hold on to that ifstream object, so that should just be a local in the constructor.

- For constant values, use actual constants. Better yet, use an enum for a set of possible values that logically create their own "type" (like "direction"). #define hackery is no longer required in C++ and takes on unnecessary risk.

- Checking the ifstream fail() *after* trying to read the number of nodes is a bit of a problem.

- As mentioned, use a for loop rather than a while loop where it is called for.

- "using namespace std;" shouldn't have the :: in it. I'm surprised that compiles. Anyway, using it in *header* files is generally considered a bad idea.

//LevelLoad.h#ifndef LEVEL_LOAD_H#define LEVEL_LOAD_H#include <iostream>#include <fstream>#include <string>#include <vector>enum direction { NORTH, EAST, SOUTH, WEST, NUM_DIRECTIONS };class Node {  // With proper design, we won't need these to be public.  std::string BMPNames[4];  int adjacentNodes[4];    public:  void Draw(direction d);  Node() {} // To be stored in a std::vector (keep reading), Nodes need to be  // default-constructible - thus we must explicitly define this ctor, since  // there is another one provided. The default-constructed objects won't  // really be useful, so we need to be careful to initialize every object  // that's needed (see Level constructor, and comments about making this an  // inner class).  Node(std::istream& in);};class Level {  // I will use the standard library class std::vector to hold the map.  // It automatically tracks the length of data storage, and behaves basically  // like a "resizable array" with minimal overhead - and also does memory  // management for us.  std::vector<Node> map;  // A helper function for loading.  void init(std::istream& in);  public:  // Note that we can't just do this with a temporary ifstream (like  // "init(ifstream(filename))", because temporaries need to be kept constant,  // and reading from a file stream changes that stream object.  Level(const char *filename) { std::ifstream in(filename); init(in); }  Level(std::istream& in) { init(in); }};#endif


//levelload.cpp#include "LevelLoad.h"using namespace std;void Node::Draw(direction d) {  // do something with BMPNames[d] and adjacentNodes[d]}Node::Node(istream& in) {  // In constructors, normally the best way to indicate a fatal error ("oops,  // not possible to construct an object after all") is to throw an exception.  // Here we want to do that if there's a problem with the input stream; we do  // that by telling the input stream object to treat any problems as  // exceptional, thus:  in.exceptions(ifstream::eofbit | ifstream::failbit | ifstream::badbit);  for (int i = 0; i < NUM_DIRECTIONS; i++) {    in >> BMPNames >> adjacentNodes;    // We can chain these actions the same way we would with printing to cout.    // Note that this is treating the items as separated by *any* whitespace;    // if you really want *line* at a time behaviour, investigate std::getline.  }}// A helper function for loading. The user will not call this directly, but// instead just use a constructor. The user has the option to open the file// himself, or pass a string literal and let that constructor open the file.void Level::init(istream& in) {  // As before, enforce stream validity.  // For our purposes we only really need to do this here, but I've done it  // in the Node class as well so that it can "stand on its own".  // Another option would be to make the Node an inner class of Level, since  // outside code probably shouldn't be mucking around with Nodes directly at  // all.  in.exceptions(ifstream::eofbit | ifstream::failbit | ifstream::badbit);  // Count the nodes (in a local variable!) and resize the vector accordingly  int nodeCount; in >> nodeCount;  map.resize(nodeCount);  // Read everything in  for (int i = 0; i < nodeCount; i++) {    map = Node(in);  }}


//main.cpp#include "LevelLoad.h"int main() {  Level test("test.txt"); // and it will clean itself up.}
Another windows error when I added sdl surfaces to the node class for each direction and try to load BMP's into them. The problem is, SDL_LoadBMP takes a const char *, and I read in strings. How do I fix this?

BTW, thanks for helping with the code. I will fix it soon, but I never learned how to use exceptions or vectors, so I have to look into that.
I hate signatures.
*bump*
I hate signatures.
Use std::string's c_str method. It returns a const char pointer to a null terminated string.

cheers
sam
I tried that. The code still causes an error.
I hate signatures.
Are the images actually there with the indicated path names?

Assuming it's nothing that simple, show the relevant code (new declaration of the Node, and the code that's attempting to load the surfaces).

This topic is closed to new replies.

Advertisement