Skip to main content
gamedev.net
🔒 Locked

Code design help

Started by liv1d 11 replies 2.6k views
Original Post
liv1d
liv1d
Hello,

I'd like to preface my request for help with some clarification on what I'm looking for. I am looking for someone experienced in C++ to help me figure out what exactly is going wrong. I believe it has to do with object pointers and passing them among member functions. I also have some issues with the code design and I would appreciate help on ways to better structure it. I'm also really not too sure on how to ask for design and coding help, If I excluded something necessary, just let me know and I'll do my best to post it.

I have working on a 2d platformer with Visual Studio C++ 2010 Express in my spare time using the allegro graphics lib. All was going well until I hit a game breaking bug. It happened during the process of restructuring game assets and adding animation. I tried and tried to figure it out but was never successful. I decided to take an extended break from the project in hopes I could come back to it with a clear head and figure it out. I have now come back to the project and much to my dismay, am still not making much progress on finding/fixing the bug.

The first thing I did after familiarizing myself with the code again was rewrite it using single bitmaps for objects (the way it was before it broke, I also restructured the assets the way they were before as well). Unfortunately, the bug is still there. The debugger still stops at the same location with the same symptoms.

When I start the game, it makes it past all initialization and reaches a check for the players shield before crashing. It is a simple bool check and the code itself is fine, it fails because the player object pointer is invalid (this is what I gather from what the debugger shows). The player object is a member of the map object. The debugger shows the map object variables assigned in the constructor are valid, but all member objects are empty or "???" which probably means an invalid pointer or something of the like.

I am fairly certain the issue has to do with how I'm handling object pointers. I'll try to detail the code flow where I believe the problem is.

-declare mapFile* globaly in main.cpp
-call map constructor in main
-set basic data variables in constructor (when debugging, these values are correct)
-next call loadMapFile() in main.cpp
-open file, for each line
-return MapObject* from parseLine() and add it to objects vector, also sets type, x,y cord and bitmap location to object
-call buildMap()
-for each object in object vector
-determine type, create specific object (derived from MapObject class), set x,y cords and bitmap location from MapObject
-add object to array of that type of object
-start main loop

I really just want to fix my game. I know the way I handled object creation could use some work, and would appreciate any detailed responses on better ways of doing it. I'm willing to post any relevant snippets if someone is interested in helping me go through the code. It is entirely too large of a project to post (27 files) so let me know what would be helpful.

Thanks for reading/any help!

EDIT: Including the code for the functions mentioned above. DISCLAIMER: I know there is a total absence of error checking and it would have probably helped the situation to have it there, but in this thread I'm only looking for help on the actual code/design issue.


void MapFile::loadMapFile()
{
std::string line = "";

std::ifstream file;
file.open(map_file_location, std::ios::in);
if (file.is_open())
{
while(file.good())
{
getline(file,line);
objects.push_back(parseLine(line));
}
file.close();
buildMap();
}
}



MapObject* MapFile::parseLine(std::string line)
{
int last_search = 0;
size_t found = 0;

std::string str_type = "";
std::string str_bmp = "";
int x = 0;
int y = 0;

found = line.find(',',0);
str_type = line.substr(0,found);
last_search = found;

//get bitmap
found = line.find(',',found+1);
str_bmp = line.substr(last_search+1,found - (last_search + 1));
last_search = found;

found = line.find(',',found+1);
x = atoi(line.substr(last_search+1,found - (last_search + 1)).c_str());
last_search = found;

found = line.find(',',found+1);
y = atoi(line.substr(last_search+1,found - (last_search + 1)).c_str());
last_search = found;

return new MapObject(str_type,str_bmp,x,y);
}



void MapFile::buildMap()
{
for(int i=0;i<objects.size();i++)
{
if (objects.at(i)->getType() == PLAYER)
{
player = new Player(objects.at(i)->getX(),objects.at(i)->getY(),objects.at(i)->getBitmapStr());
player_created = true;
}
else if (objects.at(i)->getType() == BLOCK)
{
blocks.push_back(new block(objects.at(i)->getX(),objects.at(i)->getY(),objects.at(i)->getBitmapStr()));
}
else if (objects.at(i)->getType() == BOT)
{
bots.push_back(new Bot(objects.at(i)->getX(),objects.at(i)->getY(),objects.at(i)->getBitmapStr()));
}
else if (objects.at(i)->getType() == GOAL)
{
goal = new Goal(objects.at(i)->getX(),objects.at(i)->getY(),objects.at(i)->getBitmapStr());
is_goal = true;
}
else if (objects.at(i)->getType() == SPIKES)
{
spikes.push_back(new SpikePit(objects.at(i)->getX(),objects.at(i)->getY(),objects.at(i)->getBitmapStr()));
}
else if (objects.at(i)->getType() == JUMP_PAD)
{
jump_pads.push_back(new JumpPad(objects.at(i)->getX(),objects.at(i)->getY(),objects.at(i)->getBitmapStr()));
}
}
}
Zakwayda
Zakwayda
Before looking over your code, I'll just offer some (fairly obvious) observations.

