Sign in to follow this  
Iderik

C++ Console-based game (help with the maps)

Recommended Posts

Iderik    142
Hello, I've made my first game few days ago, and now I rewrote the whole game and I got a problem med the maps. When I walk into a teleporter (wich change map and x,y pos) it won't load the correct map, something is mixed up in the loading part, but I don't know what, I've tried thousands of things. Here is the whole source code: http://pastebin.com/d17cce3e3 And the source only on the problem (wich I think it is): http://pastebin.com/m41dea2cf lvlMap - is what map to load (mapOne.txt or something) handleMap - will load the lvlMap; (mapOne.txt) from a .txt file and load it to an array Sorry if it's some swedish comments in the source.

Share this post


Link to post
Share on other sites
Wan    1366
In the main loop you check for a portal, which apparently can be identified with the magic number 2:
if (bMap[y][x] == 2)

Than in the mapTele function you are comparing the value bMap[y][x] with some other tile's value.
if (bMap[y][x] == bMap[ytTwoToOne][xtTwoToOne]) { ..

I'm don't quit follow what the value of other tile would mean, but regardless, two observations can already be made:

  • When arriving in mapTele, the value bMap[y][x] will always be two (you already checked that in the main loop).

  • Since the value will always be the same, there's no point to the if-else construct in the function: they'd be either all false, or all true (in which case it will only handle the first case).


Your code could use some cleaning up, especially if you ask others to review it.
  • Use switch statements instead of if-elses where appropriate

  • Try to replace the 'magic numbers' with well-named constants to show what they actually mean.

  • Refactor to remove some of the duplicate code.


There's probably more you can do the improve readability and flow control, but these are some basic cleanup mechanisms that don't immediately change the actual working of the code (well, perhaps the last one does).

And don't hesitate to post the code right here on the forum, using the [ source ] tags.

Share this post


Link to post
Share on other sites
jyk    2094
1. Does your map loading code always fail? Or only when you teleport?

2. Do you get the 'unable to read map file' message when you teleport?

3. Could you post one of your map text files?

4. You say something is mixed up. Could you be more specific? What happens exactly?

I can see a lot of places in your code where improvements could be made, but first I think we just need a clearer description of the problem (e.g. a description of the behavior, error messages [if any], and so on).

Share this post


Link to post
Share on other sites
Zahlman    1682
1) The whole point of having your map in a data file is to avoid having to hard-code where all the portals are and where they go to. There is something inconsistent here: you try to check if you're on a portal by the symbol (which came from the data file), but then try to check which portal you're on by the position (which is bad even if you had the code correct for it: what if you wanted the portals on two different maps to be at the same spot?)

One way around this is to change your map format a little. Use the numbers 0 through 2 to indicate portals: choose the one that corresponds to the map. Reassign other numbers for the other things on the map. When the player hits a portal (0 through 2), you can then use that number as an index into an array of "portal" structures (it needs to know the map file name to load, and where to put the player on the new map) and just use that data.

2) The map reading code has many problems (I've reformatted it here):


void handleMap() {
// Get map from .txt file and into an array
std::string dataFolder = "data\\maps\\";
dataFolder += lvlMap;
dataFolder += ".txt";
char getMap;
//int y = 0, x = 0;
std::ifstream readMap(dataFolder.c_str());
if (readMap.is_open()) {
for (int y = 0; y <= 20; ++y) {
for (int x = 0; x <= 60; ++x) {
readMap.get(getMap);
bMap[y][x] = atoi(&getMap); // int -> char och sedan lägger in i en array
}
}
readMap.close();
} else {
std::cout << "\n\n -> Unable to read map file\n"; // error meddelande
}
}



The problems:

1) Don't just comment out unused code; delete it. Otherwise it can only confuse the reader. If you need the code back, that's what version control is for.
2) Using atoi() is usually a bad idea: it's a throwback to old-fashioned C code. The C++ stream object already knows how to convert to integers.
3) Using atoi() is usually a bad idea: it doesn't provide proper error handling. If you get a zero back, you have no way to know if there was actually a zero in the file, or some garbage instead.
4) This particular use of atoi() is undefined behaviour, because you are trying to treat a single character as if it were a string. This doesn't work: you can point a char* at that char, but atoi() is looking for a null terminated string. The next char of memory, beside the one reserved for "getMap", isn't necessarily zero. It might not even belong to your program. It might not even exist.

If you don't have any spaces between the digits in the file, and you just want to treat the symbol '0' as zero, '1' as one, etc. through '9', then that is very easy to do: just subtract '0' from whatever char you read in. If you are treating each space on the map as an actual number, though (i.e. they could be multiple digits, and you put whitespace between them so that you can see which digits belong to each number), then you can read them in with the operator>> of the stream. (I will show the second approach.)

5) Give things clear names. Don't make up something vague and then explain it away in a comment. The person calling your function, or reading your function call, should not have to go look at the implementation of the function in order to understand it. The fact that you have to explain what "handleMap" does means that "handleMap" is not a good name for it. (On the other hand, going into detail about how you will load the map is not important.) Similarly, 'char getMap' is awkward.

6) Don't just print messages locally to "handle errors". Let the calling function decide what to do. This is more flexible in the end and doesn't really take any more effort.

7) Streams know how to clean themselves up. It is unusual to want or have to call .close() yourself.

8) Don't just use global variables to communicate. The whole idea of having a function to read a map, is that it can read any map. It makes sense to tell this function which map to load. Don't "ask" by looking at a global.

To do things properly, we should create a Map structure as a result (instead of reading into the global map array), and throw an exception on failure, but that's too much to show you all at once :) So for now, I'm just cleaning up the file name global; we still read into the map, and return a boolean to say whether or not everything went OK.

9) We don't need to use += step by step to build a string. The operator+ works perfectly well, too.


bool readMap(const std::string& mapName) {
std::string dataFolder = "data\\maps\\" + mapName + ".txt";
std::ifstream mapData(dataFolder.c_str());
// We don't need to check ahead of time if the file opened; if it didn't,
// the first read will fail. We simply return false if any read fails...
for (int y = 0; y <= 20; ++y) {
for (int x = 0; x <= 60; ++x) {
if (!(mapData >> bMap[y][x])) {
return false;
}
}
}
return true; // if we got here, it must have read everything.
}



There are, of course, similar issues with the rest of the code. I hope this gives you some ideas for what you can improve. Consider making some data structures, too, to bind together related things.

Also consider cleaning up your main loop: the way you check the environment is the same no matter which direction the player moves in, so there's no reason to repeat the code each time. Think of it as three steps: move the player; check if the new location is valid; restore the old position if it's not valid. To restore the old position, all you have to do is keep track of what it was. :)

Share this post


Link to post
Share on other sites
Iderik    142
Thanks for all the replys!
Ive got the code together so it works now.
instead of bMap[x][y] == bMap[ytOneToTwo][blabla] (i think it was)
it is now y == ytOneToTwo && x == xtOneToTwo;

WanMaster:
Yes, I use ytOneToTwo becous I could more then one teleports in mapOne.
Thank you so much for your help!

Jyk:
Yeah, I'll clean upp all shit now, I totally agree with you about the mess :)

Zahlman:
I've tried to make multiple magic numbers, I think I tried to put them into an vector, but didin't work so well because I haven't read that much about vectors.
I'm still a biiiig newbie in programming :) and it is soooo fun to code :D

Here is a screenshot from my game:
http://img518.imageshack.us/my.php?image=teaser123wj9.png

I will read thru your posts more later when I got the time to it.
Thank you so much again! I'm so glad for your reyplies!

Bless to all of you!

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this