xissburg / edyn

Edyn is a real-time physics engine organized as an ECS.
MIT License
630 stars 26 forks source link

add on_destroy_aabb in broadphase to allow detaching/changing shapes of a rigidbody. #125

Closed volcoma closed 7 months ago

volcoma commented 8 months ago

Suppose you want to remove the shape of an entity at runtime

    entity.remove<edyn::box_shape>();
    entity.remove<edyn::shape_index>();
    entity.remove<edyn::AABB>();
    entity.remove<edyn::collision_filter>();

then the broadphase should listen to the on_destroy as well

broadphase::broadphase(entt::registry &registry)
    : m_registry(&registry)
{
    registry.on_construct<AABB>().connect<&broadphase::on_construct_aabb>(*this);
    registry.on_destroy<AABB>().connect<&broadphase::on_destroy_aabb>(*this);

i suggest changing the structure of the m_new_aabb_entities to something like

// entity, added/removed
    std::vector<std::pair<entt::entity, bool>> m_new_aabb_entities;

and checking if added/removed in the

init_new_aabb_entities

something like

void broadphase::init_new_aabb_entities() {
    if (m_new_aabb_entities.empty()) {
        return;
    }

    auto aabb_view = m_registry->view<AABB>();
    auto procedural_view = m_registry->view<procedural_tag>();

    for (auto kvp : m_new_aabb_entities) {
        auto entity = kvp.first;
        auto added = kvp.second;
        // Entity might've been destroyed, thus skip it.
        if (!m_registry->valid(entity)) continue;

        if(added)
        {
            auto &aabb = aabb_view.get<AABB>(entity);
            bool procedural = procedural_view.contains(entity);
            auto &tree = procedural ? m_tree : m_np_tree;
            tree_node_id_t id = tree.create(aabb, entity);
            m_registry->emplace<tree_resident>(entity, id, procedural);
        }
        else
        {
            m_registry->remove<tree_resident>(entity);
        }
    }

    m_new_aabb_entities.clear();
}
xissburg commented 8 months ago

It could be an utility function in itself, such as edyn::rigidbody_clear_shape. Modifying or replacing shapes is already supported. Though replacing with a different type would require some manual steps, such as changing the shape index.

volcoma commented 8 months ago

all the issues i opened are connected to the fact that i am trying to

  1. add/remove/modify a rigidbody at runtime, For example change it's gravity, kind etc.
  2. add/remove/modify a rigidbody's shape at runtime. For example change box's bounds or change it to different one.

The mentioned fix above would allow me to to the second one.

I am still strugling with the first. It seems i am not clearing/updating something properly.

i have

void remove_rigidbody(entt::entity entity, entt::registry& registry)
{

    APPLOG_INFO("-----BEFORE REMOVE");

    for(auto [id, storage] : registry.storage())
    {
        auto name = storage.type().name();
        if(storage.contains(entity))
        {
            APPLOG_INFO("storage {}", std::string(name));
        }
    }
    auto count = registry.remove<dynamic_tag,
                    procedural_tag,
                    kinematic_tag,
                    static_tag,
                    position,
                    orientation,
                    mass,
                    mass_inv,
                    inertia,
                    inertia_inv,
                    inertia_world_inv,
                    linvel,
                    delta_linvel,
                    angvel,
                    delta_angvel,
                    gravity,

                    material,

                    present_position,
                    present_orientation,

                    dynamic_tag,
                    procedural_tag,
                    kinematic_tag,
                    static_tag,
                    sleeping_disabled_tag,
                    sleeping_tag,

                    networked_tag,
                    multi_island_resident,
                    island_resident,
                    graph_node,
                    rigidbody_tag>(entity);

    APPLOG_INFO("-----AFTER REMOVE");

    for(auto [id, storage] : registry.storage())
    {
        auto name = storage.type().name();
        if(storage.contains(entity))
        {
            APPLOG_INFO("storage {}", std::string(name));
        }
    }
    int a = 0;
    a++;

    APPLOG_INFO("-----AFTER REMOVE FINISH");

}

and it kind of seems it works, but when detaching the registry i get a crash in the dynamic_tree class. Any help on the matter will be greatly appreciated. Thank you

xissburg commented 8 months ago

You can change gravity for one particular rigid body by modifying its edyn::gravity component. It's important to trigger the EnTT's on_update event though or else changes won't be propagated to the simulation worker, so you have to use registry.replace<edyn::gravity>(entity, gravity); or registry.patch<edyn::gravity>(entity, lambda).

Modifying the kind (static, kinematic, dynamic) is not something I have yet attempted. Certainly it requires a couple of non-trivial changes in the internal structures. It's something I will have to look into.

For the time being, perhaps you could simply destroy the entity and recreate. If you're using the same entity for other purposes and don't wanna destroy it, create a component that holds the rigid body entity and assign to this original entity instead. Then recreate the rigid body with a new def.

volcoma commented 8 months ago

your suggestion with the component that holds the "extra"entity of the rigidbody seems to work, but there is an issue i am basically doing the following:

attach(registry)

create rigidbody entity with make_rigidbody update() destroy rigidbody entity with registry.destroy

update() detach(registry)

registry->clear() -> here i get a crash in the dynamic_tree class, BUT only when using sequential mode. Async mode seems not to crash

it is triggered by this assert

void dynamic_tree::destroy(tree_node_id_t id) {
    EDYN_ASSERT(m_nodes[id].leaf());
    remove(id);
    free(id);
}

Maybe there is a bug or i am doing something wrong

xissburg commented 8 months ago

There was a problem with observers not being removed in the edyn::broadphase destructor. I have pushed a fix.

volcoma commented 8 months ago

so i tried your suggestion with the "extra"entity that i destroy/create each time i try to change the rigid body's type and it works for the most part. I still get a crash sometimes here in the erase

void island_manager::on_destroy_multi_island_resident(entt::registry &registry, entt::entity entity) {
    auto &resident = registry.get<const multi_island_resident>(entity);
    auto island_view = registry.view<edyn::island>();

    for (auto island_entity : resident.island_entities) {
        auto [island] = island_view.get(island_entity);
        island.nodes.erase(entity);     ->  HERE

        triggers this assert
       [[nodiscard]] size_type index(const entity_type entt) const noexcept {
        ENTT_ASSERT(contains(entt), "Set does not contain entity");
        return static_cast<size_type>(traits_type::to_entity(sparse_ref(entt)));
    }

I have to do quite some creations/destructions though to replicate this. Happens i both sync/async mode.

xissburg commented 8 months ago

Are you changing it from dynamic to static/kinematic? That will require some internal changes in the engine since bodies of different kinds live in different AABB trees and they're also treated differently from the perspective of islands because dynamic entities can only exist in one island at a time while kinematic/static can be present in multiple islands at once.

volcoma commented 8 months ago

yes i am changing it but i destroy the previous rigidbody entity and create a new one which is what you suggested.

xissburg commented 8 months ago

Then there must be a problem with doing so without an island manager update in-between. I'll have to check this as well.

volcoma commented 7 months ago

Hi, i see you did a commit to achieve this, but there is a typo in your commit

broadphase::broadphase(entt::registry &registry)
    : m_registry(&registry)
{
    m_connections.emplace_back(registry.on_construct<AABB>().connect<&broadphase::on_construct_aabb>(*this));

    // THIS LINE HERE SHOULD connect on_destroy_aabb instead of on_construct
    m_connections.emplace_back(registry.on_destroy<AABB>().connect<&broadphase::on_construct_aabb>(*this));   

    m_connections.emplace_back(registry.on_destroy<tree_resident>().connect<&broadphase::on_destroy_tree_resident>(*this));
    m_connections.emplace_back(registry.on_construct<island_AABB>().connect<&broadphase::on_construct_island_aabb>(*this));
    m_connections.emplace_back(registry.on_destroy<island_tree_resident>().connect<&broadphase::on_destroy_island_tree_resident>(*this));

    // The `should_collide_func` function will be invoked in parallel when
    // running broadphase in parallel, in the call to `broadphase::collide_tree_async`.
    // Avoid multi-threading issues by pre-allocating the pools that will be
    // needed in `should_collide_func`.
    static_cast<void>(registry.storage<collision_filter>());
    static_cast<void>(registry.storage<collision_exclusion>());
}
xissburg commented 7 months ago

Nice catch, thanks. That was retarded on my part.

xissburg commented 7 months ago

The new functions edyn::rigidbody_set_shape and edyn::rigidbody_set_kind fulfill the requirements of this issue.

volcoma commented 7 months ago

thanks, i will test them asap and let you know if i find any issues :)