First, just to be sure, are you debugging using the debug build? Attempting to debug using a release build can sometimes lead to confusing output in debug windows.

The next thing I'll ask is, are you using pointers more than is necessary? Generally, in C++ it's best only to use pointers when you have a specific reason to do so (creating an object dynamically, storing references to polymorphic objects in a container, 'pimpl' idiom, etc.). You might take a look at your pointer usage and see if it can be reduced; for example, perhaps some objects could be stored by value rather than via pointer.

Two more questions:

1. Are you using smart pointers?
2. Are you following the 'rule of three'?

A quick look at your code suggests the answer to question 1 is 'no'. As for question 2, if you're not following the 'rule of three', that's probably one of the first things you'll want to look at.
liv1d
liv1d

Before looking over your code, I'll just offer some (fairly obvious) observations.

First, just to be sure, are you debugging using the debug build? Attempting to debug using a release build can sometimes lead to confusing output in debug windows.

The next thing I'll ask is, are you using pointers more than is necessary? Generally, in C++ it's best only to use pointers when you have a specific reason to do so (creating an object dynamically, storing references to polymorphic objects in a container, 'pimpl' idiom, etc.). You might take a look at your pointer usage and see if it can be reduced; for example, perhaps some objects could be stored by value rather than via pointer.

Two more questions:

1. Are you using smart pointers?
2. Are you following the 'rule of three'?

A quick look at your code suggests the answer to question 1 is 'no'. As for question 2, if you're not following the 'rule of three', that's probably one of the first things you'll want to look at.


Thanks for the reply.

I am using the debug build. The game loads in objects from map files so the object creation is dynamic. When loading I store the objects in the parent MapObject and then create specific objects and store them in vectors. I didn't think you could create objects dynamically without the use of pointers.

I am not using smart pointers, though that would probably help. I am using the rule of three. I won't have the code in front of me until tomorrow but I can post the copy constructors then if you want to take a look.

I really can't help but think its a scope issue with the pointers. I'm not sure how, I create the map class object in main and then all objects are created in member functions of the map class and stored in the map class. I've heavily tested the map input from inside the member function and it seems to go ok..it just looses the objects when It gets back to main..Any ideas?
Zakwayda
Zakwayda

The game loads in objects from map files so the object creation is dynamic.

That the objects are created using data loaded from a file doesn't mean they have to be created using 'new'.

I didn't think you could create objects dynamically without the use of pointers.[/quote]
It depends on what you mean by 'create dynamically', but you can certainly create objects (and store them in containers) without using pointers.

As I mentioned earlier, there are certainly situations where use of pointers is appropriate; but, they should (arguably) only be used where there's a specific reason to do so.
liv1d
liv1d
Hmm, I was ignorant about how little I should be using pointers. For some reason I thought I had to use new to create objects in a function parameter like I was doing. I wrote some test code and it looks like everything should work without the use of pointers so I'm going to do that.

I'll let you know if that fixes the problem. Thanks for your help so far.
Zakwayda
Zakwayda

Hmm, I was ignorant about how little I should be using pointers. For some reason I thought I had to use new to create objects in a function parameter like I was doing. I wrote some test code and it looks like everything should work without the use of pointers so I'm going to do that.

Just to be sure I don't mislead you, let me stress that the message isn't so much not to use pointers, but rather not to use them when they're not needed and where using references or (non-pointer) values would be more appropriate.

Without digging into your code I can't say for sure if/where you should be using pointers, but if there are specific situations that you're unsure of, you can always ask.
liv1d
liv1d

Just to be sure I don't mislead you, let me stress that the message isn't so much not to use pointers, but rather not to use them when they're not needed and where using references or (non-pointer) values would be more appropriate.

Without digging into your code I can't say for sure if/where you should be using pointers, but if there are specific situations that you're unsure of, you can always ask.



It worked. I stopped using pointers on everything but a few objects that needed it. It took awhile but my game is up and running again!

Thanks a lot for your help!
Zahlman
Zahlman

It worked. I stopped using pointers on everything but a few objects that needed it. It took awhile but my game is up and running again!

Thanks a lot for your help!


Glad to hear it. Two quick notes in summary:

1) Remember, classes define a data type. In general, you don't add a layer of indirection all the time with [font="Courier New"]int[font="Arial"], so you don't need it all the time with instances of [font="Courier New"]MyClass[/font], for the same reasons; and when you do, you need it for generally the same reasons.[/font][/font] The only extra uses for indirection with class instances, really, are (a) to support polymorphism via inheritance; (b) the pImpl pattern, if you use it.

