entt icon indicating copy to clipboard operation
entt copied to clipboard

storage<entt::entity>("id"_hs) should be asserted against

Open pgruenbacher opened this issue 1 year ago • 2 comments

Looking at registry

    template<typename Type>
    [[nodiscard]] auto &assure([[maybe_unused]] const id_type id = type_hash<Type>::value()) {
        if constexpr(std::is_same_v<Type, entity_type>) {
            return entities;
        } 

Seems to indicate that id_type id is unused and that reg.storageentt::entity("custom_id"_hs) just simply returns the single entities which can be misleading behavior.

I suggest at the very least

    template<typename Type>
    [[nodiscard]] auto &assure([[maybe_unused]] const id_type id = type_hash<Type>::value()) {
        if constexpr(std::is_same_v<Type, entity_type>) {
            ENTT_ASSERT(id == type_hash<Type>::value(), "id cannot be used for entt::entity storage);
            return entities;
        } 

or allow for custom entt::entity_type storage so that users can have various id<->id sets i.e. auto parentId = reg.storage<entt::entity>("parent"_hs).get(childId)

    template<typename Type>
    [[nodiscard]] auto &assure([[maybe_unused]] const id_type id = type_hash<Type>::value()) {
        if constexpr(std::is_same_v<Type, entity_type> && id == type_hash<Type>::value()) {
            return entities;
        } 

pgruenbacher avatar May 09 '24 20:05 pgruenbacher

I think that asserting is the right thing to do here. In fact, there is a problem with this:

or allow for custom entt::entity_type storage so that users can have various id<->id sets

Entity storage works differently. It's meant to generate entities and recycle them. It never destroys its elements. Instead, it moves them around and increases their versions. An entt::storage<entt::entity> isn't an id-to-id mapping. Quite the opposite actually. It picks a storage specialization with a completely different goal. You can still have id-to-id mappings but you've to use boxed types for that. This is the drawback of managing the entities with a storage type. I think it's worth it because of all the pros but 🤷‍♂️ you know, all solutions have their pros and cons at the end of the day.

So, yeah, I think I'll add an assert to avoid errors and that's it. Let me know your thougts too. 👍

skypjack avatar May 10 '24 07:05 skypjack

yea I agree ASSERT is the best option for now. i think entt::storage<ENTT_ID_TYPE> / entt::storage<uint32_t> still works just fine anyways it just means there's lots of casting. or yea boxed types works too

pgruenbacher avatar May 10 '24 12:05 pgruenbacher