Upcoming changes

[skypjack]

Here is the discussion mentioned in #10 to talk about upcoming features.
The plan is to clean up the API (some methods are simply redundant) and to make it more user-friendly if possible. Moreover, performance on multi components views should be improved significantly.

Let's make a list of changes and discuss about it:

• Remove copy, it's syntactic sugar for:

  registry.accomodate<Position>(to, registry.get<Position>(from));

  accomodate creates the component if to has not it, otherwise it overwrites the existent component. One can safely use it wherever copy has been used. Finally, entity wide versions of copy is useless and error-prone and the burden to maintain it doesn't worth the price. replace can also be removed, but probably it's the sole function that make sense along with accomodate.

• Remove clone, it cannot be implemented correctly. Users should not clone an entity as a whole for they don't know to what components the entity is bound. clone should rather be a high level functionality that picks components up selectively.

• Remove swap, it's syntactic sugar for:

  std::swap(std::get<Position>(e1), std::get<Position>(e2));

  Moreover, it's confusing what should be the expected behavior. It could either swap the sole components between entities or the entities with all their data within the entity list. The result isn't the same.

• Remove the requirement of declaring components at compile-time. This:

  Registry<Position, Velocity> registry;
  // ...
  auto entity = registry.create<Velocity>();
  // ...

  Becomes this:

  Registry registry;
  // ...
  auto entity = registry.create<Velocity>();
  // ...

  The main advantage should be in terms of compilation time for large projects. The registry is included in each and every file that uses a component. On the other side, a component is included only in a few files that work with it.
  The price to pay is a slightly slower assign function. Iterations shouldn't be affected by this change.

• Rework the way multi components views are treated and bring the performance down to the ones of single components views.
  The price to pay is probably slightly slower assign and remove functions.

• Largely improve the sort functionalities, both versions.

• Introduce entity version. It will make reuse of IDs easier and safer. Entities will be represented by opaque numbers that contain both the id and the version. The system will verify the latter at each use. valid will use the version of the entity to compute a result.

• ...

Any comment and suggestion is welcome, feel free to participate and post here your thoughts.

[dbacchet]

totally agree on the copy: for a library like this usually less is more, and utility functions can be easily placed outside of the core library, reducing the maintenance effort. About removing template parameters from the Registry, that will be a huge improvement, imho. Already with ~20 components the build time is in the order of seconds (on my 16 cores workstation...) and this will also allow to forward-declare that class more easily (where it makes sense). assign and remove are usually not in the critical path, and a small slowdown there is a price I'm very happy to pay, if this leads to a cleaner system design. Thanks for what you're doing, this library has a huge potential. If you're planning to visit the Bay Area please let me know, I'd love to have a chat and show you how we are using EnTT.

[skypjack]

@dbacchet Thank you very much. If you have any suggestion of features you would like to see implemented, have a go and we can discuss them.

On branch experimental you can find new features implemented step by step. Currently I'd discourage you to use it for it contains only a huge clean up of stuff I had to remove before to start. Performance are worse than the ones on master as it stands right now. Anyway stay tuned, I'll post news on this issue each and every time I push something that works.

Bay Area? Wow, I hope I'll be there soon then!! ;-)
Actually I'm tremendously curious about your project since the first time you said you were using EnTT... I'm really glad you find it the right choice for your project!! Really appreciated.

[skypjack]

I think clone should be removed as well.
Branch experimental contains a work in progress registry that doesn't require anymore a compile-time list of components when declared.
Single component view is almost there in its final version. Multi component view has been adapted so that it compiles and works as expected. Anyway it's not in its final version and performance are far from being the expected ones.

[skypjack]

I think swap should be removed as well. It's confusing (see description above).
Branch experimental contains a work in progress where sort functionalities have been greatly improved.

[skypjack]

@dbacchet Time ago you mentioned the entity version as a wanna have feature. Well, I plan to put it in EnTT v2.0, see the last point of the list above. ;-)

[dbacchet]

great news thanks! will it still be possible to select the underlying type (uint32_t, uint64_t, etc)?

[skypjack]

Yep. It's based on traits internally. Currently I defined them for std::uint32_t (8 bit version) and std::uint64_t (24 bit version). Anyway it takes a few minutes to add other traits for other types if needed.

