Run a derived class on a template function with a cloned object

[asterisk nullptr]

Hi,
I am creating a game engine and I've got stuck cloning GameObjects. GameObjects contain a map of components (Component*), a vector of children (GameObject*), and a pointer to a parent (GameObject*).

class GameObject
{
private:
  std::unordered_map<const std::type_info*, Component*> components;
  GameObject* parent;
  std::vector<GameObject*> children;
…
};

As the components are pointers, when the they also have to be manually duplicated when the GameObject is cloned. I’ve got two methods of adding components to the object both inside the GameObject class, one for creating the component and the other for adding a pre-made one that isn’t attached to any other objects:

  template<typename T>
  T* AddComponent(void)
  {
    if (components.count(&typeid(T)) > 0) 
    {
      std::cout << "Error: GameObject already has a componenet of the type: " << typeid(T).name() << std::endl;
      return nullptr;
    }
    T* obj = new T(this);
    components[&typeid(T)] = obj;
    return obj;
  }

  template<typename T>
  T* AddComponent(T* component)
  {
    if (components.count(&typeid(T)) > 0) 
    {
      std::cout << "Error: GameObject already has a componenet of the type: " << typeid(T).name() << std::endl;
      return nullptr;
    }

    components[&typeid(T)] = component;
    component->SetObject(this);

    return component;
  }

The components constructor only requires the GameObject that it will be attached to. The second ‘AddComponent’ is used when cloning objects as it retains the data from the original when passed. However, when cloning the component, it returns in the Component type, not a derived one. This causes the second plus components not to be added as the type_info already has been used. I clone the components in this format:

GameObject::GameObject(GameObject& object) 
{
  for (auto i = object.components.begin(); i != object.components.end(); i++)
  {
    AddComponent(i->second->Clone());
  }

  for (size_t i = 0; i < object.children.size(); i++)
  {
    GameObject* child = object.children[i]->Clone();
    child->SetParent(this);
  }
}
GameObject* GameObject::Clone(void)
{
  return new GameObject(*this);
}
class Component
{
Public:
  virtual Component* Clone(void) = 0;
};
class Player : public Component
{
  Player* Clone(void) { return new Player(*this); }
};

But when it is run, the error message is run. Is there a way to run the template method with the correct typename? Or am I going about this in the wrong way?
On a side note, is it possible to move template methods to the cpp file to keep the header file as clean as possible?
Thanks in Advance.

[MrHutch]

The whole type_id/info thing feels a bit cumbersome to me. Couldn't you expand your Component interface to include a type and use that has your key value?

https://ideone.com/YZpJvW
(Excuse the obvious leaks)

At least that way, it doesn't matter if you're passing around pointers to your interface - the concretions have state that easily identifies their types.

WRT templates - yes, you can put them in the source file but then you have to add explicit template specialisations as well.

[TheIdeasMan]

On a side note, is it possible to move template methods to the cpp file to keep the header file as clean as possible?

https://isocpp.org/wiki/faq/templates#templates-defn-vs-decl

[TheIdeasMan]

A couple of other things:

Avoid using new use smart pointers instead:

http://en.cppreference.com/w/cpp/memory/unique_ptr
http://en.cppreference.com/w/cpp/memory/unique_ptr/make_unique

http://en.cppreference.com/w/cpp/memory/shared_ptr
http://en.cppreference.com/w/cpp/memory/shared_ptr/make_shared


Consider a range based for loop when needing to process a whole container:

http://en.cppreference.com/w/cpp/language/range-for

Hope this helps a little bit :+)

[asterisk nullptr]

Thank you for the responses.

@MrHutch
The solution for component identification seems great.
In your example, you have GameObject as a component that can be added into someContainer. However, my implementation has GameObject as the container. Is it better practice to have a separate container? I guess it would be better for objects that don't have a visual component (eg GameManager) as it would save some RAM (not having to store a transform for all objects).

@TheIdeasMan
Thanks for the suggestions, I'll try and convert what I've got to use smart pointers and range based loops. I have had a small attempt at smart pointers but got stuck and reverted back to standard ones. I think I understand them now thanks to the sources you linked to.

Edit:
While implementing MrHutch's method of identifing objects, I found a small "issue" (more of a preference) has been noticed. When getting the component from the list a typename and string is needed, this means that there are two methods of specifying the component.

  template<typename T>
  T* GetComponent(const std::string& _component)
  {
    if (m_components.count(_component) > 0)
    {
      return static_cast<T*>(m_components[_component]);
    }
    else
    {
      return nullptr;
    }
  }

which would be called like:

GetComponent<Player>("Player");

which doesn't seem like the correct way to call it. It also allows user error as it cast to the wrong type, eg:

GetComponent<Player>("GameObject");

Any way that it can return the component in the correct derived type without having two "parameters"?

[48T7M4Gy]

@cire says:

But, hey.. keep talking out of your ass and filling the forums with . misinformation and novice code while pretending you're an expert. I'm sure you're impressing yourself.

This sort of disgraceful language, exaggeration and bullying will not be tolerated Cire. Stop it and apologise.

[TheIdeasMan]

@kemort

Wrong thread again.

[48T7M4Gy]

Its not the wrong thread pal. See the others.

[MrHutch]

The container was really just something I could set up quickly to show how the AddComponent method would work internally in this scenario.

Whether or not it's better to have a separate container is hard to say - it depends on the context of your application and how you choose to model your objects. Your pattern looks to me like a hierarchical structure where each node has a map that may or may not contain some components. It might be that this is exactly what you want or need.

In terms of the template stuff, I think the interest is possibly in the call site there. What are you doing when you call GetComponent? Why do you need to care which type of component it is?

My guess is you're doing something special based on the component. If this is the case, then something might be up with the general structure here. A couple of things come to mind:

The interface is insufficient and needs extending
We've got a method that gets a Component*. If that's the public facing API in our class, then why do we want to specialise for particular component types?

If we want different components to do different things at the call site, dynamic dispatch allows this.

For example: https://ideone.com/5y5s8p

We can retrieve the components as Component* types, call ComponentType at the call site and the output shows that we're calling into each types respective method.

The common interface isn't a good fit for the concrete types
If the dynamic dispatch above isn't suitable, then it could be that the interface (and there for your GetComponent API point) is insufficient here. Given that, aside from the method for identifying type, the commonality is the Clone method (in my opinion, a better name for your interface generally would be Cloneable or something like that), does it make sense semantically to store maps of Cloneable objects?

Even if you want both concrete types to be Cloneable, it could be that storing them all in a container as such doesn't make sense and what you actually want is stronger modelled API points for the concrete types.

Given that you're restricting to one of each type anyway, you could have something like this: https://ideone.com/3RZstb

Admittedly, in this scenario I'd also be inclined to give Player and GameObject their own interfaces. I'm a personal fan of designing by interface - it offers a lot in terms of both flexibility and testability.


There's always the option of casting at the call site. For example (assuming you've a method that returns a Component*):

auto pPlayer = dynamic_cast<Player*>(someContainer.GetComponent("Player"));
assert(pPlayer != nullptr);

However, I think this is hideous and smells of bad design. I'd personally opt to avoid it.

[asterisk nullptr]

I cant use a common interface as derived components (ICloneabe) may have additional methods that would not be able to be accessed. eg:

class ICloneable
{
public:
   virtual ICloneable* Clone() const = 0;
}

class Player : public ICloneable
{
private:
  float health;
public:
  ICloneable* Clone() const { return new Player(*this); }
  
  float GetHealth() { return health; }
  
}

ICloneable* player = new Player();
player->GetHealth(); //does not exist without casting 

I like the idea of inheriting a node and listing the components that it can have.

Class Node
{
protected:
//Any components that will likely be in a node - use a pointer so they are still optional, just
//check for nullptr before. eg
Transform* transform;
}

Class PlayerObject : public Node
{
private:
  Player* player;
}

but then we run into a new issue: components that are accessed by other nodes. eg, getting a game manager (which I have got as a component). Before it was accessed like this (ignore the lack of error checks):

FindObjectWithTag(Tag::GAMEMANAGER)->GetComponent<GameManager>()->AddPoints(GetScoreID());

I think I have a solution to this (it might also work specifying components) using a static and a virtual function:

class Node
{
public:
  virtual std::string GetType() const = 0;

   template <typename T>
   T* GetObject();
}

class PlayerObject : public Node
{
public:
  std::string GetType() const { return Type(); }
  static std::string Type() { return "Player"; }
}

template <typename T>
T* Node::GetObject()
{
  if (GetType() == T::Type())
  {
     return static_cast<T*>(this);
  }
  else
  {
     return nullptr;
  }
}

I don't know if it uses good practices (or if it works, its just an idea).

An alternative would be to store the GameManagerObject in the Game class which all nodes have access to.

class Game
{
private:
  GameManagerObject* gameManager;
public:
  GameManagerObject* GetGameManager() const { return gameManager; }
}

//in a component
//                    -node (GameManangerObject)
//                                        -component (GameManager)
GetObject()->GetGame()->GetGameMananger()->GetGameManager()->AddPoints(GetScoreID());

but this method can reduce flexibility as specified nodes have to be put into the Game class. The externally accessible nodes have to be defined before runtime.