Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Component polymorphism support #871

Open
wants to merge 27 commits into
base: wip
Choose a base branch
from

Conversation

zheka2304
Copy link

@zheka2304 zheka2304 commented Apr 19, 2022

Adds extension, that allows to declare and work with polymorphic components. See discussion in the delated issue #859.

Example usage (will be updated in case of changes):

#include <entt/entity/polymorphic.hpp>

struct ticking : public entt::inherit<> { // declare as polymorphic component without parents
    virtual void tick(const entt::entity) = 0;
};

class physics : public entt::inherit<ticking> { // inherit from ticking
    // ...
    void tick(const entt::entity e) override {
         // ...
    }
};

class ai : public entt::inherit<ticking> { // inherit from ticking
    // ...
    void tick(const entt::entity e) override {
         // ...
    }
};

// iterate all components, derived from ticking for given entity
for (ticking& ticking : entt::algorithm::poly_get_all<ticking>(reg, e)) {
    ticking.tick(e);
}

// try get any component, derived from ticking for given entity
if (ticking* ticking = entt::algorithm::poly_get_any<ticking>(reg, e); ticking != nullptr) {
    ticking->tick(e);
}

// iterate all components, derived from ticking in the registry
entt::algorithm::poly_each<ticking>(reg, [] (entt::entity e, ticking& c) {
    ticking.tick(e);
});

// count components, derived from ticking 
entt::algorithm::poly_count<ticking>(reg, e); // attached to entity
entt::algorithm::poly_count<ticking>(reg); // in the registry

// remove all components, derived from ticking, attached to entity
entt::algorithm::poly_remove<ticking>(reg, e); 

@skypjack skypjack self-assigned this Apr 19, 2022
@skypjack skypjack added the triage pending issue, PR or whatever label Apr 19, 2022
@codecov-commenter
Copy link

codecov-commenter commented Apr 19, 2022

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (83009ba) to head (6255e8b).
Report is 2506 commits behind head on wip.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##               wip      #871    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files          129       134     +5     
  Lines        19539     20019   +480     
==========================================
+ Hits         19539     20019   +480     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@skypjack skypjack changed the base branch from master to wip April 19, 2022 17:34
@skypjack
Copy link
Owner

Updated the base branch to wip. It fails to compile on all platforms though (yeah, the toolset v141 is always a pleasure to work with in 2022 😅).

@zheka2304
Copy link
Author

zheka2304 commented Apr 19, 2022

Updated the base branch to wip. It fails to compile on all platforms though (yeah, the toolset v141 is always a pleasure to work with in 2022 😅).

Thank you, I've reproduced this one on my machine, and I will commit the fix tomorrow

@zheka2304
Copy link
Author

zheka2304 commented Apr 20, 2022

It seems, the problem goes much deeper, than just a bug with parameter pack expansion in static assert. It looks like there are some problems with if constexpr in this toolset.

Here, when Type is a pointer, it fails to compile the else branch, that should not be compiled at all in this case.

if constexpr(std::is_pointer_v<Type>) {
    // Type and ChildType are pointers, no need for getting address
    ChildType ptr = static_cast<StorageType*>(pool)->get(entity);
    return static_cast<Type>(ptr);
} else {
    // Type is a base of ChildType, do pointer conversion
    return static_cast<Type*>(std::addressof(static_cast<StorageType*>(pool)->get(entity)));
}

However, this code compiles successfully, both for T = int and T = int*. In either case the unevaluated branch is invalid, but if constexpr works as expected here, and everything is fine.

template<typename T>
void test_if_constexpr(T a, T b) {
    if constexpr(std::is_pointer_v<T>)
        *a = *b;
    else
        a = a + b;
}

Maybe you've encountered something like this and got some ideas about the solution to this problem? The only solution I see right now is just not using if constexpr here and moving conversion logic into some helper template function with enable_if.

Copy link
Owner

@skypjack skypjack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First pass to leave some comments and questions before proceeding.
I need to review the whole thing again for sure, mainly because there are some parts that aren't that clear to me yet.

I really appreciate the effort to make it an external tool.
However, if I get this as the bare minimum, I see lot of code here and I wonder whether it's necessary and if we can reduce and simplify it.
Unfortunately I'm not (yet) into all the details, so I can't say where and how to intervene. My gut feeling is that we can shrink some stuff probably but I might be utterly wrong.

