Could this depth system for a game be improved?

Question

I am still new to C++ and don't have a great insight on my coding yet, so I would be very grateful to anyone and everyone that gives advice.

Also, this code is meant to: keep all of my objects in an ordered fashion based on depth. I have a couple functions that allow for easy management and I made object friends with the depth manager because I only want the depthManager to have control over each objects idDepth and depth which are two different things.

The reason I need a depth system is because I need to have objects execute there code in an orderly fashion, and I also need to have control of what objects are drawn first to last.

This class has been tested and works as expected.

• Stay up to date with this project on GitHub.com
  
  

---

Abstract object class:

class object{
 // Placement Data
 unsigned int depth, idDepth, idObject, idMain;
 // Friends
 friend class depthManager;
 friend class objectManager;
protected:
 unsigned int getDepthId(){ return idDepth; }
public:
 virtual void update() = 0;
 virtual void draw() = 0;
 unsigned int getDepth() { return this->depth; }
};

note: I have each object handle its own update and draw events for easier design. I'm mainly asking on approval for the depth system.

---

depthManager class

class depthManager{
private:
 std::map<unsigned, std::vector<object*>* > objectMap;
 void changeListPlacement( unsigned int depth, unsigned int position, int change);
public:
 void objectAdd( unsigned int depth, object* obj);
 void objectRemove( object* obj );
 void objectMove( unsigned int depth, object* obj );
};

depthManagers Functions:

void depthManager::objectAdd(unsigned int depth, object *obj)
{
 obj->depth = depth;
 // Check if depth key existant
 if ( objectMap.find( depth ) != objectMap.end() )
 {
 std::vector< object* >* &refVec = objectMap[ depth ];
 refVec->push_back( obj );
 obj->idDepth = (unsigned)(int)refVec->size() - 1;
 }
 else // Add new Key
 {
 objectMap[ depth ] = new std::vector< object* >;
 std::vector< object* >* &refVec = objectMap[ depth ];
 refVec->push_back( obj );
 obj->idDepth = (unsigned)(int)refVec->size() - 1;
 }
}
void depthManager::changeListPlacement(unsigned int depth, unsigned int position, int change = -1)
{
 if ( objectMap.find( depth ) == objectMap.end() )
 {
 return;
 }
 std::vector<object*>* &refVec = objectMap[ depth ];
 for( unsigned int i = refVec->size() - 1; i > position; i -- )
 {
 object* &pObj = refVec->at( i );
 pObj->idDepth += change;
 }
}
void depthManager::objectRemove(object *obj)
{
 if ( objectMap.find( obj->depth ) == objectMap.end() )
 {
 std::cout << "ERROR DEPTH NOT FOUND \n" << std::flush ;
 return;
 }
 std::vector<object*>* &refVec = objectMap[ obj->depth ];
 changeListPlacement( obj->depth, obj->idDepth);
 refVec->erase( refVec->begin() + obj->idDepth );
}
void depthManager::objectMove(unsigned int depth, object *obj)
{
 this->objectRemove( obj );
 this->objectAdd( depth, obj );
}

previous verions

Depth Manager Source Code 1

• Karolis Juodelė: Can you elaborate more on the purpose? Why is this better than a single flat vector of objects ordered by depth? What are the use cases, besides iterating though all objects in all depths in either order?
• Andrew: @KarolisJuodele It is easier to manage. Instead of keeping track of how large each depth section is in one vector, it's split in to more easily manageable sections based on depth. This way the number of iterations is reduced to a fraction of the amount. Notice how if you were to remove an object in a single vector in the middle of one large vector it would effect the position of objects not even in the same depth resulting in unnecessary processing time. This engines main purpose is depth, and keeping track of it, so there is little scope outside of that reason.
• Andrew: @YoungJohn Yes, I was aware of this when writing the code. That's why the map holds vectors that contain a list of objects there for more than one object can be located in the same depth. -- std::map<unsigned, std::vector<object*>* > objectMap;
• YoungJohn: @Lemony-Andrew while a map of vectors will achieve what you are looking for you could also look into using std::multimap which exists for this purpose. Also, if you are using C++11 you could look into using unordered_map or unordered_multimap, but those may all be overkill for your project depending on your performance concerns.
• YoungJohn: @Lemony-Andrew Yeah, there's always more than one way to solve a problem and you just have to go with what works best for you, but I'm glad you at least took the time to look at your alternatives.

Answer 1

Some remarks to your current map-based implementation:

1. The idiomatic way of accessing an item in a map and adding it if not present usually goes like this:

   if not map.contains(key)
    map[key] = new value
   value = map[key]
   ... modify value
   

   So objectAdd could be shortened:

   obj->depth = depth;
   // make sure we have an object vector for the given depth
   if ( objectMap.find( depth ) == objectMap.end() )
   {
    objectMap[ depth ] = new std::vector< object* >;
   }
   std::vector< object* >* &refVec = objectMap[ depth ];
   refVec->push_back( obj );
   obj->idDepth = (unsigned)(int)refVec->size() - 1;
   

2. In objectRemove it is apparently illegal to pass an object with a non-existent depth. Printing an error message to stdout is not the best way to handle an error like that. You should throw an appropriate exception (fail early is a valuable debugging tool) or allow it by ignoring invalid depths.

3. It is not imminently clear what objectMove exactly does based on its name and the names of its parameters. It looks like it moves an object to a different depth. So a better name and signature might be:

   void depthManager::changeDepth(unsigned int newDepth, object *obj)
   

4. In class object the methods getIdDepth() and getDepth do not modify the object state so you should consider making them const (i.e. unsigned int getDepth() const { return this->depth; }).

5. I'm not 100% convinced of the idDepth property. It basically just reflects the current position of the object in the depth-list and you are writing a fair amount of boiler plate code to keep it that way. This increases the complexity of the class somewhat. I'd revisit the concept and check if I can't get by without it.

• Andrew: Great catch on point #1! It definitely should be simplified. On #2 I have been semi rushing this code, I haven't sat down to learn error handling yet; but I do intend to add that functionality. #3 when I made that function I was uncertain on the naming convention, changeDepth does sound more reasonable. #4 This functionality was replace with const getDepth() earlier today, and yes it should be read only. #5 All the Id's are merely positions in a corresponding vector, this way looping is cut to a minimum. --- I greatly Appreciate the insight!
• bcrist: @Lemony-Andrew On #1: map::operator[] default constructs and inserts an element for the provided key if none exists. So if default construction of values is okay, the idiomatic form is just auto& value = map[key]. In the OP's case, where the value type is a raw pointer, you can just add if (!value) value = new Value to create the new value object, but that's ignoring the questionable usage of raw pointers with ownership semantics. Anyway, this method only involves 1 tree search, instead of the 3 required for a contains() and 2 operator[] calls.
• Andrew: @bcrist Alright I'll have a look at the code later today. Also were you using auto for shortening purposes to explain the code? Else I see no benefits in using it in my case. The IDE wouldn't like to work with it haha ( doesn't know the type so no friendly auto-completion )
• bcrist: @Lemony-Andrew Yes, the auto& was just for ease of reading the comment. Personally I have no problem using auto in a program that's already using other C++11 features, and Visual Studio intellisense has no problem determining the actual type of an auto, but I try to only use it where the entire type would be much harder to read; eg. auto is much easier to read than std::unordered_map<Identifier<SomeType>, SomeType, SomeTypeHasher<Identifier<SomeType>>, std::equal_to<Identifier<SomeType>>, SomeAllocatorTypedef>::const_iterator