8 replies to this topic

[davinci28]

Hi all, I'm working on a simple RPG for an intermediate programming course (Sophomore, college), and have hit a bit of a snag. Usually I'd ask the instructor, but I'm not sure he has experience with SDL.

I've been resorting to Lazy Foo's tutorials, though I've done this function on my own. My goal is to feed the function a string for the spritemap source file, then return a completed map. My approach is to store every tile to a vector of surfaces, then apply the tiles individually to the map surface based on what tile the map file indicates.

As far as I can tell, all the files are being opened correctly, and are in the proper directory.

Right now everything compiles fine, but the returned map is just white (from the BaseMap.bmp). This is my first attempt at using SDL, and I'd like to get the graphics down before moving on to the rest of the game mechanics.

[source lang="cpp"]// Compiles map surface from map file and spritesheetSDL_Surface *MCSFLoadMap(string source) { SDL_Surface *spriteSheet = NULL; spriteSheet = SDL_LoadBMP(source.c_str()); SDL_Surface *map = NULL; map = MCSFLoadImage("BaseMap.bmp"); vector<SDL_Surface*> tileVector; SDL_Surface *tempSurface; for (int i = 0; i < SPRITES_PER_ROW; i++) { for (int j = 0; j < SPRITES_PER_COLUMN; j++) { Sint16 x = i * TILE_WIDTH; Sint16 y = j * TILE_HEIGHT; SDL_Rect currentTile = {x, y, TILE_WIDTH, TILE_HEIGHT}; MCSFApplySurface(0, 0, spriteSheet, tempSurface, &currentTile); tileVector.push_back(tempSurface); } } cout << tileVector.size() << endl; ifstream mapFile; mapFile.open("map1.map"); if (mapFile.is_open()) { int currentTileID = 0; mapFile >> currentTileID; int iMax = MAP_WIDTH / TILE_WIDTH; int jMax = MAP_HEIGHT / TILE_HEIGHT; for (int i = 0; i < iMax; i++) { for (int j = 0; j < jMax; j++) { MCSFApplySurface((i * TILE_WIDTH), (j * TILE_HEIGHT), tileVector[currentTileID], map); mapFile >> currentTileID; } } } else { cout << "File cannot be opened" << endl; } mapFile.close(); SDL_FreeSurface(spriteSheet); return map;}[/source]

Thanks for your help!

[Servant of the Lord]

A few stylistic notes:

