Hey. A doubt about copy constructors and stuffies (C++)

[desocupado]

So, because of some copying and destructor shenanigans, I'm forced to implement the Rule of Three (now five since C++11!) in one of my classes.

But I have a question.

The class I'm implementing a copy (and assignment) constructor is called Tile, and it has a member called m_obj

m_obj is a pointer to a Map_Object class. However, Map_Object is actually an abstract class with many child classes, and m_obj of a Tile object might be pointing to a child class object, instead of Map_Object object.

But I won't know at the time of copying. It might be some child class object, it might be a Map_Object object.

So, how does one deep copy m_obj in this case?

 

[Alex]

Deso, you could just make all your classes implement a Cloneable abstract class. The class would need one method: Cloneable* clone(); which returns a pointer to a copy of itself. Just clone all pointers and you should be ok.

However, why are you copying tiles and Map_Objects?

 

[desocupado]

Tiles have a pointer to map objects, and thus have "delete" on the destructor. The pathfinding class might copy some tiles, I'm not sure, I don't remember anymore.

Plus the way I designed map class creates one tile object, sets it's stuff proper and copies it into a vector.

Everytime I callled "delete" on stuff, I was having things go bonkers.

 

[Alex]

1) I don't think Tiles should "own" the objects, since the objects are continuously changing Tiles. You could keep the Map_Objects in the Map, and let Map handle their deletion (deleting a tile wouldn't delete the objects, nor need to).

2) Do you really need to give out Tile copies? Pathfinding should be able to make do with const tile refferences or pointers (pointers probably would be better). If some class needs access to what is in a tile, you could give them an iterator to a const container. The class would be able to view and call non const methods on the contents of the tile, but be unable to change the tile itself, which I suspect is what you almost always want (you should only need to change the contents of a tile when the contents move or are destroyed. Both cases can be done as special functions in tiles (destroy (Map_Object o) would just remove it, while move(Map_Object o, Tile t) would remove it from the current tile and add it to t).

 

[Sankarihauta]

Never have objects clean themselves up. Have other objects clean them up.

 

[desocupado]

1) I don't think Tiles should "own" the objects, since the objects are continuously changing Tiles. You could keep the Map_Objects in the Map, and let Map handle their deletion (deleting a tile wouldn't delete the objects, nor need to).

2) Do you really need to give out Tile copies? Pathfinding should be able to make do with const tile refferences or pointers (pointers probably would be better). If some class needs access to what is in a tile, you could give them an iterator to a const container. The class would be able to view and call non const methods on the contents of the tile, but be unable to change the tile itself, which I suspect is what you almost always want (you should only need to change the contents of a tile when the contents move or are destroyed. Both cases can be done as special functions in tiles (destroy (Map_Object o) would just remove it, while move(Map_Object o, Tile t) would remove it from the current tile and add it to t).

You approach of keping map_objets in map instead of tiles sounds a lot less complicated. However, the idea of having the map_objects inside the tile was because map_objects can change tile properties, and having the map_object inside tile, would allow map_objects to modify tile without altering it's contents. Say you have a function

bool Tile::blocks_LoS()

With a map_object inside tile, I could

if (map_object->blocks_LoS() == true) return true;
else return m_block_loS; <----This line refers to the Tile m_block_LoS member variable

or more importantly, in the case of a function that return AP travel costs

return m_travel_cost + map_object->r_added_travel_cost();

With map_objects outside of tiles, when I sent my tiles to the pathfinding function (which can make do with references only, like you mentioned), they won't take into account added travel costs or impassability generated by map_objects (unless I alter the pathfinding function to deal exclusively with map, which while doable, might generate some headache)

Let's talk keeping map_objects inside map. I see two approaches, keeping a map_object for each tile position ( a null map_object when you don't have an actual object ), or keeping a list of actual, non-null, map objects with their positions, and just search that list to see if there's a map_object in the same position as the tile that's being accessed when I need to know something about a tile. Like:

bool Map::r_blocks_LoS(int x, int y)
{
check_if_object_exists_at(x, y); <-----This function would iterate over a list or something
deal with LoS of the tile and object, if it exists
return result
}

The first approach seems faster, because you wouldn't need to iterate over a list. But the second one wouldn't need to have scores of null objects stored, which I don't like, seems dumb to me .

 

[Alex]

Deso, you can keep your Map_Objects inside a tile. What I was saying is that the Tile shouldn't own them. That is, the tile has a pointer to its object (or a list of pointers, if it can have more than one). But it doesn't deallocate these objects when the tile is destroyed. The Map deallocates them, because it is the map that owns them. And, by the way, I still think you shouldn't copy tiles unless you have a very good reason to.

 

[desocupado]

I believe you I shouldn't copy tiles. :D

I'm doing it at least once, tho, when I declare "Tile tile", set its properties, and push it inside a vector. I don't see any harm, tho, because the tile created goes out of scope very quickly.