2) Pointers aren't the only way to achieve indirection. They are the most "raw" way possible, so following the general rule, you should treat them as a last resort. (If you don't mind the extra typing, it would not be horrible to default to a simple [font="Courier New"]ptr<>[/font] class with a limited for scalars (I am not sure if you can get away with using [font="Courier New"]boost::weak_ptr<>[/font] for this if you aren't referring to things that are already shared with [font="Courier New"]boost::shared_ptr<>[/font]), and stick with [font="Courier New"]boost::array<>[/font] for arrays everywhere (since you now have an object, you can pass it around by reference, and even return it, by either value or reference).
MeshGearFox
MeshGearFox
Just something I noticed.

"return new MapObject(str_type,str_bmp,x,y);"

I don't personally like to return new objects. There's no guarantee that they're going to be used sanely and as such you can get some weird memory leak errors. One option might be to have parseLine take a reference to whatever data structure you're storing and just have parseLine add the new object to that structure. That way, you're sure that the new object is being kept in something stable instead of just shot off into space.

Or you could turn parseLine into a constructor for MapObjects, actually, and keep the other constructor when you're not making map objects from a file.

I don't, unfortunately, know why your pointers are CURRENTLY getting lost. Althoooouuuuugh, and I don't know if this is what's happening here or not, I do recall that the memory addresses of stuff in vectors can get shifted around during vector resizes, so it's not generally a good idea to store pointers to things in vector. I think having a vector be a vector of pointers actually is safer in this sense because, while the pointers themselves can get moved around, the stuff they point to never will.

If you plan on having a lot of additions and removals to objects, a list might be a more appropriate container.

liv1d
liv1d
Thanks for the tips guys, great suggestions. I'm having an issue though. After getting away from pointers for class objects, any variable changes I make inside a member function don't carry over to that instance of the class in the main program.

Example: (Note, normally my member variables aren't public, this was for testing)

in main.cpp:

map.player.x += map.player.getSpeed();


Works fine to move the player, but the way I had it before with pointers (and it worked fine) doesn't work anymore.

in player.cpp, in move member function:

x += speed;


Any ideas on why this stopped working? I much prefer doing the work inside the member functions.
Zakwayda
Zakwayda
It sounds like you've made copies of your objects and are operating on separate instances of them. (E.g. you have an instance A, but have inadvertently made a copy of A and are operating on the copy rather than on the original object.)

To tell you why/how that's happening, I think we'd need to see more code. A minimal example demonstrating the behavior would be ideal, but barring that, just posting all the relevant code would probably suffice.
liv1d
liv1d

It sounds like you've made copies of your objects and are operating on separate instances of them. (E.g. you have an instance A, but have inadvertently made a copy of A and are operating on the copy rather than on the original object.)

To tell you why/how that's happening, I think we'd need to see more code. A minimal example demonstrating the behavior would be ideal, but barring that, just posting all the relevant code would probably suffice.



That makes sense. I wrote a stripped down example and was able to replicate the issue. From what you said, I'm pretty certain it has to do with the member function returning a copied instance of the private Player object that isn't the correct instance, but I'm still unsure on how to remedy it or exactly why its happening. I tried including my own copy constructor/assignment operator to see if that would fix the issue to no avail. I left them out as they didn't seem to effect anything in the example.

Test program:

#include <iostream>

using namespace std;

class Player
{
int x;
int speed;
public:
Player() { x=0; speed=1; } //req default constructor
Player(int s_x) { x=s_x; speed=1; }
void move() { x += speed; }
int getX() { return x; }
};

class MapFile
{
Player player;
public:
MapFile() { loadMap(); }
Player getPlayer() { return player; }
void loadMap() { player = Player(0); }
};

int main()
{
MapFile map = MapFile();
cout << map.getPlayer().getX() << endl;
map.getPlayer().move();
cout << map.getPlayer().getX() << endl;
system("PAUSE");
}


Thanks for the help, again.
Zakwayda
Zakwayda
Here:

Player getPlayer() { return player; }
You're returning a copy of the player object. Any operations performed on that copy will not affect the original object in any way (not under normal circumstances, at least).

If you want to return a mutable reference, you can return a non-constant reference as follows:

Player& getPlayer() { return player; }
If you want to return a reference for query purposes only, you can return a constant reference:

const Player& getPlayer() const { return player; }
Returning a copy may be indistinguishable from the above in terms of end effect in some cases, but returning a (constant) reference is likely to be more efficient.

There's also the question of whether exposing the player object directly in this way is the best choice design-wise, but that's more or less separate from the (purely technical) problem you're trying to solve.

Topic Locked

This topic has been locked by a moderator. New replies are not allowed.

Sign in to reply to this topic.