Math wrong and a memory leak?
Matthew Dalrymple

I've posted this similar code a month or so ago, I'm back working on it. This time I am using OpenLayer.

I am trying to create a 2D tiling engine. The tiles are not tiling correctly and there must be a memory leak because my processor is using 100% right now >.< (It's making typing hard and I don't want to restart until I finish this)
First here are the resources I am using and a screen shot:
Output:
{"name":"errorns2.png","src":"\/\/djungxnpq2nug.cloudfront.net\/image\/cache\/b\/b\/bbc6fb5e4cd202edcafe68147998ffd0.png","w":626,"h":512,"tn":"\/\/djungxnpq2nug.cloudfront.net\/image\/cache\/b\/b\/bbc6fb5e4cd202edcafe68147998ffd0"}errorns2.png
tile.bmp
http://img227.imageshack.us/img227/4897/tilefq0.png
Here is majority of the code:
FYI: m_* Variables are members of that class
main.cpp

1#include "stdafx.hpp"
2 
3int main()
4{
5 Setup::SetupProgram(true, true, true);
6 Setup::SetupScreen(screenWIDTH, screenHEIGHT, WINDOWED);
7 set_window_title("moooooo");
8 int map[16][21] = {
9 {1, 0, 3, 1, 3, 4, 1, 9, 1, 3, 2, 0, 4, 5, 8, 3, 2, 0, 4, 5, 9 },
10 {3, 0, 3, 1, 3, 4, 1, 9, 1, 3, 2, 0, 4, 5, 8, 3, 2, 0, 4, 5, 9 },
11 {1, 0, 3, 1, 3, 4, 1, 9, 1, 3, 2, 0, 4, 5, 8, 3, 2, 0, 4, 5, 9 },
12 {1, 0, 3, 1, 3, 4, 1, 9, 1, 3, 2, 0, 4, 5, 8, 3, 2, 0, 4, 5, 9 },
13 {1, 0, 3, 1, 3, 4, 1, 9, 1, 3, 2, 0, 4, 5, 8, 3, 2, 0, 4, 5, 9 },
14 {1, 0, 3, 1, 3, 4, 1, 9, 1, 3, 2, 0, 4, 5, 8, 3, 2, 0, 4, 5, 9 },
15 {1, 0, 3, 1, 3, 4, 1, 9, 1, 3, 2, 0, 4, 5, 8, 3, 2, 0, 4, 5, 9 },
16 {1, 0, 3, 1, 3, 4, 1, 9, 1, 3, 2, 0, 4, 5, 8, 3, 2, 0, 4, 5, 9 },
17 {1, 0, 3, 1, 3, 4, 1, 9, 1, 3, 2, 10, 4, 5, 8, 3, 2, 0, 4, 5, 9 },
18 {1, 0, 3, 1, 3, 4, 1, 9, 1, 3, 2, 0, 4, 5, 8, 3, 2, 0, 4, 5, 9 },
19 {1, 0, 3, 1, 3, 4, 1, 9, 1, 3, 2, 0, 4, 5, 8, 3, 2, 0, 4, 5, 9 },
20 {1, 0, 3, 1, 3, 4, 1, 9, 1, 3, 2, 0, 4, 5, 8, 3, 2, 0, 4, 5, 9 },
21 {1, 0, 3, 1, 3, 4, 1, 9, 1, 3, 2, 0, 4, 5, 8, 3, 2, 0, 4, 5, 9 },
22 {1, 0, 3, 1, 3, 4, 1, 9, 1, 3, 2, 0, 4, 5, 8, 3, 2, 0, 4, 5, 9 },
23 {1, 0, 3, 1, 3, 4, 1, 9, 1, 3, 2, 0, 4, 5, 8, 3, 2, 0, 4, 5, 9 },
24 {4, 0, 3, 1, 3, 4, 1, 9, 1, 3, 2, 0, 4, 5, 8, 3, 2, 0, 4, 5, 9 }
25 };
26 cTile firsttiles;
27 firsttiles.LoadTileSet("tile.bmp", 32, 32);
28 cMap firstmap;
29 firstmap.SetTileSet(&firsttiles);
30 firstmap.LoadMap((int *)&map, 21, 16, 1);
31 while(!key[KEY_ESC])
32 {
33 firstmap.Draw(0, 0, 1);
34 Canvas::Refresh();
35 }
36 firsttiles.Release();
37
38 return 0;
39}
40END_OF_MAIN()