• Add whitespace to your code, it makes it more legible.
• Don't construct a variable with null, then initialize it in the very next line. Construct and initialize it on the same line. Similarly, you should construct and initialize 'mapFile' on the same line, thusly: ifstream mapFile("map1.map"); instead of needlessly spreading it over two lines.
• You don't need to explicitely call close() on mapFile, since it's destructor will take care of that. RAII is very important to understand.
• I get that 'i' is usually used for iterating, but when iterating over 2D, use 'x' and 'y', not 'i' and 'j' (alternatively, 'column' and 'row' are also acceptable), Variables names should describe their purpose.
• Be consistent with your bracketing. Your first set of for() loops are bracketed on the same line as the 'for' keyword. The second set of for() loops are bracketed at the start of the next line. Use one or the other, but be consistent. (Personally, I prefer the second method - having the brackets on a new line has several minor benefits - but it's mostly a matter of personal preference).

A few problem notes:

• I think your second set of for() loops should have 'mapFile >> currentTIleID' before the call to 'MCSFApplySurface()', and that you should remove the 'mapFile >> currentTileID' right after currentTileID is constructed. Otherwise, you'd have (iMax * jMax) + 1 calls to mapFile >> currentTIleID instead of just (iMax * jMax).
• On the line "MCSFApplySurface(0, 0, spriteSheet, tempSurface, ¤tTile);" you have a funky symbol in the final parameter.

Your actual issue:

But the biggest issue is... *dramatic drumroll*
You never constructed 'tempSurface', and 'tempSurface' is thus invalid. And you are drawing to an invalid surface that doesn't exist.
Worst still, 'tempSurface' is ironically the only surface you didn't initialize the NULL (and it was the only one you actually needed to).

The fact that it is all white is lucky - since you never initialized the surface to NULL, it can randomly crash your program (or worse *shudders* - it can randomly write to parts of your program's RAM in unexpected and very hard to track ways).

So, how do you actually create an empty SDL surface? I'll re-direct you to this thread, where another user was having the same problem.

The rule of thumb with pointer initialization to NULL:

• Immediately initialize your pointers to real data whenever possible. (You *don't* need to initialize them to NULL if you can immediantly initialize them to something else instead).
• If you need to construct your pointers but can't yet initialize them to real data, then you *must* initialize them to NULL, or face dire programming pitfalls that drive even experienced programmers crazy.

One final parting shot: I see you are using uniform initialization. Good for you! But that comes from C++11. Since you are using C++11, you actually shouldn't use NULL at all, and instead should use nullptr.

Hope that helped! If you have any questions, just ask.

Edited by Servant of the Lord, 12 November 2012 - 10:45 PM.

[davinci28]

Wow. Thanks for all that. I went and fixed the stylistic comments, as well as the extra call to mapFile >> currentTileID. The funky symbol was an error in copy/pasting the code here. I also added your CreateEmptySurface function you linked to. I see what you mean about trying to draw to a nonexistant surface, but I'm still just getting white when I run. I'll keep looking at my code, but thank you again for such a complete response!

Also, by setting tileVector[0] to a single tile bitmap that I map, it is properly displayed, so I guess that the lower half of the function is correct.

Edited by davinci28, 13 November 2012 - 08:49 AM.

[Servant of the Lord]

Post your new code, and I'll be happy to look at it again to see why it might still be giving white.

[davinci28]

Alright, I think I got it. I moved the tempSurface initialization from outside the nested for loops to right before the current sprite from the spriteSheet is applied to it. I'm not really sure what was happening, but after each tile was pushed onto the tileVector, every entry in the vector was that new tile. I changed the for loop limits to only get the 3 sprites (instead of the whole sprite sheet) and I got textures, then moving the initialization fixed the vector error.

[Servant of the Lord]

Yes, you'll need a *different* empty surface for each new surface you are creating,
Also, don't forget to free all those empty surfaces once you are done with them.

But a more important question is why are you drawing your entire map onto a single surface? Instead, you should draw the entire map (tile by tile) onto the screen each frame (this is the commonly used approach).

Imagine this: If you ever want your map to be 100 tiles wide and 100 tiles high, and your tiles are (let's say) 32 pixels, your entire map image would be 3200 by 3200 pixels in memory (almost 40 MB), instead of just the size of your tilesheet (just over 1 MB).

[davinci28]

I definitely see what you mean from a memory standpoint; that's what the Lazy Foo' tutorial said as well. It just struck me as an awfully processor-intensive task (relatively speaking) to make the map on the fly like that. Of course, it's also very possible (likley?) that I have an skewed view and it really is *way* better to do it on the fly.

I suppose that if I were to do it that way, I would just take my upper and lower bounds of the camera (what is displayed of the screen) and just apply the corresponding mapFile tiles directly to the screen, every cycle of the main game loop?

[Servant of the Lord]

Yes, except you wouldn't load the mapFile each time, you'd save the map data (the numbers) into a vector somewhere, and use that.

You are basically doing the same amount of drawing every frame anyway. What are you currently doing? Drawing the surface returned by 'MCSFLoadMap()' every frame right? So where's the difference between drawing the millions of pixels of one surface, to the millions of pixels split between multiple tiles? There's some minor calculations that need to be done per-tile, true, but they don't add up too much overhead.

The only real speedup you can do here is to not update the entire screen every frame, and only update the parts of the screen that have changed since last frame (called "dirty rectangles"). But that overly complicates things as the scale of your game increases, and is a premature (and often unnecessary) optimization.

[davinci28]

Ah, so the decrease in memory usage essentially negates any hit to performance. Well, I think I can figure things out from here then; thank you *very* much for your assistance, good sir!