3

I have an entity class which makes use of a vector of unique pointers. For some reason it is giving the following error:

Error log:

In file included from /usr/include/c++/8/vector:62,
                 from ./RactorUtils.h:8,
                 from ./OpenGLRenderer.h:4,
                 from ./TestGame.cpp:2:
/usr/include/c++/8/bits/stl_construct.h: In instantiation of ‘void std::_Construct(_T1*, _Args&& ...) [with _T1 = std::unique_ptr<IComponent>; _Args = {const std::unique_ptr<IComponent, std::default_delete<IComponent> >&}]’:
/usr/include/c++/8/bits/stl_uninitialized.h:83:18:   required from ‘static _ForwardIterator std::__uninitialized_copy<_TrivialValueTypes>::__uninit_copy(_InputIterator, _InputIterator, _ForwardIterator) [with _InputIterator = __gnu_cxx::__normal_iterator<const std::unique_ptr<IComponent>*, std::vector<std::unique_ptr<IComponent> > >; _ForwardIterator = std::unique_ptr<IComponent>*; bool _TrivialValueTypes = false]’
/usr/include/c++/8/bits/stl_uninitialized.h:134:15:   required from ‘_ForwardIterator std::uninitialized_copy(_InputIterator, _InputIterator, _ForwardIterator) [with _InputIterator = __gnu_cxx::__normal_iterator<const std::unique_ptr<IComponent>*, std::vector<std::unique_ptr<IComponent> > >; _ForwardIterator = std::unique_ptr<IComponent>*]’
/usr/include/c++/8/bits/stl_uninitialized.h:289:37:   required from ‘_ForwardIterator std::__uninitialized_copy_a(_InputIterator, _InputIterator, _ForwardIterator, std::allocator<_Tp>&) [with _InputIterator = __gnu_cxx::__normal_iterator<const std::unique_ptr<IComponent>*, std::vector<std::unique_ptr<IComponent> > >; _ForwardIterator = std::unique_ptr<IComponent>*; _Tp = std::unique_ptr<IComponent>]’
/usr/include/c++/8/bits/stl_vector.h:463:31:   required from ‘std::vector<_Tp, _Alloc>::vector(const std::vector<_Tp, _Alloc>&) [with _Tp = std::unique_ptr<IComponent>; _Alloc = std::allocator<std::unique_ptr<IComponent> >]’
./ECS/ECS.h:42:7:   required from here
/usr/include/c++/8/bits/stl_construct.h:75:7: error: use of deleted function ‘std::unique_ptr<_Tp, _Dp>::unique_ptr(const std::unique_ptr<_Tp, _Dp>&) [with _Tp = IComponent; _Dp = std::default_delete<IComponent>]’
     { ::new(static_cast<void*>(__p)) _T1(std::forward<_Args>(__args)...); }
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /usr/include/c++/8/memory:80,
                 from ./ECS/ECS.h:6,
                 from ./RactorUtils.h:23,
                 from ./OpenGLRenderer.h:4,
                 from ./TestGame.cpp:2:
/usr/include/c++/8/bits/unique_ptr.h:394:7: note: declared here
       unique_ptr(const unique_ptr&) = delete;
       ^~~~~~~~~~

Entity class

class IEntity{
private:
    bool active = true;
    vector<unique_ptr<IComponent>> components;
    ComponentArray componentArray;
    ComponentBitset componentBitset;

public:
    const char* name;

    template<typename T> bool hasComponent(){
        return componentBitset[getComponentTypeID<T>()];
    }

    template<typename T> T& addComponent(){
        T c;
        unique_ptr<T> uPtr(&c);
        components.emplace_back(move(uPtr));

        componentArray[getComponentTypeID<T>()] = &c;
        componentBitset[getComponentTypeID<T>()] = true;

        return *move(uPtr);
    }

    template<typename T> T& getComponent() const{
        auto ptr(componentArray[getComponentTypeID<T>()]);
        return *static_cast<T*>(ptr);
    }
};

I know that unique pointers can't be copied but I can't figure out where I am copying it.

Edit: I actually got where it was getting copied by adding the following code to IEntity:

explicit IEntity();
IEntity(const IEntity&) = delete;
IEntity& operator= (const IEntity&) = delete;
~IEntity() = default;

But now it is showing error at a function given below:

//Prototype
void UniformCaller(void (*ptr)(IEntity object, Shader shader), IEntity object, Shader shader);

//Call
void Render(IEntity object, glm::mat4x4 projectionMatrix, glm::mat4x4 viewMatrix){
Shader* shader = object.getComponent<MeshRenderer>().shader;
UniformCaller(shader->uniform_callback, object, *shader);
}

//Definition
void UniformCaller(void (*ptr)(IEntity object, Shader shader), IEntity object, Shader shader){
    (*ptr) (object, shader);
}

Edit2: As pointed in comments I changed all of them to references and now I get a wierd error as follows:

/usr/bin/ld: /tmp/ccPzMwMa.o: in function `ComponentManager::createComponent(IComponent*)':
TestGame.cpp:(.text._ZN16ComponentManager15createComponentEP10IComponent[_ZN16ComponentManager15createComponentEP10IComponent]+0x41): undefined reference to `ComponentManager::components'
/usr/bin/ld: /tmp/ccPzMwMa.o: in function `EntityManager::addEntity()':
TestGame.cpp:(.text._ZN13EntityManager9addEntityEv[_ZN13EntityManager9addEntityEv]+0x6c): undefined reference to `EntityManager::entities'
collect2: error: ld returned 1 exit status

addEntity():

static IEntity& addEntity(){
        IEntity* e = new IEntity();
        unique_ptr<IEntity> uPtr(e);
        entities.emplace_back(move(uPtr));
        return *move(uPtr);
}