Tile.cpp

1#include "stdafx.hpp"
2 
3cTile::cTile()
4{
5 
6}
7 
8bool cTile::LoadTileSet(char* filename, int TileWidth, int TileHeight)
9{
10 m_Filename = filename;
11 if((m_TileWidth = TileWidth) <= 0 || (m_TileHeight = TileHeight) <= 0)
12 return false;
13 if(!m_TileSet.Load(filename))
14 return false;
15 m_Cols = m_TileSet.Width() / TileWidth;
16 m_Rows = m_TileSet.Height() / TileHeight;
17
18 m_TileHeight = TileHeight;
19 m_TileWidth = TileWidth;
20 return true;
21}
22 
23bool cTile::DrawTile(int TileNum, float Xpos, float Ypos)
24{
25 float tileXpos = 0;
26 float tileYpos = 0;
27 
28 tileYpos = ((TileNum / m_Cols) * m_TileHeight);
29 tileXpos = ((TileNum % m_Cols) * m_TileWidth);
30
31 m_TileSet.Blit(Xpos, Ypos, Clipped(tileXpos, tileYpos, m_TileWidth, m_TileHeight));
32 return true;
33}
34 
35int cTile::GetTileWidth()
36{
37 return m_TileWidth;
38}
39 
40int cTile::GetTileHeight()
41{
42 return m_TileHeight;
43}
44 
45bool cTile::Release()
46{
47 m_TileSet.Destroy();
48 return true;
49}

Map.cpp

1#include "stdafx.hpp"
2 
3cMap::cMap()
4{
5 TilesAreSet = false;
6 Loaded = false;
7}
8 
9bool cMap::LoadMap(int* Map, int Width, int Height, int NumLayers)
10{
11 if((m_Map = Map) == NULL)
12 return false;
13 m_Width = Width;
14 m_Height = Height;
15 m_NumLayers = NumLayers;
16 Loaded = true;
17 return true;
18}
19 
20bool cMap::SetTileSet(cTile* tiles)
21{
22 if((m_Tiles = tiles) == NULL)
23 return false;
24 TilesAreSet = true;
25 return true;
26}
27 
28bool cMap::Draw(int Xpos, int Ypos, int Layer)
29{
30 // Called at the right time?
31 if(!TilesAreSet || !Loaded)
32 return false;
33 // How many Rows and Columns fit on the screen?
34 int SRows = 0;
35 int SCols = 0;
36 SCols = screenWIDTH / m_Tiles->GetTileWidth();
37 SRows = screenHEIGHT / m_Tiles->GetTileHeight();
38
39 int Column = Xpos;
40 int Row = Ypos;
41
42 int Xpixel = 0;
43 int Ypixel = 0;
44
45 int TileNumber = 0;
46 
47 while(Row < m_Height && Ypixel < screenHEIGHT)
48 {
49 while(Column < m_Width && Xpixel < screenWIDTH)
50 {
51 TileNumber = m_Map[(Row + Xpos) * m_Width + Column + Ypos];
52 m_Tiles->DrawTile(TileNumber, (float)Xpixel, (float)Ypixel);
53 Column++;
54 Xpixel += m_Tiles->GetTileWidth();
55 }
56 Xpixel = 0;
57 Column = 0;
58 Ypixel += m_Tiles->GetTileHeight();
59 Row++;
60 }
61 return true;
62}

Simon Parzer

firstmap.LoadMap((int *)&map, 21, 16, 1);
IMO this should be
firstmap.LoadMap((int *)map, 21, 16, 1);
as an array already represents a pointer.
But I'm not exactly sure what you want to do there.

TileNumber = m_Map[(Row + Xpos) * m_Width + Column + Ypos];

I don't get that. What should (Row + Xpos) be? I think you mean (Row + Ypos). At the end you add Ypos where you probably meant Xpos.
Anyway, even if you swap Xpos and Ypos it doesn't make much sense, because you already added Xpos and Ypos to Row and Column before...

int Column     = Xpos;
int Row        = Ypos;

So, either make Column = 0; Row = 0; at the beginning and say (Row + Ypos) and Column + Xpos at the "get TileNumber" line or do the opposite in each case. But if you do both you will obviously run out of bounds.

