Entt: Components references are invalidated upon call to registry.assign().

[gamagan]

The file:
entt/src/entt/entity/sparse_set.hpp

has a member std::vector<object_type> instances, which is where component instances are stored.

The call to registry.assign<Component>(...) will ultimately store the components in the 'instances' member mentioned above. registry.assign() returns a reference to the component created.

The problem is that this returned reference is brittle. As soon as another call to registry.assign() is made, previously held component references are invalidated. That is because the underlying vector holding the component instances will be resized, which will move the contents to a different memory. This is bound to create extremely subtle bugs.

It should be possible to keep the reference to components around as long as the instance is alive and the component hasn't been removed.

[gamagan]

I just noticed that this issue has been mentioned before: https://github.com/skypjack/entt/issues/17

The last developer comment was "That being said, this is not a bug. It just works as intended and I cannot do much to help."

There are ways to deal with that:

• Change the underlying type in SparseSet to a non-memory-moving container, like std::list (Yes, there is a speed hit; it could be a template param to sparse-set, for trade-offs).
• Instead of storying the memory directly in the vector, store it in a std::unique_ptr, and return a '*it.get()' from the unique_ptr.
• Use a block allocator that doesn't resize memory, but just fetches blocks when it runs out of memory.
• etc

In any case, that api is way too brittle:

void fun(Registry& registry) {
  auto& comp1 = registry.assign<Comp>(id1);
  auto& comp2 = registry.assign<Comp>(id2);
  comp1.foo(); // crash
}

If you wont change the underlying SparseSet type, then you shouldn't return references to components from anywhere except while iterating views. That way, it's clear from the API that such references are error prone.

[Kerndog73]

@gamagan You should shouldn’t be holding onto component references. This is not a bug. You should be using entity IDs. Entity IDs are stable. You can hold onto them for as long as you like and they won’t become invalidated.

All of your suggestions to “fix” this will slow down this very fast library. The following example is safe. foo will not be handed invalid references.

void fun(entt::registry<> &reg) {
  reg.assign<Comp>(id1);
  auto &comp2 = reg.assign<Comp>(id2);
  foo(reg.get<Comp>(id1), comp2);
}

[skypjack]

The problem is that this returned reference is brittle.

The actual error is that you shouldn't retain references to components, the same way as you shouldn't retain references to elements in a vector.
Moreover, if a vector offers you a position (index) as a safe way to access elements, the registry offers you the entity identifier.
The parallel can continue, but you got the idea.

If you wont change the underlying SparseSet type, then you shouldn't return references to components from anywhere except while iterating views. That way, it's clear from the API that such references are error prone.

Note that ie std::vector<T>::emplace_back returns a reference to the inserted element. I wouldn't define _not clear_ a behavior just copied over from the standard library.

It should be possible to keep the reference to components around as long as the instance is alive and the component hasn't been removed.

Indeed, it shouldn't. Put aside the fact that you shouldn't do that in general in C++, references are not meant for that. Use entity identifiers instead. Moreover, pools won't be able to rearrange elements when needed and offer you the best performance in all cases if we decided to give such a strong guarantee.

There are ways to deal with that:

Let me explain why these are all the ways wrong.

Change the underlying type in SparseSet to a non-memory-moving container, like std::list (Yes, there is a speed hit; it could be a template param to sparse-set, for trade-offs).

Non contiguous memory, it would defeat one of the purposes of using an ECS and ruin performance. Moreover, the parameter should be of the registry, then pushed down to sparse sets and so on. More code to maintain (for free, and I'm not willing to maintain code I dislike for free) for no benefits.
Just use the tool the way it's meant.

Instead of storying the memory directly in the vector, store it in a std::unique_ptr, and return a '*it.get()' from the unique_ptr.

This would defeat completely the effort spent to keep components tightly packed. You would jump in memory each and every time you access a component, with an indecent loss of performance.

Use a block allocator that doesn't resize memory, but just fetches blocks when it runs out of memory.

No longer contiguous memory, the API would change (ie no more data member functions), you would miss some features (you cannot memcpy components at once) and performance would degrade, just to mention some drawbacks.
Moreover, a fully allocator-aware library is in my plans and the goal is quite different than this, therefore it would also force me to redesign it to match another allocator introduced for no benefits.

As a side note, none of the above proposals would solve the problem. A sparse set rearranges elements on destroy and references would still be invalidated in this case. So, they are just half a solution and cannot be applied this way.

Ironically, entity identifiers already manage to abstract everything and they offer even much more than what you would obtain with the approaches above mentioned (stable index, versioning, null value, and so on). So, we would also miss some other useful features.

---

EnTT already offers a way to keep track of components with stable identifiers, it sticks to the behavior and the API of the standard library and so far I haven't read in this issue of an alternative approach that would represent an improvement.

For me, an invalid request. I'm sorry.
I'll close this in a couple of days if no good reasons pop out in the meantime to convince me that this is a must have (extra) feature.

[Danvil]

I ran into a quite subtle bug in my code and it is related to this discussion. The following code will not behave correctly:

if (auto* x = reg.try_get<X>(some)) {
  reg.emplace<X>(other) = *x;
}

This also doesn't work for the same reason:

if (auto* x = reg.try_get<X>(some)) {
  reg.emplace<X>(other, *x);
}

Even something quite innocent and correct looking like this will (potentially) not work:

reg.emplace<X>(other) = reg.get<X>(some);

Even this will (potentially) not work:

reg.emplace<X>(other, reg.get<X>(some));

Note that these versions don't visibly "hold on to references" although of course it does happen under the hood.

I believe this would work however it's quite ugly:

auto& y = reg.emplace<X>(other);
y = reg.get<X>(some);

It's really not apparent to a user why the "direct" versions shouldn't work.

I understand the reasons why things are how they are, but the current situation can lead to very hard to understand bugs.
What would be your suggestion for best practice to copy a component to another entity? Is there a helper function for this?

[skypjack]

Let's consider this:

if (auto* x = reg.try_get<X>(some)) {
  reg.emplace<X>(other) = *x;
}

The equivalent with an std::vector would be:

auto &value = vec[42];
vec.emplace_back({}) = value;

It doesn't matter what the actual type of the vector is, this can fail in C++. Moreover:

Note that these versions don't visibly "hold on to references" although of course it does happen under the hood.

Your version does visibly hold on to a reference, that is x. Just because it's declared as auto * instead of auto & it doesn't mean that you aren't referencing an object in memory through its address.

What would be your suggestion for best practice to copy a component to another entity? Is there a helper function for this?

There is no helper function, even though it shouldn't be difficult to write one.
However, since you're already doing a copy here, you can construct a temporary yourself and let the system move it internally:

if (auto* x = reg.try_get<X>(some)) {
  reg.emplace<X>(other, X{*x});
}

Unfortunately, you're hitting a common problem of many contiguous containers. The language doesn't help here and its expressiveness only makes it possible to obfuscate even more what you're doing.

[Danvil]

With "not visibly holding on to references" I meant this one reg.emplace<X>(other, reg.get<X>(some));. Of course there are references at work here, it's just a bit hidden.

There is one important difference between std::vector API and the entt API. std::vector::emplace returns an iterator, and not a reference. Thus vec.emplace(vec.end(), {}) = vec[5] does not work. I would argue that this helps with not making this mistake with std::vector. std::vector::emplace_back as used in your example does return a reference though since C++11 (a mistake in my opinion...).

One might argue that the easiest way to avoid that problem is by not returning a reference from emplace. However it's debatable and your API is set, so not an option anyway. Thanks for the reply

[skypjack]

std::vector::emplace returns an iterator [...] One might argue that the easiest way to avoid that problem is by not returning a reference from emplace.

This is interesting actually. It's a breaking change and probably less user-friendly in a lot of cases but still, it would _solve_ your issue by returning an error at compile-time.
I could also make emplace _safe_ in all cases. It's so already for aggregates, since it consumes the arguments before invoking the emplace function of the vector. However, in case the type isn't an aggregate, all arguments are forwarded to the underlying function. If I constructed the type externally no matter what, the reference would be _consumed_ before getting invalidated. With a move immediately after, the performance is _almost_ the same (ctor + move ctor instead of ctor only for non-aggregate types).

In other terms, this is already safe for aggregates:

if (auto* x = reg.try_get<X>(some)) {
  reg.emplace<X>(other, *x);
}

We can make it so also for non-aggregate types probably and pay the a little price. If you're using non-aggregate types, you deserve it anyway! 😄

[Danvil]

Haha, yes, I am trying to get rid of my std::strings and std::vectors in my components, but it's a bit of a headache. Would be great if STD would have a type for "at most N elements" with N a constexpr (like std::array + size).

Back to topic: emplace has the connotation that it constructs in-place, so not sure you want to change it to ctor+move. reg.emplace(other, std::move(blah)) might start to suffer as it would do move + ctor + move instead of single move and the ctor price can be very heavy for certain undeserving non-aggregate types. Might even remove support for non-copyable types.

Another solution might be that users can get access to something like registry.reserve<Component>(n). For example in my case I know that I will now add X new components based on already existing components. Then I could first do a reserve and afterwards create+copy all my components without having to worry about memory allocations happening under the hood.

Another nice feature related to that might also be that the user can fully specify the container type used by entt. For example to do her own memory management. This can be quite useful to do for example save and load by just dumping the whole registry to a file. Maybe even different container type per component type. Sometimes you might also know you will have exactly a certain number of components of a certain type, or an upper limit is known and std::array+size works. Then the whole registry could be an aggregate type ;)

[skypjack]

Would be great if STD would have a type for "at most N elements" with N a constexpr (like std::array + size).

Well, you can easily implement it with an std::arrayand an external counter that doesn't grow more than N.

Another solution might be that users can get access to something like registry.reserve(n)

The registry already offers a reserve method, as well as a range-add functionality to assign N components to N entities at once. In the latter case, you don't need to invoke reserve because it's implicitly invoked under the hood for obvious reasons.

Another nice feature related to that might also be that the user can fully specify the container type used by entt.

Custom pools. Ironically, I finished this rework yesterday and I'm making some tests.
You can define your own storage, not only the container type. For example, if you like to use a tree to storage some components or a fixed size, preallocated array that you can't exceed, it will be possible with the next release of EnTT. :+1:

[Danvil]

Ok, that sounds great. Thank you for your work on entt!

[skypjack]

Thank you for using it @Danvil 👍 ❤️

[mattiasljungstrom]

Just ran into this issue, it might be worth to add to the documentation for emplace that doing several emplace will invalidate the references created. I think it's fairly common to do something like this: (which will be invalid)

auto entity = registry.create();
auto& c1 = registry.emplace<comp1_t>(entity);
auto& c2 = registry.emplace<comp2_t>(entity);
c1.setup()
c2.setup(c1)

Instead something like this is necessary?

auto entity = registry.create();
registry.emplace<comp1_t>(entity);
registry.emplace<comp2_t>(entity);
auto [c1, c2] = registry.get<comp1_t, comp2_t>(entity);
c1.setup()
c2.setup(c1)

If I'm wrong here, and there is a better way, please let me know! :)

[skypjack]

@mattiasljungstrom I don't think you've this problem though.

auto& c1 = registry.emplace<comp1_t>(entity);
auto& c2 = registry.emplace<comp2_t>(entity);

You're modifying different pools here. There is no reason fro which the reference for comp1_t should be invalidated if you emplace a comp2_t.

[mattiasljungstrom]

Is this also the case when I have a group setup? Like this:

registry.group<comp1_t, comp2_t>();

I'll investigate a bit more, thanks for the reply!

[skypjack]

Oh, ok, yeah, in this case the reference can get invalidated. Good point.