Help With Copy Constructor.

[JoeC]

I am still working on my game and my program is getting better. Most
of what I want to works. I think I am having trouble with copy
constructor. Basically I want it to copy the gdata array. My gdata
will work if I do: gdata[32] = this->get(lp); But that is not what I
want to do I get some cryptic errors when I write the code like that is
commented out.

49 C:\Documents and Settings\Work\My Documents\C++\Dungeon
Adventure2\gaphic.cpp passing `const graphic' as `this' argument of
`BYTE graphic::get(int)' discards qualifiers

C:\Documents and Settings\Work\My Documents\C++\Dungeon
Adventure2\Makefile.win [Build Error] [gaphic.o] Error 1

I am pretty good with pointer and objects but my compiler is fustrating
me. Can I get some advice to make this copy constructer do what it is
suppoded to do.

class graphic{
int btmap;
int lr,ud; //Diminsion (size) of the graphic
BYTE gdata[32]; //bitmap array
HBITMAP hbitmap;
BITMAP bitmap;
HDC hdc, hdcmem;
void copy(BYTE in[]);
BYTE get(int n){return gdata[n];}

public:
graphic();
graphic(BYTE c[]);
graphic(const graphic&);
graphic& operator = (graphic&);
void SetGr(BYTE c[]);
void set(BYTE c[]);
void display(HWND, int, int);
};

graphic::graphic(const graphic& gr){
for(int lp = 0; lp != 32; lp++){
//gdata[32] = gr.get(lp); <- I get errors.
}
ud = lr = 16;
BITMAP bitmap = {0,ud,lr,2,1,1};
bitmap.bmBits = gdata;
hbitmap = CreateBitmapIndirect(&bitmap);
}

 

[JoeC]

I am calling the copy constructr like this:

if(play){
cgr = new graphic(play->gOut());
return *cgr;
}

In case this helps

 

[Phlip]

BYTE get(int n){return gdata[n];}

BYTE get(int n) const {return gdata[n];}

Now Google for "const correct". You should make constant any method that
doesn't change *this.

 

[Dennis Jones]

I am still working on my game and my program is getting better. Most
of what I want to works. I think I am having trouble with copy
constructor. Basically I want it to copy the gdata array. My gdata
will work if I do: gdata[32] = this->get(lp); But that is not what I
want to do I get some cryptic errors when I write the code like that is
commented out.

It seems to me that the easiest and most straight-forward way to copy the
contents of gdata is to simply do this:

graphic::graphic(const graphic& gr)
{
memcpy( gdata, gr.gdata, sizeof(BYTE) * sizeof(gdata) );
}

- Dennis

 

[JoeC]

OK that is a start I took out some consts because I was getting errors.
Thanks.

 

[JoeC]

OK, that helped, but it seems like my graphics data is not getting
coppied.

 

[JoeC]

Thanks now it looks like it is tarting to work.

 

[Ron Natalie]

I am still working on my game and my program is getting better. Most
of what I want to works. I think I am having trouble with copy
constructor. Basically I want it to copy the gdata array. My gdata
will work if I do: gdata[32] = this->get(lp); But that is not what I
want to do I get some cryptic errors when I write the code like that is
commented out.

49 C:\Documents and Settings\Work\My Documents\C++\Dungeon
Adventure2\gaphic.cpp passing `const graphic' as `this' argument of
`BYTE graphic::get(int)' discards qualifiers

C:\Documents and Settings\Work\My Documents\C++\Dungeon
Adventure2\Makefile.win [Build Error] [gaphic.o] Error 1

I am pretty good with pointer and objects but my compiler is fustrating
me. Can I get some advice to make this copy constructer do what it is
suppoded to do.

class graphic{
int btmap;
int lr,ud; //Diminsion (size) of the graphic
BYTE gdata[32]; //bitmap array
HBITMAP hbitmap;
BITMAP bitmap;
HDC hdc, hdcmem;
void copy(BYTE in[]);
BYTE get(int n){return gdata[n];}

public:
graphic();
graphic(BYTE c[]);
graphic(const graphic&);
graphic& operator = (graphic&);
void SetGr(BYTE c[]);
void set(BYTE c[]);
void display(HWND, int, int);
};

graphic::graphic(const graphic& gr){
for(int lp = 0; lp != 32; lp++){
//gdata[32] = gr.get(lp); <- I get errors.

gdata[lp] = gr.get(lp);

I suspect you don't need the get() fucntion here, if all it returns is
gdata[lp] because gr.gdata while private is accessible here.

gdata[lp] = gr.gdata[lp];

You could avoid a lot of silliness by not using the busted
C++ array type and use vector which has proper copy semantics.

I assume eventually, you'll want to get rid of the assumptions
that ud and lr are both 16.

 

[JoeC]

I would like to use a vector but all this dosn't seem to accept a
vector when I did some earlier experiments. I would much rather use a
standard libray.

BITMAP bitmap = {0,ud,lr,2,1,1};
bitmap.bmBits = gdata; <- onc etried a vector here and it didn't
work.
hbitmap = CreateBitmapIndirect(&bitmap);

 

[Ron Natalie]

I would like to use a vector but all this dosn't seem to accept a
vector when I did some earlier experiments. I would much rather use a
standard libray.

BITMAP bitmap = {0,ud,lr,2,1,1};
bitmap.bmBits = gdata; <- onc etried a vector here and it didn't
work.
hbitmap = CreateBitmapIndirect(&bitmap);

bitmap.bmBits = &gdata[0];

 

[JoeC]

Thanks, I will have to redesign my whole class but I will give that a
try.

 

[JoeC]

I tried what you sugested the program still crashes. What am I doing
wrong?

#include<windows.h>
#include<vector>
#include<iostream>
#include<string>

using namespace std;

#ifndef GRAPHIC_H
#define GRAPHIC_H

class graphic{
int btmap;
int lr,ud; //Diminsion (size) of the graphic
vector<BYTE>gdata;
HBITMAP hbitmap;
BITMAP bitmap;
HDC hdc, hdcmem;
void copy(const BYTE in[]);
BYTE get(int n)const {return gdata[n];}
vector<BYTE>vGet() const {return gdata;}

public:
graphic();
graphic(const BYTE c[]);
graphic(const graphic&);
graphic& operator = (const graphic&);
void SetGr(const BYTE c[]);
void set(const BYTE c[]);
void display(HWND,const int, const int);
};

#endif


#include<windows.h>
#include<fstream>
#include<string>
#include<vector>

#include "graphic.h"

using namespace std;

void graphic::copy(const BYTE in[]){
for(int lp=0; lp != 32; lp++)
gdata.push_back(in[lp]);
}

void graphic::SetGr(const BYTE c[]){
//Changing the graphic
copy(c);
BITMAP bitmap = {0,ud,lr,2,1,1};
bitmap.bmBits = &gdata[0];
hbitmap = CreateBitmapIndirect(&bitmap);
}

graphic::graphic(const graphic& gr){
ud = lr = 16;
//memcpy( gdata, gr.gdata, 32 );
gdata = gr.vGet();

BITMAP bitmap = {0,ud,lr,2,1,1};
bitmap.bmBits = &gdata[0];
hbitmap = CreateBitmapIndirect(&bitmap);
}

 

[Dennis Jones]

I tried what you sugested the program still crashes. What am I doing
wrong?

You will need to be more specific -- "still crashes" doesn't tell us
anything. What is crashing and where?

- Dennis

 

[JoeC]

I am experimenting with different ways to do what I want. It is
tricky. I want the graphics data to be held in the objects on a grid
of space objects. That works. But what I want to do is to have
objects stored in those objects with graphic data and I want that that
data to be displayed on the screen. I got the program to run like I
would like but it the program is not written to be expanded. I want to
have variouse kinds of objects that contain graphics data later and I
don't want to keep updating my board object with new data. That is how
I made it work, I put the graphics data for my player on the map with
all the graphics for the map.

class board{

player * play;
static const int size = 30;
map<char, coord> keys;
space spaces[size][size];
coord n;
coord s;
coord e;
coord w;

ifstream& cfill(ifstream&, char&); /*Reads map from file */
void fill();
coord find();
void seeing(int, int);

public:
board();
void setPlayer(player*, int, int);
graphic& display(int, int);
int sze(){return size;}
void move(char);
};

There might or might not be a player in the map so I use a pointer. I
wand the graphics data from that player to be returned to the main
program for display.

graphic& space::graphicOut(){
if(play){
BYTE * cr = &play->GetData();
cgr = new graphic(cr);
//&play->GetData());
return *cgr; //I want to get the player's graphic data either the
bitmap array or
the graphic object and return it for display.
}
if(seen){return *gr;} //returns the graphic on the map (space or
wall)
else {return *grDefault;} //returns unseen space (blank)
}

This is the trick of what I am trying to do.

 

[Dennis Jones]

<irrelevant content snipped>

I thought you wanted help with a crash, so I asked how and where your
program was crashing. You didn't answer that question and now you seem to
be talking about something else. Was there supposed to be a question
somewhere in that last message?

- Dennis

 

[JoeC]

It is a complex problem I am working on. Still thanks for the help.
The best I can do right now is get the graphic to show garbled. So
some where I a loosing the graphics data. Thanks for your time.

 

[JoeC]

I have been working on this problem for a while and still my program is
getting better. The object seems to work but it looks like my graphics
information is getting corrupted.

What I do is create the graphic
Then I change the graphics information
finlaay I use the copy constructor.


Here is the code:

graphic::graphic(){
lr = ud =16;
BYTE c[32] = {0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,
0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,
0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,
0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff};

copy(c);
BITMAP bitmap = {0,ud,lr,2,1,1};
bitmap.bmBits = gdata;
hbitmap = CreateBitmapIndirect(&bitmap);
}

void graphic::set(const BYTE c[]){
copy(c);
ud = lr = 16;
BITMAP bitmap = {0,ud,lr,2,1,1};
bitmap.bmBits = gdata;
hbitmap = CreateBitmapIndirect(&bitmap);
}


graphic::graphic(const graphic& gr){
ud = lr = 16;
//memcpygdata, gr.gdata, 32 );
for(int lp = 0; lp != 32; lp++){
gdata[lp] = gr.get(lp);}
BITMAP bitmap = {0,ud,lr,2,1,1};
bitmap.bmBits = gdata;
hbitmap = CreateBitmapIndirect(&bitmap);
}

 

[Dennis Jones]

I have been working on this problem for a while and still my program is
getting better. The object seems to work but it looks like my graphics
information is getting corrupted.

What I do is create the graphic
Then I change the graphics information
finlaay I use the copy constructor.

<snip>

If 'gdata' is a vector (which I think you stated in one of your previous
messages), you can simplify things significantly. If 'gdata' is not already
a vector, consider making it a vector so that you can do this (note the
simplicity -- no 'copy' loop):

graphic::graphic(const graphic& gr)
{
...
gdata = gr.gdata;
...
}

Okay, now let's refactor things a little. I think it would be good to
factor out the bitmap creation code into its own function, because it is
getting duplicated in several places:

HBITMAP graphic::createBitmap( void )
{
BITMAP bitmap = { 0, ud, lr, 2, 1, 1 };
bitmap.bmBits = &gdata[0];
HBITMAP hBitmap = CreateBitmapIndirect(&bitmap);
return hBitmap;
}

Now IMO, an "ideal" copy constructor would perform a _simple_ assignment of
as many data members as it possibly can, only performing more complex copy
operations if/when it is absolutely necessary. For instance, obviously, you
can't do a simple assignment with pointer members (such as hbitmap), but you
can with most other members (assuming they support an assignment operator,
like vector does). So, I think I would probably write the copy-constructor
like this:

graphic::graphic(const graphic& other)
{
ud = other.us;
lr = other.lr;
gdata = other.gdata;
hbitmap = createBitmap();
}

You don't need the "copy" function. Assuming you make 'gdata' a vector, a
simple assignment will work just fine (and it makes the code easier to read
and understand and more robust in the face of change).

I'm not sure why you have a "set" method, since it pretty much does the same
thing as the copy-constructor, but if you need to keep it for some reason, I
would change the 'c' argument to be a vector, like this:

void graphic::set( const std::vector<BYTE> &c )
{
gdata = c;
hbitmap = createBitmap();
}

And your default constructor becomes:

graphic::graphic()
{
ud = lr = 16;
for ( int i=0; i<32; ++i )
{
gdata.push_back( 0xFF );
}

hbitmap = createBitmap();
}

I hope this makes sense to you and provides you with some direction.

- Dennis

 

[JoeC]

Thanks for the time in explaining how that works.

That helped, ithink my graphic object is much better but it still
crashes at this point:

graphic::graphic(const graphic& gr){
ud = lr = 16;
//gdata = gr.gdata; <-- This crashes
BITMAP bitmap = {0,ud,lr,2,1,1};
bitmap.bmBits = &gdata[0];
hbitmap = CreateBitmapIndirect(&bitmap);
}

That bity of code crashes when I use the command like this:

if(play){
cgr = new graphic(play->gOut());
return *cgr;

 

[JoeC]

Thanks for the time in explaining how that works.

That helped, ithink my graphic object is much better but it still
crashes at this point:

graphic::graphic(const graphic& gr){
ud = lr = 16;
//gdata = gr.gdata; <-- This crashes
BITMAP bitmap = {0,ud,lr,2,1,1};
bitmap.bmBits = &gdata[0];
hbitmap = CreateBitmapIndirect(&bitmap);
}


That bity of code crashes when I use the command like this:

if(play){
cgr = new graphic(play->gOut());
return *cgr;