skypjack / entt

Gaming meets modern C++ - a fast and reliable entity component system (ECS) and much more
https://github.com/skypjack/entt/wiki
MIT License
9.6k stars 844 forks source link

`entt::internal::group_handler::swap_elements` affects storages that are not owned by the group #1100

Closed Handsome-cong closed 4 months ago

Handsome-cong commented 5 months ago

When a partial-owning group relies on components owned by another group which relies on other components, sparse_set may fail to index entity because of swap_elements.

This works fine before v3.12.0, but it gives an error here currently.

namespace my_namespace
{
    struct Position
    {
        float x;
        float y;
    };

    struct Name
    {
        std::string name;
    };

    struct Health
    {
        int health;
    };

    void print_on_destroy(entt::registry& registry, entt::entity entity)
    {
        std::cout << "Component of Entity(" << entt::to_integral(entity) << ") destroyed" << std::endl;
    }
}

int main(int argc, char* argv[])
{
    auto registry = entt::registry{};
    auto name_group_with_pos = registry.group<my_namespace::Name>(entt::get<my_namespace::Position>);
    auto pos_group_with_health = registry.group<my_namespace::Position>(entt::get<my_namespace::Health>);

    auto entity_with_pos_health = registry.create();
    registry.emplace<my_namespace::Position>(entity_with_pos_health, 1.0f, 2.0f);
    registry.emplace<my_namespace::Health>(entity_with_pos_health, 100);

    auto entity_with_name_pos = registry.create();
    registry.emplace<my_namespace::Name>(entity_with_name_pos, "Entity with name and pos");
    registry.emplace<my_namespace::Position>(entity_with_name_pos, 3.0f, 4.0f);

    name_group_with_pos.each([](auto entity, auto& name, auto& position)
        {
            std::cout << "Entity(" << entt::to_integral(entity) << ") has name(" << name.name << ") and position(" << position.x << ", " << position.y << ")" << std::endl;
        });

    pos_group_with_health.each([](auto entity, auto& position, auto& health)
        {
            std::cout << "Entity(" << entt::to_integral(entity) << ") has position(" << position.x << ", " << position.y << ") and health(" << health.health << ")" << std::endl;
        });

    return 0;
}

Maybe a picture would explain what happened better: Group issue swap_elements

skypjack commented 4 months ago

Sorry for the late reply. You know, holiday break. 🙂 Can I ask you why you said that _may fail to index entity because of swap_elements_ in your original message? I don't get where swap_elements enters the game in your reasoning below. Thanks.

Handsome-cong commented 4 months ago

In the example above, when calling the each method on pos_group_with_health, entity will be retrieved from the pool of Position (through this code). At this point, an entity with a value of 1 will be retrieved. Entt will then try to use this entity to retrieve Health, which obviously cannot succeed.

skypjack commented 4 months ago

Bug confirmed. I'll push a fix upstream soon too. I'd say it's eligible for the first v3.13 patch release probably. 👍

skypjack commented 4 months ago

Fix available on branches wip and v3.13.x (next patch release for version 3.13). Let me know if it doesn't work for you. Thanks. 👍

Handsome-cong commented 4 months ago

Just tried it out and it looks like it works fine for me. Well done.

anton-kl commented 4 months ago

Also fixes the issue for me, thank you! 👍 (I had reported it on Discord with this test case: https://godbolt.org/z/joov57173)

skypjack commented 4 months ago

Fixed with version 3.13.1, thanks for pointing this out.