[dbacchet]

that's awesome, thanks!

[skypjack]

EnTT v2.0 is upstream on experimental. Any feedback is appreciated.
I've still to update in-code documentation (I plan to use doxygen for that) and the README file. That being said, all the functionalities are there and ready to use.

If you have any suggestion for other features, this is a good time to comment.
Thank you.

---

Just a side note: persistent multi component views are as fast as single component views now, that is extremely fast indeed. Enjoy!! :-)

[skypjack]

PR #14 issued.
If everything goes right, tomorrow morning I'm merging EnTT v2 on master and deleting the experimental branch.
Then I'll close the ticket and I'll start documenting a few other classes I want to put in the project the next weeks.

Feedback are welcome.

[dbacchet]

If you can wait a few days before merging I will test it in my codebase next Friday (10/20) and report any issue I find (I have a few deadlines before that, sorry). If you want to go ahead and merge instead, that's fine; we can always work on master :)

[skypjack]

@dbacchet Well, if you link to the tagged version, I'd merge anyway and add a new tag, so that you can easily switch between the two versions when you prefer. Test coverage is close to 100% and hopefully you shouldn't incur in severe bugs. For issues and/or questions I'm at your disposal as usual and I'll try to fix bugs as soon as possible if any.

[skypjack]

Added tests to get the full coverage and fixed typos in the documentation. The API is fully documented in-code now and the API reference is also available online.
I'll merge on master tomorrow morning.

[skypjack]

EnTT v2 is upstream. It's time to close the issue. I reviewed completely the crash course to introduce all the new features. I've also fully documented the API and put it online. Hope it works for you all guys. Any feedback is appreciated. Thanks for your support so far.

[skypjack]

@dbacchet Have you tested it? I'm just curious to have your feedback, suggestions and requests for features if any. :-)

[dbacchet]

Hi @skypjack, I made a first test and run into a slightly different behavior. With version 1, the only requirement for a component to be usable was to be std::move-able, but with version 2 the sparse_set requires the component to have a copy assignment operator (sparse_set.hpp,line 533). For example the following component works with ver1 but not with ver2 (it's a stripped-down version of one of my components):

struct Vehicle {
    struct Inputs {
        double steering;
        double throttle;
        double brake;
    };
    Inputs     input;  ///< vehicle input
};

struct PhysicsVehicle: public Vehicle {
    PhysicsVehicle(int32_t inner_id): px_object_id_(inner_id) {}
    PhysicsVehicle(PhysicsVehicle &&) = default;
    ~PhysicsVehicle() = default;

    int32_t   px_object_id_ = -1;
};

adding PhysicsVehicle& operator =(const PhysicsVehicle&) = default; obviously makes it compile.

In general supporting component that are movable-only would be nice, but I don't believe it's a problem right now. One of my systems has to manage non-copyable components, but I believe I can implement some workaround in case.

Just wanted to see if you were aware of this.

attached a minimal test case reproducing the problem (comment line 25 of v2/main_sample_ecs_registry.cpp to see the compilation error): entt_testcase.zip only tested in macOS Sierra.

Thanks for sharing this library!

[dbacchet]

I updated my codebase to EnTT 2.0.1, (temporarily) adding support for the copy assignment operator to the components that didn't have it. Everything compiles just fine, but I have many unit tests that are now failing, with a crash inside entt::SparseSet. The unit tests code has not been touched wrt EnTT v1.

I've not been able yet to carve a self-contained executable to reproduce the problem, but attached you can find a screenshot of a Xcode debug session with the line that is triggering the EXC_BAD_ACCESS.

as you can see from the watch panel, the size of reverse and direct is suspicious, and reverse[entt] with entt:=3 produces the crash. I will try to isolate a test case and open a proper issue asap. Sorry but I cannot share the real code :)

[skypjack]

@dbacchet

In fact, components have not the requirement to be copy constructible or copy assignable. On the other side, they must be move constructible and move assignable because of std::swap in sparse_set.hpp line 555.
Just define your PhysicsVehicle as:

struct PhysicsVehicle: public Vehicle {
    PhysicsVehicle(int32_t inner_id): px_object_id_(inner_id) {}
    PhysicsVehicle(PhysicsVehicle &&) = default;
    ~PhysicsVehicle() = default;
    PhysicsVehicle & operator=(PhysicsVehicle &&) = default;
    int32_t   px_object_id_ = -1;
};

Or even better as:

struct PhysicsVehicle: public Vehicle {
    PhysicsVehicle(int32_t inner_id): px_object_id_(inner_id) {}
    ~PhysicsVehicle() = default;
    int32_t   px_object_id_ = -1;
};

The fact is that movement assignment operator isn't generated implicitly if you declare the move constructor. However, they can be (and are indeed) implicitly generated for your class because all its parts are (let me say) moveable and you don't have to actually declare them.

I could work around it and get rid of std::swap somehow, but I guess it's an acceptable requirement indeed. If it's move constructible, I don't see any reason for which it shouldn't be move assignable.
I'm updating the documentation to specify clearly the requirement.

Can it work for you or you find it too much stringent?
It works like a charm with POD and aggregates, but it requires you to define both the move constructor and the move assignment operator in those cases where they aren't implicitly generated for some reasons (as in your example).

So, I would say: not a bug nor a regression, only something not clearly specified in the documentation.

[skypjack]

@dbacchet

The second error is interesting indeed. There are a lot of tests that stress the registry, the views and thus the sparse sets. I never ran into this kind of errors. I cannot say if it's a bug of EnTT or client side unless we reproduce it.
Just a few hints to which to think:

• EnTT isn't thread safe for itself. Users must add their own controls on top of the registry if accessed from different threads.
• DefaultRegistry used 32 bit for the entities in v1, it uses only 24 bit for them in v2. How many entities are you creating in your application? Do you need to use a Registry<std::uint64_t> instead? I added an assert in experimental to intercept the case for the users.

If you find a way to reproduce the issue I'd be glad to fix it. Otherwise, if you find the issue was due to your code, please share the information with me so that I know that's not a bug in EnTT. In both cases, feel free to ask me whatever you want, I'm at your disposal for help as usual. ;-)

--- EDIT

Not sure if you are doing it, but I suggest to run the test in debug mode. Probably the asserts put all around can help in figuring out what's going on under the hood.

[dbacchet]

about the need of a move assignment operator, I believe is totally reasonable. I should have added that one in the first place regardless, so nothing to be done there, thanks! Maybe, if you want to add in the doc that for more complex types you need to make sure to have both MyType(MyType&&); and MyType& operator =(MyType&&); defined somehow, would be great :).

About the error, it only happens inside gtest and could very well be due to some nasty bug on my side that now has been exposed. The total number of entities is less than 10, and it's all happening in a single function with no threads. Unfortunately, replacing the components with dummy types is not enough to reproduce it. I shall spend some time today to backtrack the problem in the debugger. I suppose that reverse should have at least size:=4 in this example, right? Checking who's accessing that variable is probably a good starting point. I'll let you know as soon as I find anything, thanks for your support!

[skypjack]

I've updated the documentation in experimental to specify what moveable means in this case for more complex types. Soon I'll merge it on master.

About the error, no matter if it's in EnTT or in your code, what's important is to solve it.
Of course, if you can reproduce it, I can easily look into the issue with you. Otherwise feel free to fallback on me for questions and support.
reverse has the size of the greatest entity to which you have assigned that component. As an example, if you have assigned component C to entity N, direct for C has size 1 and reverse has size N+1.

If you can reproduce the error with a couple of entities it's definitely easy to track.
Let me know, I'm curious to know what's wrong now. :-)

[dbacchet]

so while debugging the issue, I noticed something interesting. Have a look at the following picture:

I notices that the address of t1 was the same of the SparseSet in pools[1], with the effect that when I was modifying t1 the set was corrupted.

Looks like the reference to the Transform component of e1 is no more valid after I add other Transform components to other entities.

I created a self contained test case to reproduce the problem, and I'm opening a new issue right now. (Sorry if it took me a while, but I spent most of the day carving pumpkins with the kids :) )

Thanks for your help!

[skypjack]

Don't worry, I spent most of the day after my son that got a fever and is still getting out of it.
I'll try to give you a solution as soon as possible. Thank you a lot for your efforts in finding a way to reproduce the bug. Really appreciated!!