Matthew Dalrymple

firstmap.LoadMap((int *)&map, 21, 16, 1); I've switched it between those before and it had no effect via compile time but I switched it back to how you thought it should be.

int Column     = Xpos;
int Row        = Ypos;

This sets it so when you call cMap::Draw() You can set the topleft coords to draw. Normally this would be 0,0 But the map can be bigger than the tiles you can display on the screen so we are going to want to change that position a lot. With a map 100x100tiles you are going to want to draw the rest of the map sometimes :-D. In my example program Column and Row are set to 0.

Does anyone know of a way to get Two-Dimensional data out of a One-Dimensional array?

Steve Terry
Quote:

Does anyone know of a way to get Two-Dimensional data out of a One-Dimensional array?

Sure bitmaps really are a single dimensional array, to get the x and y on a bitmap you can do some simple math:
bit_array[x + y * width];
which will give you x and y coordinates from a single dimensional array.

Matthew Dalrymple

Still hasn't solved my problem :-( I've narrowed it down though. Something is going wrong with the inputs to cTile::Draw(). Its inputing some huge number as the TileNumber. So it must be something wrong with my loop inside of cMap::Draw();

Simon Parzer
Quote:

int Column     = Xpos;
int Row        = Ypos;

This sets it so when you call cMap::Draw() You can set the topleft coords to draw. Normally this would be 0,0 But the map can be bigger than the tiles you can display on the screen so we are going to want to change that position a lot. With a map 100x100tiles you are going to want to draw the rest of the map sometimes :-D. In my example program Column and Row are set to 0.

Yeah, I know. I fully understand your code. But you add Xpos and Ypos twice!
First time:

int Column     = Xpos;
int Row        = Ypos;

Second time:
(I assume that you have fixed the Xpos/Ypos error there)
TileNumber = m_Map[(Row + Ypos) * m_Width + Column + Xpos];
So, try to figure. Let's say Xpos is 20.
int Column = Xpos
so Column is 20.
TileNumber = m_Map[(Row + Ypos) * m_Width + Column + Xpos];
What do you think Column + Xpos is? 20 + 20 = 40. So you have 40 instead of 20.

I know it isn't the error you currently have, because Xpos and Ypos are 0 in your test case. You really should run your map::Draw function through a debugger. And maybe you should use std::vector instead of two-dimensional arrays as soon as you have fixed your problem.

Indeterminatus

I know this is not related to your problem, I just felt like pointing it out.

