C++

Forum

 
Dynamic multidimensional array problem

Moarthenfeeling (21)
I asked a question on stackoverflow.com, but I haven't recieved the corecct answer.
So, I'll repost this question here

'm developing a 2d-platformer. Everything was fine until I've got some hard to solve problem. Level map is stored in dynamic multidemension array(char **map). It works fine, until I want to redefine it

Here's the part of code:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
 
Map& Map::operator=(const Map& rhs)
{
    if(width!=0||height!=0)
    {
        for(int i=0;i<width;i++)
            delete[] map[i];
        delete[] map;
    } //deleting previously created array

    height=rhs.height;
    width=rhs.width; //receiving other map's size

    map=new char* [width];
    walkmap=new unsigned char* [width];
    objmap=new char* [width];
    for(int i=0;i<width;i++)
    {
        *(map+i)=new char[height];
    } //creating new array

    for(int h=0;h<height;h++)
        for(int w=0;w<width;w++)
        {
            map[w][h]=rhs.map[w][h];
        } //receiving new values

    //...
}


Everything works fine for the first time, but when I need to redefine array for the second time my program crashes at the part, when array is receiving new values from another one. May be I miss something, but I can't find it! I was searching for this problem, but didn't find what I am doing wrong. Help me, please.
I guess I'm freeing the memory wrong...
Disch (13742)
Other than not protecting against self assignment, I don't see anything wrong with that snippit you posted.

To protect against self assignment, you can just throw this at the beginning of that function:

1
2
 
if(&rhs == this)
  return *this;


That said, can you post your constructors and destructor as well? And anything else that manages this 'map' memory?

EDIT: also, for what it's worth: http://cplusplus.com/forum/articles/17108/

EDIT 2: Also, why do you have multiple maps? walkmap/objmap? I don't see where you're deleting/copying those.

If you have multiple of the same thing like that you really should objectify it to minimize duplicate code and work out problems like this. Instead of using pointers to pointers like you are, you really should make an array2d class or something and use that instead.
Last edited on
Moarthenfeeling (21)
Yeah, I know about this code, and I've got it too, but I just cutted down the parts, which don't really matter.

Constructor don't work with this array, but functions, which work with the map do.
For example
1
2
3
4
5
6
7
8
9
10
11
12
13
 
bool Map::LoadMap()
{
	
	height=CountStrings(fileName); //simple function which counts number of strings in the text file
	map=new char* [width];
	for(int i=0;i<width;i++)
	{
		*(map+i)=new char[height];
	} 
	ReadMap();
	//etc. Other code don't work with array
        return true;
}


here's ReadMap function:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
 
void Map::ReadMap()
{
	std::fstream is;
	is.open( fileName, std::ios_base::in );
	char t;
	for(int h=0;h<height;h++)
		for(int w=0;w<width;w++)
		{
			is.get(t);
			if(t=='\n'||t=='\r'){w--;} //ignoring newline symbols
			else
				map[w][h]=t;
		}
		is.close();
}


When I work with a single Map object everything works just fine!
But I need to change maps. So I have
Map currentLevel;
And then I use '='
1
2
3
4
 
Map level1;
Map level2;
currentLevel=level1; //works nice!
currentLevel=level2; //crashes when assigning map


And constructor is:
1
2
3
4
5
6
 
Map::~Map()
{
	for(int i=0;i<width;i++)
	delete[] map[i];
	delete[] map;
}

Moarthenfeeling (21)
And the only thing I learned from this article, that I have a big trouble, I guess :D
Disch (13742)
Well I still don't see anything wrong with 'map' code. But what is with these 'walkmap' and 'objmap' buffers? Perhaps the problem is with them.

Or maybe you're stepping outside of your array bounds somewhere?

But really... the best solution here would be to get rid of the char** nonsense and make an array2d class.
Last edited on
Moarthenfeeling (21)
They use the same methods as 'map' array. I just don't include code, but they are the same size.
And I'm not stepping outside, because It crashes at
1
2
3
4
5
 
for(int h=0;h<height;h++)
        for(int w=0;w<width;w++)
        {
            map[w][h]=rhs.map[w][h];
        } //receiving new values

So when I'm trying to redefinite map[0][0] it crashes. Debugger says:"<Bad Ptr>" etc.


Any good solutions to make this class?

EDIT:
Also, why do you have multiple maps? walkmap/objmap? I don't see where you're deleting/copying those.

This is not maps, this is arrays, which store information of every map tile(yeah, it's the same, as define map[width][height][2], but it will drive me mad, ha-ha)
Last edited on
Disch (13742)
Any good solutions to make this class?


I'm actually working on one now because I'm bored and this is a quick and easy project for me. If you don't mind waiting a few mins I should have it for you.

which store information of every map tile(yeah, it's the same, as define map[width][height][3], but it will drive me mad


What you should do is make a Tile struct and have an array of Tiles:

1
2
3
4
5
6
7
 
struct Tile
{
  char ground;
  char obj;
  char walk;
  // ... etc
};
Moarthenfeeling (21)
Yep, I'll do it, thanks. I really thought of building Tile struct, but it's not very important at this moment.

I'm actually working on one now because I'm bored and this is a quick and easy project for me. If you don't mind waiting a few mins I should have it for you.

Sure! :)
Disch (13742)
Prelim testing shows this is working OK. I could go stl style and allow you to specify default values and be more efficient about memory allocation, but this will do for now:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
 
#pragma once

#ifndef ARRAY2D_H_INCLUDED
#define ARRAY2D_H_INCLUDED

//====================================
//  Bounds checking option.  Change this #define to one of the following values:
//  0 = never bounds checking
//  1 = bounds checking in Debug builds only (DEBUG or _DEBUG defined)
//  2 = bounds checking always

#define ARRAY2D_BOUNDCHECK_OPTION       1


//====================================
//  v* DON'T MESS WITH THIS *v
//====================================
#if (ARRAY2D_BOUNDCHECK_OPTION < 0) || (ARRAY2D_BOUNDCHECK_OPTION > 2)
#error "Invalid value for ARRAY2D_BOUNDCHECK_OPTION in array2d.h"
#elif (ARRAY2D_BOUNDCHECK_OPTION == 2) || ( (ARRAY2D_BOUNDCHECK_OPTION == 1) && (defined(DEBUG) || defined(_DEBUG)) )
#define ARRAY2D_BOUNDCHECK_PERFORM      1
#else
#define ARRAY2D_BOUNDCHECK_PERFORM      0
#endif
//====================================
//  ^* DON'T MESS WITH THAT *^
//====================================

// need <algorithm> for std::copy
#include <algorithm>

#if ARRAY2D_BOUNDCHECK_PERFORM == 1
#include <stdexcept>
#endif


// the actual class!
template <typename T>
class array2d
{
private:
    T*          p;
    unsigned    w;
    unsigned    h;

public:
    //===============================
    // ctors / dtors
    array2d() : p(0), w(0), h(0) { }

    array2d(const array2d<T>& r) : p(0), w(r.w), h(r.h)
    {
        if(w && h)
        {
            try
            {
                p = new T[w*h];
                std::copy(  r.p,
                            r.p + w*h,
                            p           );
            }catch(...)
            {
                delete[] p;
                throw;
            }
        }
    }

    array2d(unsigned width,unsigned height) : p(0), w(width), h(height)
    {
        if(w && h)
        {
            p = new T[w*h];
        }
    }

    ~array2d()
    {
        delete[] p;
    }

    //================================
    //  assignment
    array2d& operator = (const array2d<T>& r)
    {
        if(this == &r)
            return *this;

        T* n = 0;

        if(r.w && r.h)
        {
            try
            {
                n = new T[r.w * r.h];
                std::copy(  r.p,
                            r.p + r.w*r.h,
                            n                   );
            }catch(...)
            {
                delete[] n;
                throw;
            }
        }

        delete[] p;
        p = n;
        w = r.w;
        h = r.h;
        return *this;
    }

    //================================
    //  info
    inline unsigned GetWidth() const        { return w; }
    inline unsigned GetHeight() const       { return h; }

    //================================
    //  accessing
#if ARRAY2D_BOUNDCHECK_PERFORM == 0
    inline       T& operator () (unsigned x,unsigned y)         { return p[(y*w)+x]; }
    inline const T& operator () (unsigned x,unsigned y) const   { return p[(y*w)+x]; }
#elif ARRAY2D_BOUNDCHECK_PERFORM == 1
    inline       T& operator () (unsigned x,unsigned y)         { AssertBounds(x,y); return p[(y*w)+x]; }
    inline const T& operator () (unsigned x,unsigned y) const   { AssertBounds(x,y); return p[(y*w)+x]; }
#endif

    //===============================
    //  resizing
    void Resize(unsigned width,unsigned height)
    {
        T* n = 0;
        if(width && height)
        {
            try
            {
                n = new T[width * height];
                unsigned copyx = std::min(w,width);
                unsigned copyy = std::min(h,height);

                for(unsigned y = 0; y < copyy; ++y)
                    std::copy(  p + (y*w),
                                p + (y*w) + copyx,
                                n + (y*width)       );
            }catch(...)
            {
                delete[] n;
                throw;
            }
        }
        delete[] p;
        p = n;
        w = width;
        h = height;
    }

    //===============================
    //  bounds checking
private:
#if ARRAY2D_BOUNDCHECK_PERFORM == 1
    inline void AssertBounds(unsigned x,unsigned y) const
    {
        if((x >= w) || (y >= h))
            throw std::out_of_range("Attempting to access out-of-bounds element in array2d class");
    }
#endif
};

#endif // ARRAY2D_H_INCLUDED
Last edited on
Disch (13742)
Just in case it's unclear... usage is as follows:

1
2
3
4
5
6
7
8
9
10
11
12
 
struct Tile
{
  char ground;
  //  ... etc
};

array2d<Tile> Map;

Map.Resize( width, height );

if( Map(x,y).ground == whatever )
  etc();
Moarthenfeeling (21)
Okay, I'll try this code.(but there's so many lines of code to be redone!) and inform you as soon as I can. Thanks a lot!