Pathfinding can't work with const references, tho, it Tile has some member functions to store heuristc values and parent tile AP cost values that are used by the pathfinding to... well, find a path. I'll try using pointers, tho.

How do I make Map handle the Map_Object destruction? Right now, the only variable pointing to map objects, is a pointer inside a tile.

Also, how do I clone a pointer? How do I clone a base class pointer that is pointing to a derived class?

 

[Declinator]

Also, how do I clone a pointer? How do I clone a base class pointer that is pointing to a derived class?

A pointer is just an address really. You copy it the way you would copy a pointer that points to a base class because the pointer does not care whether it is a derived class or not as the address is the same regardless.

So probably something along the lines of:
Base *a = b;
Where b is the existing pointer that may or may not point to a derived class.

 

[Alex]

I believe you I shouldn't copy tiles. :D

I'm doing it at least once, tho, when I declare "Tile tile", set its properties, and push it inside a vector. I don't see any harm, tho, because the tile created goes out of scope very quickly.

Well, unless there is good reason for making copies, the program would be uselessly copying objects and then destroying them. In other words, I am afraid this is just a waste of time.

Pathfinding can't work with const references, tho, it Tile has some member functions to store heuristc values and parent tile AP cost values that are used by the pathfinding to... well, find a path. I'll try using pointers, tho.

Ok, but do you need to actually change these values during the pathfinding? Because if you don't, you could just create a static getter for these members.

How do I make Map handle the Map_Object destruction? Right now, the only variable pointing to map objects, is a pointer inside a tile.

Just create a List (or Vector, or whatever you prefer) inside Map of Map_Objects. In the Map_Object Constructor, have a Map argument, and you can tell the map to add the built object to itself. Something like:

MapObject (Map& owner) {
owner.addObject(this)
}

You could also make addObject private and make MapObject a friend class to make it clear this is only supposed to be used by the constructor.

Now, you can be sure that MapObjects will only be deleted when the Map is deleted. Of course, you might also want to have a destroy object method in the Map that removes the object from the object list and deletes it, so objects can be deleted when they are destroyed in game.

Also, how do I clone a pointer? How do I clone a base class pointer that is pointing to a derived class?

Well, if you were to follow my suggestion, it would look like this:

class Cloneable {

public:

virtual Cloneable * clone () = 0;

}

class Map_Object : public Cloneable {
//Map Object stuff.
}

class Wall : public MapObject {
//Wall stuff.

virtual Wall* clone ();

}

//.cpp file:

virtual Wall* Wall::clone () {
Wall* clone = new Wall();
*clone = *this; //copy constructor.
return clone;
}

 

[desocupado]

Pathfinding does need to change the values during its calculations. :/ But like I said, I'll try making pointers and see if it works.

About the putting the map_objects in a list, well that's the obvious solution, but messing with them would fuck the pointers inside the tiles. For example, I have a fire object, I create it and insert it into the list. When the fire is extinguished, I call the map method to destroy the fire object. But then the pointer inside tile that pointed to the map object now points to a deleted object. BOOOOM! The program comes crashing down in a fireball, killing dozens of innocent bystanders.

Some map_objects might add an AP cost to travelling through a tile, and the pathfinding currently asks Tile for the AP costs. If Tile has a pointer to the map_object in its position, tile can just ask it for its added cost, add to its own, and return the proper info to pathfinding. That's why I want to keep a pointer to map_objects inside it's respective tile.

However, this is becoming so convoluted (and bug ridden) that I'm almost saying "fuck this shit", store the map_objects inside map with no pointer inside tile, and rewriting the whole pathfinding class.

The only bad thing about storing map_objects in a list is that if I keep a pointer to map_object inside the tile and need to check something like visibility of each tile, I only have to ask tile, and tile asks his own map_tile object, and gives me the answer. If map_objects are kept inside a list, and I need to check visibility, I'll ask tile, tile will give me an answer, then I'll have to iterate over the whole list of map objects, checking if they're at tile position, and if they are, ask for their visibility. And I need to do that for each tile in the game, every frame.

How much of a performance hit is that? Is that acceptable?

 

[Alex]

Pathfinding does need to change the values during its calculations. :/ But like I said, I'll try making pointers and see if it works.

You could also use a non-const reference. Whatever works best.

About the putting the map_objects in a list, well that's the obvious solution, but messing with them would fuck the pointers inside the tiles. For example, I have a fire object, I create it and insert it into the list. When the fire is extinguished, I call the map method to destroy the fire object. But then the pointer inside tile that pointed to the map object now points to a deleted object. BOOOOM! The program comes crashing down in a fireball, killing dozens of innocent bystanders.

Well, the deletion process would need to account for that. I think the simplest solution would be for each Map_Object to have a reference to its current tile (which needs to be 1 and only 1, I suppose, but the problem becomes more interesting when you start having multitle stuff). The moment a Map_Object is deleted, it removes itself from the Tile and the Map contaning it. Note that using this solution, when a Map is deleted, it must first delete its objects and then its tiles, for correct deletion order.

Some map_objects might add an AP cost to travelling through a tile, and the pathfinding currently asks Tile for the AP costs. If Tile has a pointer to the map_object in its position, tile can just ask it for its added cost, add to its own, and return the proper info to pathfinding. That's why I want to keep a pointer to map_objects inside it's respective tile.

However, this is becoming so convoluted (and bug ridden) that I'm almost saying "fuck this shit", store the map_objects inside map with no pointer inside tile, and rewriting the whole pathfinding class.

The only bad thing about storing map_objects in a list is that if I keep a pointer to map_object inside the tile and need to check something like visibility of each tile, I only have to ask tile, and tile asks his own map_tile object, and gives me the answer. If map_objects are kept inside a list, and I need to check visibility, I'll ask tile, tile will give me an answer, then I'll have to iterate over the whole list of map objects, checking if they're at tile position, and if they are, ask for their visibility. And I need to do that for each tile in the game, every frame.

I think keeping the object list inside the Tile is a good idea. I just think you should make it so that deleting the object isn't the Tile's responsibility. It isn't that big of a deal if it is the Tile's, however do avoid copying Tiles in that case, because you will need to copy the objects, and if this happens in a loop it could become slow. Pr if you aren't careful with your copies, you might end up with a lot of memory leaking.

How much of a performance hit is that? Is that acceptable?

Well, if it is a problem or not depends on how frequently you will call that function. Usually I don't worry much about this until my code is more or less formed, that is, representing what I have in mind. The issue is that thinking of Objects inside Tiles do make sense. So you could be making your code both slower and harder to read.

 

[desocupado]

Tiles are only copied on two ocasions. On creation, when they're copied and stored on the vector, and when the pathfinding function is called. I'm going to change the pathfinding class, so it's not a problem at all if tiles take a while to be created/copied/destroyed, since that only happens when the map is loading at the start of the scenario.

Objects, however, can be created/destroyed during the scenario, but not that often.

The bad news is that I'll need for a tile to hold multiple objects (lava + smoke, for example)

EDIT: I just changed the pathfinding class to take pointers instead of a copy, but while doing it I remembered that the class taking copies of tiles was the intended behaviour. So I could do whatever shenanigans I wanted and not worry about altering important stuff. Specially since the pathfinding function returns a path made of tiles (now tile pointers).

I feel this is very cowboy programming. I hope the indians don't come.

 

[belowmecoldhands]

(.....)
I feel this is very cowboy programming. I hope the indians don't come.

Or aliens:

 

[belowmecoldhands]

OP, this is how I felt after reading about the rule of three/five/zero:

 

[J1M]

Hopefully this doesn't sound too arrogant, but I'm going to step in here and address the root cause of your problems: object oriented programming.

Let's take a step back and figure out what you really want to do here:

-First, you need a collection of tiles. This seems like a pretty important piece of the puzzle, so I'd avoid making it dependent on something else.

-Next, you have multiple systems that want to reference tiles. You have characters standing on tiles, you have pathfinding that wants to analyze the tiles, etc. This suggests that the other systems should reference tiles, but that seems to be causing you headaches. An alternative here is for those other systems to store a key related to a tile rather than a pointer. That way the tile object can change, and you don't have pointer and object deletion problems. The most sensible key for a grid system is probably an (x,y) coordinate. If you catalog your tiles in some sort of map structure a tile lookup will be fast enough to do whenever you need to access it.

-This idea that the pathfinding algorithm needs to modify the tiles is silly. Create a parallel data structure that maps a key/coordinate to a heuristic cost. That way you can rip out your pathfinding system and replace it without changing what tiles are or how they are used.

-You mentioned some issues with adjacency if you stored the tiles differently. You are probably overthinking this. It should be a fairly simple one-time cost to calculate and store which tiles are next to each other at map creation. Using a compound key like (x,y) you may not even notice a difference in how you access (x+1,y).

-Iterating over all of your tiles during the render of a frame should be trivial for modern hardware. (How would you draw items if you didn't do this at least once?) If it becomes a problem you can get smarter about it, such as calculating visibility when a character moves.

 

[Alex]

Hey Jim, I think your suggestions are mostly good! I only think that decoupling here isn't going to be very productive. I mean, a tile needs to know what is inside itself, so it can know if it is blocking line of sight, movement, providing cover, etc. Which are things I think Deso wants to do with his code. So having some dependency between tiles and map objects is ok in this case, I think.

 

[J1M]

In this case, the primary benefit to accessing tiles with a lookup is to simplify the memory management problems that desocupado mentioned.

I would also caution against using bi-directional references between the tile and the character.