bool cTile::LoadTileSet(char* filename, int TileWidth, int TileHeight)
{
     m_Filename          = filename;
     if((m_TileWidth = TileWidth) <= 0 || (m_TileHeight = TileHeight) <= 0)
          return false;
     ...

In these few lines of code, I can spot some issues that might be problematic. Let's have a closer look, shall we? if((m_TileWidth = TileWidth) <= 0: Making use of side-effects never is a good idea. Why? Because it makes the code harder to read, and thus also harder to understand. Let's assume this method is called with 0 for TileWidth and some other value for TileHeight (not important). LoadTileSet will return false as expected, only that the internal state of this cTile instance now is really messed up. m_TileHeight wasn't assigned the value we passed, so it is probably uninitialized. m_TileWidth is 0, even though this is an incorrect value. m_Filename will hold the value of filename, even though the loading failed.

     m_TileHeight   = TileHeight;
     m_TileWidth    = TileWidth;

These lines are absolutely unnecessary if you keep the assignments in the previous if clause (which I advise you not to!).

My point is, you make it hard for anyone to determine the exact footprint (i.e., postcondition if you prefer that term) for all possible input data. I'm guessing you are not overriding or implementing LoadTileSet, so for starters you could declare both TileHeight and TileWidth as unsigned int. This rules out negative numbers and makes that fact also obvious on first sight.

Keeping the state of the object clear might work like this:

1bool cTile::LoadTileSet(const char* filename, unsigned int TileWidth, unsigned int TileHeight)
2{
3 if(TileWidth == 0 || TileHeight == 0)
4 return false;
5 if(!m_TileSet.Load(filename))
6 return false;
7 
8 // invariant: From here on, m_TileSet was loaded successfully
9 
10 m_Cols = m_TileSet.Width() / TileWidth;
11 m_Rows = m_TileSet.Height() / TileHeight;
12
13 m_TileHeight = TileHeight;
14 m_TileWidth = TileWidth;
15 
16 m_Filename = filename;
17 return true;
18}

Alright, to wrap it up, always expect the worst to happen. Think through the cases with bad input and all combinations of them, and try to catch as many of them as you possibly can. Define what exactly a method should do (like, change any of the object's members only if successful, and on error leave the object entirely untouched). I can almost certainly assure you that this thinking will help the prevention of many bugs.

This might not help you right now, but hopefully you can use it in the long run.

Matthew Dalrymple

Ahh thank you very much Simon for pointing that out >.< I'm trying to get a formula to work for me right now. I've tried Column * Row + m_Width but I'm still not getting correct results.

To Indeterminatus,
Thanks for your insight to my LoadTileSet() method. It makes perfect sense and I shall try and make future decisions like that :-D, really good advice. I'm having trouble though. What should m_Filename be declared as? I can't get it to be compatible with const char*.

Indeterminatus
Quote:

What should m_Filename be declared as? I can't get it to be compatible with const char*.

Best choice is probably std::string

Matthew Dalrymple

Alright that works :-D Now back to finding my big errrrrrooooooorrrrrr >.< 8-)

Simon Parzer
Quote:

Ahh thank you very much Simon for pointing that out >.< I'm trying to get a formula to work for me right now. I've tried Column * Row + m_Width but I'm still not getting correct results.

This is still wrong.
Example:

0 0 0 0 0 0 0
0 0 0 0 0 0 0
0 0 0 1 0 0 0
0 0 0 0 0 0 0

if the upper-left 0 is at the position (0,0), you want element (3,2). So how do you calculate the one-dimensional index? Each row has 7 numbers (m_Width in your case). So if we want row 2 we say 2 * m_Width, which is 2*7 = 14. Then we simply add the column index, 14 + 3 = 17. Let's count (beginning with 0).. it is RIGHT!
So your formula would be Row * m_Width + Column.

And if that doesn't work (another error somewhere else?) you should add debugging output like the following:
Original code

          while(Column < m_Width && Xpixel < screenWIDTH)
          {
               TileNumber = m_Map[Row * m_Width + Column];
               m_Tiles->DrawTile(TileNumber, (float)Xpixel, (float)Ypixel);
               Column++;
               Xpixel += m_Tiles->GetTileWidth();
          }

With debugging output:

          while(Column < m_Width && Xpixel < screenWIDTH)
          {
               int arrayIndex = Row * m_Width + Column;
               printf("Row: %d, Column: %d, ArrayIndex: %d, ",Row,Column,arrayIndex); 
               TileNumber = m_Map[arrayIndex];
               printf("TileNumber: %d\n",TileNumber);
               m_Tiles->DrawTile(TileNumber, (float)Xpixel, (float)Ypixel);
               Column++;
               Xpixel += m_Tiles->GetTileWidth();
          }

That will give you a lot of output, so you should redirect it to a file. Then you can check the following: Your map array in the test case is 16*21, so ArrayIndex mustn't be bigger than (16*21) = 336!

Matthew Dalrymple

Alright that formula works perfectly :-D I am speculating that the error is inside the cTIle::Draw() method now. I have been using debugging output like that, but in the form of allegro messages. Because of annoyance of trying to close those pop-up messages and then trying to hit escape to close the program I devised a debugging key. I use:

if(key[KEY_D])
{
    allegro_message("Output: %$", output I want to check);
}

printf() has no way of displaying to the screen in allegro >.<

Edit: Wee I'm almost stuck again, but I'm a bad programmer if I can't find my mistake so I must tear into this some more >.<

Simon Parzer
Quote:

printf() has no way of displaying to the screen in allegro >.<

I know. printf() outputs to the standard output, so if you are smart you compile your program as a console application, start it from terminal in windowed mode and then look at the output in the terminal. You can also start it like
program.exe >log.txt
to output into a file. Or you directly write to a file, by using ofstream or fprintf().

Matthew Dalrymple

Ooooo I'm liking this. I just tested it using that redirection operator mmmm. How I love writing "moo" to a file. Thanks, I love learning new stuff :D:D:D:D

Thread #588873. Printed from Allegro.cc