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

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

Open
pgruenbacher opened this issue May 9, 2024 · 2 comments
Open

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

pgruenbacher opened this issue May 9, 2024 · 2 comments
Assignees
Labels
feature request feature request not yet accepted solved available upstream or in a branch

Comments

@pgruenbacher
Copy link
Sponsor Contributor

pgruenbacher commented May 9, 2024

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;
        } 
@skypjack skypjack self-assigned this May 10, 2024
@skypjack skypjack added the feature request feature request not yet accepted label May 10, 2024
@skypjack
Copy link
Owner

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 skypjack added the solved available upstream or in a branch label May 10, 2024
@pgruenbacher
Copy link
Sponsor Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request feature request not yet accepted solved available upstream or in a branch
Projects
None yet
Development

No branches or pull requests

2 participants