Entt: Prototype should be moveable

Created on 4 Jun 2018  Â·  9Comments  Â·  Source: skypjack/entt

Prototype currently doesn’t have a move constructor. The compiler generated move constructor doesn’t properly move the entity. This means the “moved from” and “moved to” objects both try to registry.destroy the same entity. This results in an assertion.

A well-behaved move constructor leaves the “moved from” object in a state that can safely be destroyed without affecting the “moved to” object. In terms of Prototype, this would involve moving the handlers and registry but entity requires special treatment. entity on the “moved from” Prototype must be left in a state that can safely be deleted without affecting the “moved to” Prototype. A null entity is a good state for a “moved from” Prototype to be left in.

As for the destructor,

  1. If we want a null entity to have all of the same semantics as nullptr then destroying a null entity should be a no-op.
  2. If we want want destroying a null entity to be an error, then the Prototype destructor will have to check for null and destroy if not null.

To be honest, I’m not sure whether 1 or 2 is the way to go but it probably doesn’t matter.

Copying a Prototype is an expensive operation. Either,

  1. Copy constructor is not implemented
  2. Copy constructor is implemented
  3. Copy constructor is explicit
  4. A dedicated clone member function

I think 3 or 4. I’ve haven’t needed to copy a Prototype. I don’t want to accidentally copy a Prototype but someone might want to copy it.

enhancement

Most helpful comment

My project has no stars and it’s a bit of a mess at the moment. Not really a good example for EnTT. Thanks for the offer though!

All 9 comments

Honestly, I would = delete copy constructor and assignment operator.
Because Prototype uses an internal custom type to wrap components, allow to copy it would mean to add another static function to specialize and store along with the others. If it doesn't worth it, I'd prefer to avoid it.

My proposal:

  • Make the Prototype class move constructible and add a move assignment operator.
  • Delete all what concern copying it. We can still add that stuff in future if a lot of users ask it.

I agree. Copying is a pain to implement and probably isn’t needed.

Also, I'd prefer to have an assert in debug in case I try to destroy a null entity. It helps quite a lot while trying to debug errors. On the other side, silently destroy null entities could hide subtle bug that become even more difficult to track. My two cents.
It's a matter of putting an if in the destructor of the Prototype class, something we can easily change in future in case we find that the other solutions is somehow better for some reasons.

Sounds good to me!

See branch experimental. Does it work as expected?

My game runs without crashing and I presume the tests you wrote succeed. The code looks good. I think it's ready to merge.

Side note: if you have an open source project you want to put in the project list at the end of the README file, feel free to ping me.
Use cases for EnTT can help other users.

My project has no stars and it’s a bit of a mess at the moment. Not really a good example for EnTT. Thanks for the offer though!

Here's a GIF if you're curious

https://i.imgur.com/pPDtvzd.gif

Was this page helpful?
0 / 5 - 0 ratings

Related issues

xoorath picture xoorath  Â·  3Comments

blockspacer picture blockspacer  Â·  3Comments

skypjack picture skypjack  Â·  4Comments

markand picture markand  Â·  5Comments

skypjack picture skypjack  Â·  6Comments