In any case, get ready for a second pass soon, according with my schedule. 👍

src/entt/entity/poly_storage_mixin.hpp Outdated Show resolved Hide resolved
src/entt/entity/poly_storage_mixin.hpp Outdated Show resolved Hide resolved

private:
template<typename... ParentTypes>
void bind_all_parent_types(basic_registry<entity_type>& reg, [[maybe_unused]] type_list<ParentTypes...>) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is contrived imho. You expect a user to want to bind all parent types but, in fact, trying to automate this as much as possible takes away the user's ability to choose.
I'm not sure this is the right way to go. The whole library is also designed on the opposite principle.
Without changing anything yet, can you explain what the burden is on the user in the other case?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

User can directly declare, what types are parent to the current component by specializing entt::direct_parent_types or entt::parent_types. These two provide complete freedom in declaring parent type list for each separate type.

So having control over this particular method only provides the ability to bind parent list, different from the one, that is already declared. However, the declared one is currently used only in this method and a bunch of static asserts, and I'm not sure, that modifying list in this method could be useful.

template<typename Entity>
class poly_pool_holder_base {
public:
inline poly_pool_holder_base(basic_sparse_set<Entity>* pool,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ignores deliberately the allocator of a sparse set and can easily break if it's different from the default one.

Copy link
Author

@zheka2304 zheka2304 Apr 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, gonna think how to fix this. Thanks for pointing this out.

Copy link
Author

@zheka2304 zheka2304 Apr 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to be a really nasty problem here. I may be wrong, but because everything within basic_sparse_set depends on a pointer type, provided by the allocator, it is required to virtualize everything, even the iteration itself, which is not viable. Maybe you've got some ideas how to avoid this?

My only thought is just banning using different allocators within the hierarchy. This can possibly be reduced to restricting using allocators with different pointer types, because the we don't directly allocate or deallocate anything, I just haven't examined it that closely at the moment. This one also does not look particularly good, but at least it is zero cost in terms of performance.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you just turn this:

template<typename Entity>
class poly_pool_holder_base

Into this:

template<typename Storage>
class poly_pool_holder_base

Then use like typename Storage::entity_type for your purposes? Dunno if it works but the runtime view does something similar to abstract the sparse set.

Copy link
Author

@zheka2304 zheka2304 Apr 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your advice. I finally got my hands on looking through runtime_view implementation. As I see, it allows to add only storages with the same allocator type. This seems to be similar to my proposal in the comment above, so I will try to implement it and see, how it works. I've also refined the idea a bit, so here it is.

In short, we restrict using pools with different pointer to entity types in the same hierarchy (the pointer type is the only thing we get from allocator, that is used in iteration/contains checks). Then we use allocator type, we got from storage_traits. We can do this at compile time for all parent types, or at runtime, only for types, which bind child pools to the poly_type.

In the compile time case, we check, that each parent of a poly type uses allocator with the same pointer type, that's all. In the runtime case the poly_type<entity_type, value_type> knows the allocator type of the storage for the value_type from storage_traits, so then we just assert type ids are equal for all bound child pools.

I will probably do a compile time variant first, as it seems more reliable and adds no cost. But runtime one has one advantage - it does not check allocator for components, that were never added.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've implemented the compile time variant, that was mentioned above, however, there are some changes. So right now it looks something like this:

  • I've added entt::poly_type_allocator<Type> to specialize allocator type for polymorpic component type, by default it uses std::allocator. This is only used as default allocator type getter, and in each place, where it is used, user can change it.
  • The default specialization of entt::type_traits for polymorphic components now uses entt::poly_type_allocator to pass allocator type to storage.
  • entt::poly_type<Entity, Type> now has new template parameter - Allocator, and so does entt::poly_type_holder, so now they operate on pools using the correct allocator type. When child pool is bound to entt::poly_type, it ensures (via static_assert), that pointer-to-entity type from the bound pool and from its own allocator type are same.
  • When bound as a child pool, the default implementation of entt::poly_storage_mixin uses entt::poly_type_allocator to provide allocator parameter to entt::poly_type, however this can be changed in user defined mixins.
  • entt::poly_types_accessor specializations now must declare allocator_type<T> type meta function, to get the allocator type for the given polymorphic component type. This is required for entt::assure_poly_type, which is used in every polymorphic algorithm (and, most likely, in user-defined ones), to pass the allocator type to the accessed entt::poly_type. The default specialization of entt::poly_type_accessor for entt::basic_registry, yet again, uses entt::poly_type_allocator for this, but for custom specializations, it gives user complete control over allocator types.

template<typename... ParentTypes>
void bind_all_parent_types(basic_registry<entity_type>& reg, [[maybe_unused]] type_list<ParentTypes...>) {
(reg.ctx().template emplace<poly_type<entity_type, ParentTypes>>().bind_child_storage(this), ...);
reg.ctx().template emplace<poly_type<entity_type, value_type>>().bind_child_storage(this);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose or use case of this? Not sure I get it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what do you mean here exactly, sorry. Are you asking, why I'm binding child storage for the value_type here?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, sorry, my fault. I meant the value_type part indeed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is done because when we are iterating through components, derived from Type, the instances of Type itself must be among them.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmm is it? This is another assumption on which I don't have control as an user and that I don't see as mandatory necessarily. 🤔

Copy link
Author

@zheka2304 zheka2304 Apr 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For your example let's assume, that there is another class, derived from circle. I want to iterate each circle and anything that derives from it, so I choose to use poly_each.

To do this, I get poly_type<entity, circle> and iterate every pool, that was bound as child to it. For this, to include instances of circle itself, the circle pool must be bound as a child pool for circle.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't it confirm that there are multiple use cases and no choice? 🤔

Copy link
Author

@zheka2304 zheka2304 Apr 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm missing your idea here, sorry. You mean the use case, when user wants to iterate over all components, derived from circle, but ignore all instances of circle itself at the same time? Am I correct here?

I mean, it is natural with abstract base class like shape, that could not be instantiated by itself, and I would want only its derived types. But in this case there just will be no pool for shape, so everything is ok. But for the case with circle I can't see any useful application of such behavior.

But I feel, that you've meant something else. If it is so, please give me examples of different possible use cases you have in mind, I will greatly appreciate it.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, sorry, I was out for the weekend and I was in a rush while trying to catch up with all the other things today.

Imagine you have shape (the base), circle and rectangle. What I end up after a while is:

  • poly_type<entity_type, shape>>
  • poly_type<entity_type, circle>>
  • poly_type<entity_type, rectangle>>

What I don't get is why the last two exist and what their purpose is. They don't have derived classes (let's make them final) and all I want is to iterate my shapes. Therefore, I can see the reason for the first poly_type while the others look useless.
Does it make sense now?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I think I understand now. To be sure: you are concerned, that while some poly types don't have any children and could be treated as a non-polymorphic ones, they still put poly_type for themselves into the context, and internally it allocates a vector of at least one element, so we still pay for having them?

Well, at the compile time we cannot be sure, that shape and circle don't have any children, if they aren't declared final. They may have derived types or they may not, we cannot tell this. Having poly_type allows us to iterate every pool, containing this type, it will work fine with or without any derived types. So having poly_type for each polymorphic type is very convenient, and, moreover, it is intuitive to have it for all types.

Of course, at runtime, we can add poly_type only for types, that have children, and then in each algorithm we can check, if we have a poly_type for given type, and, if we don't, fallback to non-polymorphic algorithm. But this solution breaks a really nice abstraction, that does not require polymorphic type data to be stored in the registry, because we will be required to use registry in the fallback case anyway. And the algorithms will become bigger and uglier, all of this for no reasonable gain.

However, I think I know how to resolve this. Instead of binding storage as a child for its own poly_type, which will internally cause heap allocation by the vector, we can store it separately in some, for example, std::optional<poly_type_holder> self_type_holder. In this case we still pay for polymorphic type with no children, compared to non-polymorphic type, but this cost is much smaller, than before, and anything else is not broken. What do you think?

* @brief Used to inherit from all given parent types and declare inheriting type polymorphic with given direct parents.
* All parent types are required to be polymorphic
* @code{.cpp}
* struct A : public entt::inherit<> {}; // base polymorphic type with no parents
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the end of the day, why I as an user should decide to inherit from a third party library class (and thus pollute my types and hierarchies) if you also give me an alternative? That is, is inherit really required or just syntactic sugar?
I can't yet wrap my mind around of it but it looks like one can just make it work without inheriting from it and this would make the whole thing more natural in a sense:

struct derived: base { /* definition */ };

This is my type, part of my application, it tells me about my intention and purpose. Why one should turn it into something like:

struct derived: entt::inherit<base> { /* definition */ };

I don't really see any reason for which I would do that as a final user. Granted, I'm a huge fan of non-invasive designe, solutions, whatever but still, I can accept an invasive approach if there is a reason and I don't understand the benefit here. Therefore I'm a bit puzzled.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

entt::inherit is just a shorter alternative for using direct_parent_types = entt::type_list<Parent1, Parent2, ...> or separate declaration with entt::direct_parent_types, so yes, it is just syntactic sugar. But for me personally, it just looks a lot nicer, so I would like to keep it just as one of several ways of declaring hierarchy.

namespace entt::internal {

template<typename Convert, typename It>
struct converting_iterator {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this iterator return? What's the use case? I can't easily see it at a first glance, sorry.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This iterator is used in algorithms, when iterating through internal vector of poly_pool_holder_base inside the poly_type and static casting it to the typed version. However, it was used before refactoring the whole thing and now I see the much shorter solution, by just using lambda instead of iterator. So it is to be removed, and thank you for making me notice it is still there.

namespace entt::internal {

template<typename Convert, typename It>
struct converting_iterator {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this iterator return? What's the use case? I can't easily see it at a first glance, sorry.



template<typename PoolsIterator>
class poly_components_iterator {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above. Thanks.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is used to iterate over all components for one entity, by iterating over all derived component pools and skipping ones, where the entity is not present. It is used in poly_get_all and poly_get_any.

* @param ent entity
* @return true, if component was removed, false, if it didnt exist
*/
inline bool remove(const Entity ent) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't find where you fill the remover_ptr but I still feel like you don't really need to.
You can delete an entity from a pointer to a sparse set. No need to wrap that functionality probably?
That is, the following should work just fine afaik:

pool_ptr->remove(ent); // or erase too, not sure what the purpose is here

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, I haven't noticed that the method called by erase is virtual.

@zheka2304
Copy link
Author

First pass to leave some comments and questions before proceeding. I need to review the whole thing again for sure, mainly because there are some parts that aren't that clear to me yet.

I really appreciate the effort to make it an external tool. However, if I get this as the bare minimum, I see lot of code here and I wonder whether it's necessary and if we can reduce and simplify it. Unfortunately I'm not (yet) into all the details, so I can't say where and how to intervene. My gut feeling is that we can shrink some stuff probably but I might be utterly wrong.

In any case, get ready for a second pass soon, according with my schedule. 👍

Thank you for reviewing, I greatly appreciate your effort. I've replied to the comments above, and uploaded quick fixes for some of them. However there are still some things to be done and to discuss, so I am working on it. Looking forward for the second pass :)

Copy link
Owner

@skypjack skypjack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments but it's also half past midnight, so let's settle for now. 😅

template<typename... ParentTypes>
void bind_all_parent_types(basic_registry<entity_type>& reg, [[maybe_unused]] type_list<ParentTypes...>) {
(reg.ctx().template emplace<poly_type<entity_type, ParentTypes>>().bind_child_storage(this), ...);
reg.ctx().template emplace<poly_type<entity_type, value_type>>().bind_child_storage(this);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, sorry, I was out for the weekend and I was in a rush while trying to catch up with all the other things today.

Imagine you have shape (the base), circle and rectangle. What I end up after a while is:

  • poly_type<entity_type, shape>>
  • poly_type<entity_type, circle>>
  • poly_type<entity_type, rectangle>>

What I don't get is why the last two exist and what their purpose is. They don't have derived classes (let's make them final) and all I want is to iterate my shapes. Therefore, I can see the reason for the first poly_type while the others look useless.
Does it make sense now?

* @return Returns reference to Type, converted from a given pointer. Will return pointer instead of reference, if parent type is a pointer.
*/
inline pointer_type try_get(const Entity ent) ENTT_NOEXCEPT {
return pointer_type(this->getter_ptr(this->pool_ptr, ent));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The basic sparse set also has an opaque getter that accepts an entity and returns a void *. Isn't it what you want here?

Copy link
Author

@zheka2304 zheka2304 Apr 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one handles correct pointer conversion between a pair of child and parent types, so it behaves correctly in case of multiple inheritance. It also handles conversion for pointer-like polymorphic components. Inside, getter_ptr calls non-virtual basic_storage::get to avoid double virtual call.


/** @copydoc try_get */
inline const_pointer_type try_get(const Entity ent) const ENTT_NOEXCEPT {
return const_cast<poly_pool_holder<Entity, Type>*>(this)->try_get(ent);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

Copy link
Owner

@skypjack skypjack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of comments. The best I can do on Sunday. Sorry. 😅
My only concern is that I feel this is getting pretty big for the feature it wants to introduce. Therefore, I want to get some time to see why and where this is happening so as to reduce it to something acceptable.
My gut feeling is that it's partly due to trying to tackle some corner cases for which not compiling would already be enough but take this with a grain of salt. 👍

* @tparam Type polymorphic component type to convert into
* @tparam Allocator allocator type of the pool
*/
template<typename Entity, typename Type, typename Allocator>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this the same as using type Storage, then taking typename Storage::entity_type and the others (ie value_type and allocator_type) from it?
It would also vastly simplify the definition at a first glance.

Copy link
Author

@zheka2304 zheka2304 May 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but poly_type has to be created for types, that could not have storage type (most likely ones with pure virtual methods). The possible workaround here, that I see right away, is to create storage_traits specialization for polymorphic types, that cannot have a storage type, with some placeholder storage type, that only provides entity_type, value_type and allocator_type. Then, the storage_traits could be used to access polymorphic types by pair of entity and component type.

But it looks like, this could possibly break something in the abstraction of polymorphic algorithms from registry, that I cannot see right away. Also, this will not simplify things for the end user at all, because he would still be required to declare custom allocator type in any case. And either it will not make life a lot simpler inside the implementation, because all types for poly_type, except the component type itself, is acquired from the poly_types_accessor anyway.

* @tparam Type polymorphic component type
* @tparam Allocator allocator type for all bound contained pool holders
*/
template<typename Entity, typename Type, typename Allocator>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is similar to poly_pool_holder and I wonder why we don't get the storage type directly rather than a bunch of things that can quickly go out of sync.

@zheka2304
Copy link
Author

Some of the builds have failed again, I will try reproduce it and commit the fix, as it will be ready, sorry.

@zheka2304
Copy link
Author

Hello, I'm very sorry for disappearing, I unfortunately had a very little free time. However now I am ready to continue the discussion and further improvement.

Everything seems to build properly, but codecov now does something strange, both in polymorhic.cpp and even view.cpp that was not affected at all by my changes, do you have any idea what is happening?

@skypjack
Copy link
Owner

The one in view.cpp is due to an error of mine that is already fixed on wip, so don't worry. As for the other one, it's in a new file, so very likely to be fixed.
I'll try to find some time for another review during the next couple of days. 👍

@PaulJohnson1
Copy link

PaulJohnson1 commented May 21, 2022

Hey, I was testing some stuff out on the experimental branch of the fork, and I tested for memory leaks using valgrind. I noticed a memory leak when you emplace a base component and a derived component on the same entity. It leaked 81,920 bytes in 1 block according to valgrind.
image
Here is my code if you would like to see the memory leak in action. I have put a comment above the faulty lines. I am thinking that an assertion would be the best way to prevent this. I do not know how the codebase works, this is just a suggestion.

#include <cstring>
#include <iostream>

#include "entt/src/entt/entity/registry.hpp"
#include "entt/src/entt/entity/polymorphic.hpp"

class CameraComponent : public entt::inherit<>
{
};

class SendableCameraComponent : public entt::inherit<CameraComponent>
{
};

int main()
{
    std::cout << "format" << std::endl << "testHasCameraComponent test2HasCameraComponent" << std::endl << "testHasCameraEntity    test2HasCameraEntity" << std::endl << std::endl;

    entt::registry registry;
    entt::entity test = registry.create();
    entt::entity test2 = registry.create();

    registry.emplace<CameraComponent>(test);
    registry.emplace<SendableCameraComponent>(test2);

    std::cout << "test has CameraComponent; test2 has SendableCameraComponent" << std::endl;
    std::cout << std::to_string(registry.all_of<CameraComponent>(test)) << " " << std::to_string(registry.all_of<CameraComponent>(test2)) << std::endl;
    std::cout << std::to_string(registry.all_of<SendableCameraComponent>(test)) << " " << std::to_string(registry.all_of<SendableCameraComponent>(test2)) << std::endl;

    std::cout << std::endl;
    std::cout << "deleting CameraComponent from test2" << std::endl;

    registry.erase<CameraComponent>(test2);

    std::cout << std::to_string(registry.all_of<CameraComponent>(test)) << " " << std::to_string(registry.all_of<CameraComponent>(test2)) << std::endl;
    std::cout << std::to_string(registry.all_of<SendableCameraComponent>(test)) << " " << std::to_string(registry.all_of<SendableCameraComponent>(test2)) << std::endl;

    std::cout << std::endl;
    std::cout << "adding SendableCameraComponent; removing SendableCameraComponent" << std::endl;

    registry.emplace<SendableCameraComponent>(test2);
    registry.erase<SendableCameraComponent>(test2);

    std::cout << std::to_string(registry.all_of<CameraComponent>(test)) << " " << std::to_string(registry.all_of<CameraComponent>(test2)) << std::endl;
    std::cout << std::to_string(registry.all_of<SendableCameraComponent>(test)) << " " << std::to_string(registry.all_of<SendableCameraComponent>(test2)) << std::endl;

    std::cout << std::endl;
    std::cout << "adding CameraComponent; adding SendableCameraComponent" << std::endl;
  
    // MEMORY LEAK!
    registry.emplace<CameraComponent>(test2);
    registry.emplace<SendableCameraComponent>(test2);

    std::cout << std::to_string(registry.all_of<CameraComponent>(test)) << " " << std::to_string(registry.all_of<CameraComponent>(test2)) << std::endl;
    std::cout << std::to_string(registry.all_of<SendableCameraComponent>(test)) << " " << std::to_string(registry.all_of<SendableCameraComponent>(test2)) << std::endl;

    std::cout << std::endl;
    std::cout << "removing SendableCameraComponent" << std::endl;
    
    registry.erase<SendableCameraComponent>(test2);

    std::cout << std::to_string(registry.all_of<CameraComponent>(test)) << " " << std::to_string(registry.all_of<CameraComponent>(test2)) << std::endl;
    std::cout << std::to_string(registry.all_of<SendableCameraComponent>(test)) << " " << std::to_string(registry.all_of<SendableCameraComponent>(test2)) << std::endl;

    std::cout << std::endl;
    std::cout << "remove all components from all tests" << std::endl;

    registry.erase<CameraComponent>(test);
    registry.erase<CameraComponent>(test2);

    std::cout << std::to_string(registry.all_of<CameraComponent>(test)) << " " << std::to_string(registry.all_of<CameraComponent>(test2)) << std::endl;
    std::cout << std::to_string(registry.all_of<SendableCameraComponent>(test)) << " " << std::to_string(registry.all_of<SendableCameraComponent>(test2)) << std::endl;

    registry.destroy(test);
    registry.destroy(test2);
}

@PaulJohnson1
Copy link

I have also noticed, while testing stuff, that it has no support for calling the base component class with parameters. Am I missing something? Is there not a way to do that currently?

@zheka2304
Copy link
Author

Hello, and sorry for a late reply! I'm really glad that you've tried and tested my extension to entt.

Hey, I was testing some stuff out on the experimental branch of the fork, and I tested for memory leaks using valgrind. I noticed a memory leak when you emplace a base component and a derived component on the same entity. It leaked 81,920 bytes in 1 block according to valgrind.

I see that you are using my first prototype, which was considered too complicated and introduced too many changes. Currently discussed implementation is located in the polymorpic branch and I highly recommend to try it instead.

The old implementation, that you've used, internally allocates blocks of memory to store tightly packed reference lists, those are stored statically, and it seems, that I've forgot to free them, when program terminates. This is the most probable source of this leak.

I have also noticed, while testing stuff, that it has no support for calling the base component class with parameters. Am I missing something? Is there not a way to do that currently?

Sorry, I'm not getting the idea here, can you please provide an example?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage pending issue, PR or whatever
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants