Actionable comments posted: 5

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.

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

Inline comments:
In `@src/plugin/physics/src/resource/BodyEntityMap.hpp`:
- Around line 12-13: BodyEntityMap instantiation fails because JPH::BodyID lacks
a std::hash specialization required by Utils::BiMap; add a
std::hash<JPH::BodyID> specialization that converts the BodyID to a stable
integral value (e.g., call the BodyID accessor that returns its raw/index value
or cast to its underlying integral type) and returns
std::hash<std::underlying_type_t<...>> applied to that value; place the
specialization in namespace std so unordered_map can use it and then
BodyEntityMap (aliasing Utils::BiMap<Engine::EntityId, JPH::BodyID>) will
compile.

In `@src/plugin/physics/src/system/RigidBodySystem.cpp`:
- Around line 319-320: Remove the redundant call to BodyEntityMap::Add in
OnRigidBodyConstruct: when you call
entity.AddComponent<Component::RigidBodyInternal>(bodyID) the
OnRigidBodyInternalConstruct handler will perform the same
core->GetResource<Resource::BodyEntityMap>().Add(entity, bodyID) mapping, so
delete the explicit Add() line after AddComponent. Update OnRigidBodyConstruct
to only add the RigidBodyInternal component and rely on
OnRigidBodyInternalConstruct (and the existing BiMap::Add behavior) to insert
the mapping.

In `@src/plugin/physics/src/system/SoftBodySystem.cpp`:
- Around line 471-472: The warning in OnSoftBodyInternalConstruct incorrectly
says "skipping body removal" for SoftBodyInternal; update the Log::Warn call in
the OnSoftBodyInternalConstruct handler to state "skipping body addition to
BodyEntityMap" (or similar) so it accurately reflects the construct path and
mentions SoftBodyInternal and BodyEntityMap for clarity; keep the early return
behavior unchanged.

In `@src/plugin/physics/src/utils/BiMap.hpp`:
- Around line 12-19: Add currently uses insert_or_assign on _leftToRight and
_rightToLeft but never cleans up the previous opposite mapping, leaving stale
entries; modify Add(const TLeft &left, const TRight &right) to: check if left
already exists in _leftToRight and if so get the old right (via the stored
pointer) and erase that old right from _rightToLeft before inserting the new
mapping; similarly check if right already exists in _rightToLeft and if so erase
its old left from _leftToRight to avoid orphaned pointers; then proceed to
insert_or_assign the new left→right and right→left and update the stored pointer
fields so both maps remain consistent (refer to Add, _leftToRight, _rightToLeft,
insert_or_assign).

In `@tools/ai/copilot/copilot-commit-message-instructions.md`:
- Line 49: Update the guideline text that reads "* commit must be in english" to
capitalize the language name: change "english" to "English" (locate the exact
string "* commit must be in english" in copilot-commit-message-instructions.md
and replace it so the rule reads "* commit must be in English").

---

Outside diff comments:
In `@src/plugin/physics/src/system/RigidBodySystem.cpp`:
- Around line 411-420: Remove the dead function OnRigidBodyInternalConstruct (or
add a comment) because it is never registered in InitRigidBodySystem and its
behavior (adding mappings to BodyEntityMap) is already handled in
OnRigidBodyConstruct; specifically delete OnRigidBodyInternalConstruct (or, if
you prefer to keep it, add a clear comment on top of
OnRigidBodyInternalConstruct explaining it is unused/kept for historical
reasons) and ensure InitRigidBodySystem only registers the actual handlers
(OnRigidBodyConstruct, OnRigidBodyDestroy, OnRigidBodyInternalDestroy) so there
is no misleading, unreferenced code.

In `@src/plugin/physics/src/system/SoftBodySystem.cpp`:
- Around line 546-555: ShutdownSoftBodySystem is missing a disconnect for the
handler registered in InitSoftBodySystem: add a disconnect call for
OnSoftBodyInternalDestroy in ShutdownSoftBodySystem (mirror the existing
disconnects for OnSoftBodyConstruct/OnSoftBodyDestroy) so the registry no longer
holds the callback (i.e., call
registry.on_destroy<Component::SoftBodyInternal>().disconnect<&OnSoftBodyInternalDestroy>();
or the exact event type used when connecting) to prevent callbacks after
shutdown.

---

Nitpick comments:
In `@src/plugin/physics/src/system/RigidBodySystem.cpp`:
- Around line 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).

In `@src/plugin/physics/src/system/SoftBodySystem.cpp`:
- Around line 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.
- Around line 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.

In `@src/plugin/physics/src/utils/BiMap.hpp`:
- Around line 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.

In `@src/plugin/physics/tests/BodyEntityMapTest.cpp`:
- Around line 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.

In `@src/plugin/physics/tests/BodyEntitySyncTest.cpp`:
- Around line 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.

In `@tools/ai/copilot/copilot-commit-message-instructions.md`:
- Around line 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.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ba56f620-c1d1-4b23-a020-39bcc948c7f9

Reviewing files that changed from the base of the PR and between af58b97 and ae194be.

• src/engine/src/entity/Entity.cpp
• src/engine/src/entity/Entity.hpp
• src/engine/src/entity/EntityId.hpp
• src/plugin/physics/src/Physics.hpp
• src/plugin/physics/src/plugin/PhysicsPlugin.cpp
• src/plugin/physics/src/resource/BodyEntityMap.hpp
• src/plugin/physics/src/system/RigidBodySystem.cpp
• src/plugin/physics/src/system/SoftBodySystem.cpp
• src/plugin/physics/src/utils/BiMap.hpp
• src/plugin/physics/tests/BodyEntityMapTest.cpp
• src/plugin/physics/tests/BodyEntitySyncTest.cpp
• tools/ai/copilot/copilot-commit-message-instructions.md

Pull request overview

This PR moves the Jolt BodyID ↔ Engine::EntityId association into the physics plugin itself by introducing an internal BodyEntityMap resource backed by a new bidirectional BiMap utility, and wiring map updates into rigid/soft body lifecycle hooks.

Changes:

• Added Physics::Utils::BiMap<TLeft, TRight> and Physics::Resource::BodyEntityMap (BiMap alias) for bidirectional lookup.
• Physics plugin now auto-registers BodyEntityMap and updates it from RigidBodySystem / SoftBodySystem callbacks.
• Added Engine::Entity::Kill(), std::hash<Engine::EntityId>, and new unit tests for mapping + sync.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 7 comments.

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;

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Actionable comments posted: 3

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.

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

Inline comments:
In `@src/plugin/physics/src/system/RigidBodySystem.cpp`:
- Around line 417-420: The RigidBody event hookups
(registry.on_construct<Component::RigidBody>().connect<&OnRigidBodyConstruct>(),
registry.on_construct<Component::RigidBodyInternal>().connect<&OnRigidBodyInternalConstruct>(),
registry.on_destroy<Component::RigidBody>().connect<&OnRigidBodyDestroy>(),
registry.on_destroy<Component::RigidBodyInternal>().connect<&OnRigidBodyInternalDestroy>())
need matching disconnects on plugin shutdown to avoid callbacks running during
teardown; add a ShutdownRigidBodySystem function that disconnects those four
connections (mirror the pattern used for soft body) and ensure
ShutdownRigidBodySystem is registered in the plugin shutdown schedule so it runs
before physics resources are destroyed.

In `@src/plugin/physics/src/system/SoftBodySystem.cpp`:
- Around line 332-337: The code dereferences corePtr when constructing
Engine::Entity entity{*corePtr, entityId} before checking for null, which can
crash; move the null check for corePtr (result of
registry.ctx().get<Engine::Core *()> ) before any use of corePtr, and if null
log the error via Log::Error("Cannot create SoftBody: Engine::Core not
available") and return/abort the soft-body creation path instead of constructing
Engine::Entity; update any early-return or error handling in the surrounding
function to match this control flow.

In `@tools/ai/copilot/copilot-commit-message-instructions.md`:
- Line 15: Update the sentence that currently reads "The `<type>` and
`<summary>` fields are mandatory, the `(<scope>)` field is optional." to use the
canonical placeholder `<short summary>` instead of `<summary>` so it matches the
header structure; ensure the line now reads "The `<type>` and `<short summary>`
fields are mandatory, the `(<scope>)` field is optional." and verify any other
occurrences of `<summary>` in this document are replaced with `<short summary>`
for consistency.

---

Nitpick comments:
In `@src/plugin/physics/src/system/RigidBodySystem.cpp`:
- Around line 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.

In `@src/plugin/physics/src/system/SoftBodySystem.cpp`:
- 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.
- 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.

In `@src/plugin/physics/tests/BodyEntityMapTest.cpp`:
- Around line 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.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 34785713-792d-4e3c-bd22-e3e121c637ff

Reviewing files that changed from the base of the PR and between 463a291 and fa90bcb.

• src/plugin/physics/src/system/RigidBodySystem.cpp
• src/plugin/physics/src/system/SoftBodySystem.cpp
• src/plugin/physics/src/utils/BiMap.hpp
• src/plugin/physics/tests/BodyEntityMapTest.cpp
• src/plugin/physics/tests/BodyEntitySyncTest.cpp
• tools/ai/copilot/copilot-commit-message-instructions.md

• src/plugin/physics/tests/BodyEntitySyncTest.cpp

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.

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

Nitpick comments:
In `@src/plugin/physics/src/utils/BiMap.hpp`:
- 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.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: be8cf709-ec4b-45c3-a909-d9d3aea3b99e

Reviewing files that changed from the base of the PR and between fa90bcb and cbdbc85.

• src/plugin/physics/src/system/RigidBodySystem.cpp
• src/plugin/physics/src/system/SoftBodySystem.cpp
• src/plugin/physics/src/utils/BiMap.hpp

• src/plugin/physics/src/system/SoftBodySystem.cpp
• src/plugin/physics/src/system/RigidBodySystem.cpp

Actionable comments posted: 2

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

Inline comments:
In `@src/plugin/physics/tests/PhysicsTest.cpp`:
- Around line 73-78: The assertion for collisionRemoved runs too early inside
the TestScheduler callback; add a second phase that explicitly separates or
destroys the cube and then checks collisionRemoved on a later tick (after the
separation/destruction and after the scheduler processes that change).
Concretely, modify the TestScheduler (registered via
c.RegisterSystem<TestScheduler>) to: 1) keep the existing first-phase
assertions, 2) schedule a follow-up tick or callback that either move the cube's
Transform away or call the cube destruction/removal path, and 3) assert
EXPECT_TRUE(collisionRemoved) only in that follow-up phase (or after
Engine::Scheduler::Shutdown has been processed) so the removal path is
guaranteed to have run.

In `@src/plugin/physics/tests/VehicleForwardMovementTest.cpp`:
- Around line 81-95: The scheduled lambda uses ASSERT_NE for the transform and
controller lookups which returns from the lambda without setting firstStepMade
or stopping the Engine::Core, causing the test to hang; replace those ASSERT_NE
checks with explicit null checks on
vehicle.TryGetComponent<Object::Component::Transform>() and
vehicle.TryGetComponent<Physics::Component::VehicleController>() that, on
failure, call core.Stop() and fail the test (e.g., GTEST_FAIL or FAIL()) before
returning, ensuring firstStepMade is only set after successful setup and
controller->SetForward(1.0f) is only called when components are valid.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 81146391-c85e-4355-86db-5f1cbe873cd0

Reviewing files that changed from the base of the PR and between cbdbc85 and 40cd9f6.

• src/plugin/physics/src/plugin/PhysicsPlugin.cpp
• src/plugin/physics/src/system/RigidBodySystem.cpp
• src/plugin/physics/src/system/RigidBodySystem.hpp
• src/plugin/physics/src/system/SoftBodySystem.cpp
• src/plugin/physics/tests/PhysicsTest.cpp
• src/plugin/physics/tests/VehicleForwardMovementTest.cpp
• src/plugin/physics/tests/VehicleGravityDropTest.cpp
• src/plugin/physics/tests/VehicleWheelTransformSyncTest.cpp
• tools/ai/copilot/copilot-commit-message-instructions.md

• tools/ai/copilot/copilot-commit-message-instructions.md

• src/plugin/physics/src/system/RigidBodySystem.cpp
• src/plugin/physics/src/system/SoftBodySystem.cpp