EDIT: And, btw, can you show me, how my code from the first post'll look?
Last edited on
Disch (13742)
Moarthenfeeling wrote:
can you show me, how my code from the first post'll look?

Sure:

1
2
3
4
5
6
 
Map& Map::operator=(const Map& rhs)
{
    map = rhs.map;

    //...
}


In fact, you probably don't need to overload the = operator at all. You should be able to just get rid of it entirely and use the compiler-supplied version.

Also I made a few corrections to my post so be sure to use the new updated version
Last edited on
Moarthenfeeling (21)
And is that so simple? o_o
So I don't have to worry about resizing the array?
Disch (13742)
And is that so simple? o_o


Yes, it's that simple.

Objectifying things is great. Keep code properly separated so that it's easier to use.

So I don't have to worry about resizing the array?


Nope. Reassigning the array will automatically resize it to the new array's size as you'd expect.

If you need to resize without an assignment you just call Resize():

 
 
map.Resize( newwidth, newheight );



EDIT: Also, I just noticed Resize had a memory leak. I updated it so be sure you use the latest.
Moarthenfeeling (21)
Oh, I'm so full of work :(
And now I'll just re-edit some functions at this time, so I haven't debugged the game yet.

EDIT:Wow! This works! ^_^
But if I assign an array with less more size, the
throw std::out_of_range("Attempting to access out-of-bounds element in array2d class");
appears. But I think it's my problem, and I'm wrong somewhere in the other place(because it works fine with the copy of the map, like
1
2
3
4
5
6
 
Map level1;
Map level2;//(it's the same as level2)
Map currmap;

currmap=level1;
currmap=level2;

It crashed at the last line with my code, but it works with yours.Thanks for the 9001 time :D
(This was the first time I ever asked for the help from somebody in my coding experience. Books and google usually help, but this problem got me stuck)
Solved!
Last edited on
Topic archived. No new replies allowed.