tools/ai/copilot/copilot-commit-message-instructions.md (1)

5-13: Consider adding a language specifier to the fenced code block.

The fenced code block lacks a language identifier. Adding text would satisfy the linter and improve rendering consistency across markdown viewers.

✨ Proposed improvement

-```
+```text
 <type>(<scope>): <short summary>
   │       │             │
   │       │             └─⫸ Summary in present tense. Not capitalized. No period at the end.
   │       │
   │       └─⫸ Commit Scope
   │
   └─⫸ Commit Type: build|ci|docs|feat|fix|perf|refactor|test|chore
 ```

As per static analysis hints, markdownlint flags fenced code blocks without language specifiers.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tools/ai/copilot/copilot-commit-message-instructions.md` around lines 5 - 13,
The fenced code block that contains the commit template starting with
"<type>(<scope>): <short summary>" lacks a language specifier; update the
opening fence from ``` to ```text so the block becomes "```text" (and keep the
closing fence as ```), which satisfies markdownlint and ensures consistent
rendering.

src/plugin/physics/src/utils/BiMap.hpp (1)

7-10: Consider explicitly deleting copy/move operations.

The class stores pointers to keys within its internal maps. The implicitly-generated copy constructor and copy assignment operator would copy these pointers, which would then point into the source object's maps—causing undefined behavior if the copy is used after the source is modified or destroyed.

Since BodyEntityMap is used as a singleton resource, this may not be an immediate problem, but explicitly deleting these operations would prevent accidental misuse.

♻️ Suggested change

 template <typename TLeft, typename TRight> class BiMap {
   public:
     BiMap() = default;
     ~BiMap() = default;
+    BiMap(const BiMap &) = delete;
+    BiMap &operator=(const BiMap &) = delete;
+    BiMap(BiMap &&) = delete;
+    BiMap &operator=(BiMap &&) = delete;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/plugin/physics/src/utils/BiMap.hpp` around lines 7 - 10, The BiMap
template should explicitly disable copying/moving to avoid dangling internal
pointer issues; update the BiMap class declaration (and any singleton wrapper
like BodyEntityMap usage) to delete the copy constructor, copy assignment
operator, move constructor, and move assignment operator (e.g., declare
BiMap(const BiMap&) = delete; BiMap& operator=(const BiMap&) = delete;
BiMap(BiMap&&) = delete; BiMap& operator=(BiMap&&) = delete;) so instances
cannot be copied or moved accidentally and pointers into its internal maps
remain valid.

src/plugin/physics/tests/BodyEntityMapTest.cpp (1)

5-77: Good test coverage for basic operations.

The tests cover addition, removal, retrieval, and containment checks. Consider adding tests for edge cases:

1. Updating an existing mapping — verifying that Add(existingLeft, newRight) properly replaces the mapping (this would catch the bug identified in BiMap::Add)
2. Removing non-existent keys — ensuring Remove() is safe when the key doesn't exist
3. Get() on missing key — documenting that it throws std::out_of_range

🧪 Example additional test case for update behavior

TEST(PhysicsPlugin, BodyEntityMapUpdate)
{
    Physics::Resource::BodyEntityMap map;

    map.Add(Engine::EntityId{1}, JPH::BodyID{10});
    EXPECT_EQ(map.Get(Engine::EntityId{1}), JPH::BodyID{10});

    // Update mapping for same entity to different body
    map.Add(Engine::EntityId{1}, JPH::BodyID{20});
    
    EXPECT_EQ(map.Size(), 1);  // Should still be 1, not 2
    EXPECT_EQ(map.Get(Engine::EntityId{1}), JPH::BodyID{20});
    EXPECT_FALSE(map.Contains(JPH::BodyID{10}));  // Old body should be removed
}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/plugin/physics/tests/BodyEntityMapTest.cpp` around lines 5 - 77, Tests
miss edge cases: update existing mapping, removing non-existent keys, and Get()
on missing keys; add tests and fix BiMap::Add so Add(existingLeft, newRight)
replaces the old right and updates both directions. Specifically, modify
BiMap::Add to check if left or right already exist and remove their old pairings
before inserting (ensure it updates both maps used by
Physics::Resource::BodyEntityMap's Add/Remove/Get/Contains), make Remove safe
when key absent (no-ops), and ensure Get throws std::out_of_range for missing
keys so tests can assert that behavior; then add the suggested TEST
PhysicsPlugin.BodyEntityMapUpdate plus cases for Remove non-existent and Get
missing-key.

src/plugin/physics/src/system/SoftBodySystem.cpp (2)

446-447: Same duplicate addition issue as RigidBodySystem.

OnSoftBodyConstruct adds to BodyEntityMap (line 447), and OnSoftBodyInternalConstruct (lines 461-476) would also add when triggered. However, OnSoftBodyInternalConstruct is not registered in InitSoftBodySystem, making it dead code.

For consistency with the destruction pattern, either:

1. Remove the dead OnSoftBodyInternalConstruct function, or
2. Register it and remove the Add call from OnSoftBodyConstruct

Also applies to: 461-476

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/plugin/physics/src/system/SoftBodySystem.cpp` around lines 446 - 447, The
code currently duplicates BodyEntityMap additions because OnSoftBodyConstruct
calls core.GetResource<Resource::BodyEntityMap>().Add(entity, bodyID) while
OnSoftBodyInternalConstruct (defined but not used) would also add the same
mapping; fix by choosing one approach: either remove the dead
OnSoftBodyInternalConstruct function entirely, or register it in
InitSoftBodySystem and remove the Add(...) call from OnSoftBodyConstruct so the
mapping is added only inside OnSoftBodyInternalConstruct; update
InitSoftBodySystem to subscribe the OnSoftBodyInternalConstruct handler if you
pick registration, or delete the OnSoftBodyInternalConstruct symbol and any
unused references if you pick removal to match the destruction pattern.

---

502-503: Duplicate removal from BodyEntityMap (same as RigidBodySystem).

OnSoftBodyDestroy removes from BodyEntityMap (line 502) and then removes SoftBodyInternal (line 503), which triggers OnSoftBodyInternalDestroy to remove again (line 525).

Apply the same fix as suggested for RigidBodySystem - let OnSoftBodyInternalDestroy handle the removal consistently.

♻️ Suggested consolidation

         Log::Debug(fmt::format("Destroyed SoftBody for entity {}", entity));
 
-        core.GetResource<Resource::BodyEntityMap>().Remove(internal->bodyID);
         entity.RemoveComponent<Component::SoftBodyInternal>();

Also applies to: 511-526

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/plugin/physics/src/system/SoftBodySystem.cpp` around lines 502 - 503,
OnSoftBodyDestroy currently removes the body from BodyEntityMap and then removes
the SoftBodyInternal component which causes OnSoftBodyInternalDestroy to remove
the same entry again; remove the explicit call to
core.GetResource<Resource::BodyEntityMap>().Remove(internal->bodyID) from
OnSoftBodyDestroy and only call
entity.RemoveComponent<Component::SoftBodyInternal>() so that
OnSoftBodyInternalDestroy is the single place that performs
core.GetResource<Resource::BodyEntityMap>().Remove(bodyID); apply the same
change for the duplicated block in the 511-526 region to consolidate removal
logic.

src/plugin/physics/src/system/RigidBodySystem.cpp (1)

382-383: Duplicate removal from BodyEntityMap.

OnRigidBodyDestroy removes the mapping from BodyEntityMap (line 382) and then removes the RigidBodyInternal component (line 383). Removing the component triggers OnRigidBodyInternalDestroy, which attempts to remove the same mapping again (line 404).

This mirrors the duplicate addition issue. Consider removing the BodyEntityMap.Remove() call from OnRigidBodyDestroy (line 382) and letting OnRigidBodyInternalDestroy handle it consistently.

♻️ Suggested consolidation

         Log::Debug(fmt::format("Destroyed RigidBody for entity {} with BodyID {}", entity,
                                internalComponent->bodyID.GetIndexAndSequenceNumber()));
 
-        core->GetResource<Resource::BodyEntityMap>().Remove(internalComponent->bodyID);
         entity.RemoveComponent<Component::RigidBodyInternal>();

This lets OnRigidBodyInternalDestroy handle the map removal consistently.

Also applies to: 391-405

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/plugin/physics/src/system/RigidBodySystem.cpp` around lines 382 - 383,
OnRigidBodyDestroy currently calls
core->GetResource<Resource::BodyEntityMap>().Remove(internalComponent->bodyID)
and then entity.RemoveComponent<Component::RigidBodyInternal>(), which causes
OnRigidBodyInternalDestroy to attempt the same removal again; remove the
explicit BodyEntityMap.Remove(...) call from OnRigidBodyDestroy and let
OnRigidBodyInternalDestroy perform the single authoritative removal of the
bodyID mapping when handling the Component::RigidBodyInternal destruction
(ensure any other paths that remove the component rely on that single removal).

src/plugin/physics/tests/BodyEntitySyncTest.cpp (1)

46-86: Missing error policy setting may cause test instability.

BodyEntityMapRemoval test (lines 46-86) does not call SetErrorPolicyForAllSchedulers, while other tests do (lines 17, 92). If entity.Kill() or other operations throw/log errors, this inconsistency could lead to different test behaviors.

🔧 Suggested fix

 TEST(PhysicsPlugin, BodyEntityMapRemoval)
 {
     Engine::Core core;
 
+    core.SetErrorPolicyForAllSchedulers(Engine::Scheduler::SchedulerErrorPolicy::Nothing);
+
     core.AddPlugins<Physics::Plugin>();

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/plugin/physics/tests/BodyEntitySyncTest.cpp` around lines 46 - 86, The
test BodyEntityMapRemoval omits calling SetErrorPolicyForAllSchedulers on the
Engine::Core instance, causing inconsistent error-handling compared to other
tests; update the test to invoke core.SetErrorPolicyForAllSchedulers(...) (using
the same ErrorPolicy used in the other tests) during setup—before
core.RunSystems() (for example right after core.AddPlugins or inside the
core.RegisterSystem setup) so that entity.Kill() and other operations use the
same error policy and the test behaves consistently.

src/plugin/physics/src/system/SoftBodySystem.cpp:340

• corePtr is dereferenced to construct Engine::Entity before checking for nullptr. If the registry context ever contains a null Core* (or is otherwise misconfigured), this will crash before the intended error handling. Check corePtr before using *corePtr.

        auto *corePtr = registry.ctx().get<Engine::Core *>();
        Engine::Entity entity{*corePtr, entityId};

        if (!corePtr)
        {
            Log::Error("Cannot create SoftBody: Engine::Core not available");
            return;
        }
        auto &core = *corePtr;

src/plugin/physics/src/system/RigidBodySystem.cpp (1)

382-383: Map removal is duplicated across destroy handlers.

OnRigidBodyDestroy removes the BodyID, then entity.RemoveComponent<Component::RigidBodyInternal>() triggers OnRigidBodyInternalDestroy, which removes it again. Keep one path to reduce redundant work and lifecycle coupling.

Also applies to: 391-405

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/plugin/physics/src/system/RigidBodySystem.cpp` around lines 382 - 383,
OnRigidBodyDestroy and OnRigidBodyInternalDestroy both call
core->GetResource<Resource::BodyEntityMap>().Remove(internalComponent->bodyID),
causing duplicated removal; pick a single owner of that responsibility (suggest
removing the Remove call from OnRigidBodyDestroy and let removal happen in
OnRigidBodyInternalDestroy which runs when
entity.RemoveComponent<Component::RigidBodyInternal>() is invoked) and update
comments to document the ownership; ensure no other code paths rely on the
removed line and apply the same change for the other duplicated block (the code
around the 391-405 section) so BodyEntityMap::Remove(bodyID) is only called once
per lifecycle.

src/plugin/physics/src/system/SoftBodySystem.cpp (2)

468-468: Avoid copying SoftBodyInternal in hook callbacks.

Use a const reference to avoid copying the component payload.

🧩 Proposed fix

-    auto internalComponent = entity.GetComponents<Component::SoftBodyInternal>();
+    const auto &internalComponent = entity.GetComponents<Component::SoftBodyInternal>();
...
-    auto internalComponent = entity.GetComponents<Component::SoftBodyInternal>();
+    const auto &internalComponent = entity.GetComponents<Component::SoftBodyInternal>();

Also applies to: 518-518

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/plugin/physics/src/system/SoftBodySystem.cpp` at line 468, The code is
copying the Component::SoftBodyInternal payload by using auto internalComponent
= entity.GetComponents<Component::SoftBodyInternal>(); in SoftBodySystem.cpp;
change the declaration to bind a const reference (e.g., const auto&
internalComponent or const Component::SoftBodyInternal& internalComponent) so
the hook callbacks don't copy the component data, and apply the same change to
the other occurrence around the 518 call site; ensure you still respect the
component's lifetime when using the reference.

---

447-447: BodyEntityMap is updated twice via both direct calls and internal hooks.

OnSoftBodyInternalConstruct/OnSoftBodyInternalDestroy already synchronize the map, so explicit Add/Remove in soft-body construct/destroy is redundant.

♻️ Proposed simplification

-        core.GetResource<Resource::BodyEntityMap>().Add(entity, bodyID);
...
-        core.GetResource<Resource::BodyEntityMap>().Remove(internal->bodyID);
         entity.RemoveComponent<Component::SoftBodyInternal>();

Also applies to: 502-503, 540-542

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/plugin/physics/src/system/SoftBodySystem.cpp` at line 447, Remove the
redundant explicit BodyEntityMap updates in the soft-body construct/destroy
handlers: delete the core.GetResource<Resource::BodyEntityMap>().Add(...) and
.Remove(...) calls that appear alongside the soft-body construction/destruction
code and rely instead on the existing OnSoftBodyInternalConstruct and
OnSoftBodyInternalDestroy hooks to keep BodyEntityMap in sync; specifically
remove the Add at the construct site and the corresponding Remove(s) at the
destroy sites so only the internal hook methods manage the map.

src/plugin/physics/tests/BodyEntityMapTest.cpp (1)

79-91: Add a reverse-update case (same BodyID, different EntityId).

Please also test Add(EntityA, BodyX) followed by Add(EntityB, BodyX) to validate old left-key eviction on right-key reassignment.

🧪 Proposed extension

 TEST(PhysicsPlugin, BodyEntityMapUpdate)
 {
     Physics::Resource::BodyEntityMap map;

     map.Add(Engine::EntityId{1}, JPH::BodyID{10});
     EXPECT_EQ(map.Get(Engine::EntityId{1}), JPH::BodyID{10});

     map.Add(Engine::EntityId{1}, JPH::BodyID{20});

     EXPECT_EQ(map.Size(), 1);
     EXPECT_EQ(map.Get(Engine::EntityId{1}), JPH::BodyID{20});
     EXPECT_FALSE(map.Contains(JPH::BodyID{10}));
+
+    map.Add(Engine::EntityId{2}, JPH::BodyID{20});
+    EXPECT_EQ(map.Size(), 1);
+    EXPECT_FALSE(map.Contains(Engine::EntityId{1}));
+    EXPECT_EQ(map.Get(JPH::BodyID{20}), Engine::EntityId{2});
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/plugin/physics/tests/BodyEntityMapTest.cpp` around lines 79 - 91, Add a
new test case in BodyEntityMapTest that exercises the reverse-update: call
map.Add(Engine::EntityId{1}, JPH::BodyID{10}) then map.Add(Engine::EntityId{2},
JPH::BodyID{10}) and assert the old left-key was evicted and the right-key
reassigned; specifically check map.Size() == 1, map.Get(Engine::EntityId{2}) ==
JPH::BodyID{10}, map no longer returns the old mapping for Engine::EntityId{1}
(e.g. Get(Engine::EntityId{1}) is not JPH::BodyID{10} or is invalid), and
map.Contains(JPH::BodyID{10}) is true.

src/plugin/physics/src/utils/BiMap.hpp (1)

49-49: Use std::size_t for Size() to avoid truncation.

Line 49 narrows from container size() to std::uint32_t. Returning std::size_t is safer and matches STL conventions.

Suggested change

-    std::uint32_t Size() const { return static_cast<std::uint32_t>(this->_leftToRight.size()); }
+    std::size_t Size() const { return this->_leftToRight.size(); }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/plugin/physics/src/utils/BiMap.hpp` at line 49, The Size() accessor in
class BiMap.hpp currently returns std::uint32_t causing potential truncation;
change its return type to std::size_t and return this->_leftToRight.size()
directly (no narrowing cast). Update the Size() declaration and its definition
(function named Size) to use std::size_t, and then adjust any call-sites that
assumed a uint32_t return type to handle std::size_t if necessary.