entt

entt copied to clipboard

Hi @skypjack,

as discussed in Discord I'm proposing the following code to add dynamic (runtime) sized object support to entt.

I'm looking forward to your feedback.

---

Points to discuss

There is also the possibility to move the methods from the traits objects into the stateful allocator, but I thought it would be much cleaner to keep the responsibility of the allocator and the storage related methods separate.

ToDo

• [ ] Implement missing tests
• [ ] Review whether it is intended that move_element can alias (the element to swap from and to can point to the same object by the storage algorithms): e.g. [[maybe_unused]] auto unused = std::exchange(elem, std::move(other)); in basic_storage::pop.
• [ ] Review the polymorphic allocator workaround
• [x] Discuss heavily copying of allocators in the storage class (before this PR), those should be passed as lvalue reference. Would be the possibility for a follow-up commit.

---

Usage

entt::registry registry;

    // Create a dynamic trait object
    entt::component_traits<dynamic_data> const traits(32);
    ASSERT_EQ(traits.dynamic_size(), 32);

    // Create an allocator which supplies objects of the dynamic size
    dynamic_allocator<dynamic_data> const alloc{traits.dynamic_size()};

    // Create the storage for the dynamic sized data,
    // and assert that it was created with the specified parameters.
    ASSERT_TRUE(registry.try_create_storage<dynamic_data>(alloc, traits));

    auto &storage = registry.storage<dynamic_data>();

    // Ensure that the allocator matches the provided one
    ASSERT_EQ(storage.get_allocator(), alloc);

    // Ensure that the trait matches the provided one
    ASSERT_EQ(storage.get_traits(), traits);

    entt::entity const e1 = registry.create();
    entt::entity const e2 = registry.create();

    {
        dynamic_data &d1 = registry.emplace<dynamic_data>(e1);
        dynamic_data &d2 = registry.emplace<dynamic_data>(e2);

Codecov Report

Merging #1005 (b61952d) into wip (d534fad) will decrease coverage by 0.06%. The diff coverage is 92.52%.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff             @@
##               wip    #1005      +/-   ##
===========================================
- Coverage   100.00%   99.94%   -0.06%     
===========================================
  Files          142      142              
  Lines        23546    23675     +129     
===========================================
+ Hits         23546    23662     +116     
- Misses           0       13      +13     

Impacted Files	Coverage Δ
test/entt/entity/sigh_mixin.cpp	100.00% <ø> (ø)
test/entt/entity/registry.cpp	99.42% <89.02%> (-0.58%)	:arrow_down:
src/entt/entity/registry.hpp	99.71% <93.33%> (-0.29%)	:arrow_down:
src/entt/entity/storage.hpp	99.22% <95.65%> (-0.78%)	:arrow_down:
src/entt/core/memory.hpp	100.00% <100.00%> (ø)
src/entt/entity/mixin.hpp	100.00% <100.00%> (ø)

@skypjack From my side the initial step on this PR is finished. I would add any missing unit tests or changes after receiving your feedback.

Currently there are two main points that would require a review:

• The workaround for polymorphic allocators which is needed because std::polymorphic_allocator::construct (at least on MSVC) does not propagate this correctly prior to C++20.
• I noticed that basic_storage::pop calls std::exchange and therefore traits::move_element on the same both elements (the objects can alias). While this is theoretically valid it would be better if those aliasing calls are not made by the algorithm in the first place. There is also a very small chance that this behaviour is caused by my changes.

I'm looking forward to your feedback.

@skypjack Do you have any news about this PR? Would you prefer any possible changes to it? I have updated it to the latest wip branch commit and improved the previously mentioned allocator copying.

@skypjack Do you have any news about this PR? Would you prefer any possible changes to it? I have updated it to the latest wip branch commit and improved the previously mentioned allocator copying.

Oh, sorry. I saw the todo list with some unchecked bullets yet and thought you might want to continue before reviewing. I'll look into it as soon as possible then and drop my thoughts here. I already had a look after the first commits to be honest and I must admit that very invasive changes to key library classes (like the storage) to support minor feature don't drive me crazy. I expected something less... impactful, maybe? You know, EnTT is entirely designed to offer hook points and build up from the outside. Just look at the signal support for example, which in itself is an external class and not embedded in storage. Just out of curiosity, have you tried this approach already? If yes, what problems stopped you and made you turn towards a built-in solution for the storage?

@skypjack no problem for me. Meanwhile I had some time to improve and test the PR.

I see your overall point, the changes are quite intrusive and do not go in hand with the current mix and match design of EnTT to support a specific single feature. However, I made the changes as they are currently proposed because I think that supporting dynamic component sizes is probably a first level feature you would like to support.

Theoretically, we could also make the change less intrusive to the current implementation by moving much more logic into the storage_base<...>::container_type which we would need to make adaptable (maybe as template and runtime parameter to storage_base itself). Through this way, the storage_base could be merely a facade to connect the sparse_set and the underlying component storage (container_type aka std::vector<typename alloc_traits::pointer, ...>). Eventually this might remove the need to add entire new specializations of storage_base entirely for the other cases like page_size == 0 or is_empty_v<Component>.

Currently, I think what mostly speaks against an entire new storage_base specialization is that it would share a lot of logic and code with the other specializations. Therefore it might be a good idea to keep most of the logic into a class that acts like a facade (like storage_base) to a more lower specialized component storage (like container_type) as proposed above.

I cannot think in other different ways to implement this feature as the one proposed in this PR or the other one proposed in this reply. To efficiently support dynamically sized components we need access to the underlying storage and offset computation and therefore an external solution like signals is impossible.

However, I made the changes as they are currently proposed because I think that supporting dynamic component sizes is probably a first level feature you would like to support.

For sure. Like supporting empty types or entity storage with special abilities. Yet, in both cases, this was through specialziations. So I was wondering why dynamic type support can't go the same way. I haven't stopped to think about it for long, so I might not see all the problems. However, something like storage<dynamic<32>> seems doable. What drawbacks does it have? Maybe you've already thought about it or tried it and you know it better than me.

@skypjack Thank you for your comprehensive feedback.

---

When I tell the storage to destroy an object, just pray that I call ~U() on it first to avoid problems because the storage (rightfully) doesn't know anything about my U

I think this a misconception: The allocator is responsible for construction and destruction and can be adapted to the stored object. Therefore it is entirely responsible for the objects lifetime and the objects constructor and destructor are always called. This is simply not the case in the test code which is currently only specialized for primitive destructible objects.

By moving the responsibility to the allocator, various object types can be supported. For instance also dynamically generated and reflected objects.

---

For sure. Like supporting empty types or entity storage with special abilities. Yet, in both cases, this was through specialziations. So I was wondering why dynamic type support can't go the same way.

There are no technical limitations I'm aware of. I chose to modify the existing storage to reduce code duplication.

The big issue I see is that the storage is mostly a facade to the underlying container, therefore it probably makes sence to specialize the container rather than the storage. I will look into this. The reason why the trait is made staeful in this PR is that we need an object that can be specialized for the component type. And the trait itself (if we ignore its naming) fits the role perfectly because it effectively specialized the storage based on the component type already (page size, empty type). Why not use it for any other use cases where the storage needs to be specialized?

There are no technical limitations I'm aware of.

I'd like to give it a try when the whole implementation is polished and let's say easier to port. I strongly prefer supporting things as mixin or external feature (see the signal support) if possible, for a countless number of reasons.

The big issue I see is that the storage is mostly a facade to the underlying container, therefore it probably makes sence to specialize the container rather than the storage.

No idea of what you mean actually. Sorry. Can you explain it?

The reason why the trait is made staeful in this PR is that we need an object that can be specialized for the component type. And the trait itself (if we ignore its naming) fits the role perfectly because it effectively specialized the storage based on the component type already (page size, empty type). Why not use it for any other use cases where the storage needs to be specialized?

Well, what I see is: I want to pass a few compile-time values and a couple of runtime ones. This use of a traits class leaks everywhere because of the latter. On the other hand, a storage can offer a setter function to pass the same values. Consider that all storage classes in a registry are freely available to the final user now, so you don't really have any limitations here.

Closed as starving/no longer viable.