createComponent():

static IComponent& createComponent(IComponent* component){
        unique_ptr<IComponent> ptr(component);
        components.emplace_back(move(ptr));
        return *move(ptr);
}
SarojKr
  • 65
  • 3
  • 12
  • 3
    You are trying to copy `components` somewhere. Namely in line 42 of `ECS.h`, as the error message says. Which doesn't appear to be in the code you show. – Max Langhof Dec 04 '19 at 09:10
  • 2
    Well, the copy is not in the code you've shown us. Also your `addComponent` function is broken. First of all you take reference to locally declared `c`, your pointers are invalid after the function exits. Secondly you use `move` twice. And thus you dereference `nullptr` at the end (segfault). – freakish Dec 04 '19 at 09:11
  • @MaxLanghof Line 42 is the declaration of IEntity. – SarojKr Dec 04 '19 at 09:16
  • 1
    @SarojKr Add `IEntity(const IEntity&) = delete;` and `IEntity& operator=(const IEntity&) = delete;`, then see what the errors say. – Max Langhof Dec 04 '19 at 09:20
  • @MaxLanghof Add it where? – SarojKr Dec 04 '19 at 09:24
  • 1
    @SarojKr To the `IEntity` class. It explicitly deletes copy construction and copy assignment. The compiler should then give you better errors regarding where you are trying to copy an `IEntity`, rather than explain in-depth why it can't do that. (Although in a perfect world the compiler would do that without manually coaxing it into that.) – Max Langhof Dec 04 '19 at 09:25
  • @MaxLanghof It is giving `No matching constructor for initialization of 'IEntity'` at `IEntity(const IEntity&) = delete;` – SarojKr Dec 04 '19 at 09:32
  • 1
    @SarojKr Yes, that's what was expected. You are trying to copy `IEntity` somewhere and the compiler says "you can't do that". But _where_ does it give that error? I thought that's what you wanted to know! – Max Langhof Dec 04 '19 at 09:34
  • @SarojKr Show us the code around line 42 in file ECS/ECS.h – ProXicT Dec 04 '19 at 10:04
  • @ProXicT It is where the shown code starts. – SarojKr Dec 04 '19 at 10:06
  • Oh, I see... Well, somewhere in your code you're copying `IEntity`. Do you have a function somewhere in your code that returns `IEntity`? Or some array of `IEntity`? – ProXicT Dec 04 '19 at 10:09
  • Looking at your edit, you're passing `IEntity` by value, that's the point where the copy happens. You have to pass the `IEntity` by a reference or by a pointer. – ProXicT Dec 04 '19 at 10:13
  • @ProXicT Yes, I do have a manager with an vector of unique_ptr to IEntity and a function that returns a reference to IEntity. – SarojKr Dec 04 '19 at 10:14
  • Ok, got it... I think for your particular case, just make this `IEntity` class copyable, that might be easier for you now. Remove the code you've added in your edit and use `std::shared_ptr` instead of `std::unique_ptr` in the `components` vector. – ProXicT Dec 04 '19 at 10:17
  • Seeing your new edit, it looks like you've changed the prototypes without changing the definitions of the functions or vice-versa. Also, in the `addEntity()` function, you're calling `std::move()` twice on one object. Just do `return *e;` instead of `return *move(uPtr)`. The same applies to the `createComponent()` function. – ProXicT Dec 04 '19 at 10:34
  • @ProXicT Thanks for the help but the error in edit2 still exists while the variables the compiler call "undefined" are all defined correctly. – SarojKr Dec 04 '19 at 10:37
  • 1
    @SarojKr Good, we found the place where you copy `IEntity`. It's when you pass them by value (which naturally results in copies). For your latest error, see https://stackoverflow.com/questions/12573816/what-is-an-undefined-reference-unresolved-external-symbol-error-and-how-do-i-fix. – Max Langhof Dec 04 '19 at 10:46
  • This is getting too broad, fixing one problem leads to another one appearing. This error happening in your second edit is occurring during linkage, not during compilation. As the error suggests, the problem is somewhere in a class unkown to us - `ComponentManager` in function `createComponent()`. The error says it can't find a symbol `ComponentManager::components`. But maybe you should create a new question for this. – ProXicT Dec 04 '19 at 10:48
  • 1
    @ProXicT OK I will make another question for this. – SarojKr Dec 04 '19 at 10:50

1 Answers1

4

There are numerous problems with your code. First of all, unique_ptr is used to delete a heap allocated memory. You're creating your T instance on stack, taking an address to it and feeding this address to the unique_ptr - this is wrong and will result in a crash when the deleter is called.

Second of all, as it was pointed out in the comments, you're storing a reference to a local variable, which will die when the function returns, so you'll end up with a dangling reference.

Also, you're calling std::move twice on one std::unique_ptr, this is wrong since after the first std::move call, the pointer will be nullptr.

To fix these issues, allocate the T on heap rather than on stack:

template<typename T> T& addComponent() {
    T* c = new T(); // Normally, you'd want to avoid this (naked new) but since
                    // we don't know what componentArray is, we can't know why
                    // you need to store the raw pointer, too.
    components.emplace_back(c);
    componentArray[getComponentTypeID<T>()] = c;
    componentBitset[getComponentTypeID<T>()] = true;

    return *c;
}

Now the error you're presenting us with happens to be somewhere in a code you're not showing us; you're trying to copy components - std::vector<std::unique_ptr<T>>, which suggests you're trying to copy your whole class. Nonetheless, you can fix this issue by using std::shared_ptr<T> instead of std::unique_ptr<T>, but that's just a quick and dirty solution. You should really find where you're trying to take copy of your IEntity class and figure out if there's a way to get rid of this unwanted copy.

ProXicT
  • 1,903
  • 3
  • 22
  • 46