Actionable comments posted: 1

In @src/plugin/physics/src/system/ConstraintHelpers.cpp:
- Around line 5-17: ConstraintContext::Create currently dereferences
registry.ctx().get<Engine::Core *>() without checking for null; modify Create to
first retrieve the pointer into a local (e.g., auto *corePtr =
registry.ctx().get<Engine::Core *>()), check if corePtr is null, log an error
mentioning the missing Core (using Log::Error with a clear message that includes
constraintName), and return std::nullopt if null; only then use *corePtr (or a
reference coreRef) to proceed with accessing
GetResource<Resource::PhysicsManager>() and the rest of the existing logic.

src/plugin/physics/src/system/ConstraintSystem.cpp (3)

38-43: Redundant mSpace assignment.

The mSpace setting is already assigned in CreateConstraintGeneric (see ConstraintHelpers.cpp line 124). This assignment is harmless but redundant. Consider removing it from the configurator lambdas across all constraint handlers for consistency.

---

69-74: Consider documenting the spring setting multipliers.

The magic numbers 10000.0f and 100.0f for stiffness and damping scaling are unclear. Consider extracting these to named constants with documentation explaining the rationale for these specific values, especially since ConstraintSettings documents stiffness/damping as [0.0, 1.0] ranges.

♻️ Suggested refactor

+// Scale factors to convert normalized [0,1] settings to Jolt spring parameters
+constexpr float STIFFNESS_SCALE = 10000.0f;
+constexpr float DAMPING_SCALE = 100.0f;
+
 // In the lambda:
-joltSettings.mLimitsSpringSettings.mStiffness = constraint.settings.stiffness * 10000.0f;
-joltSettings.mLimitsSpringSettings.mDamping = constraint.settings.damping * 100.0f;
+joltSettings.mLimitsSpringSettings.mStiffness = constraint.settings.stiffness * STIFFNESS_SCALE;
+joltSettings.mLimitsSpringSettings.mDamping = constraint.settings.damping * DAMPING_SCALE;

---

76-84: Use the NAME constant for consistency.

The error message uses a hardcoded "DistanceConstraint" string instead of the NAME constant defined at line 58.

♻️ Suggested fix

             if (constraint.maxDistance >= 0 && constraint.minDistance >= 0 &&
                 constraint.maxDistance < constraint.minDistance)
             {
-                Log::Error(fmt::format("{}: maxDistance < minDistance", "DistanceConstraint"));
+                Log::Error(fmt::format("{}: maxDistance < minDistance", NAME));
                 return false;
             }

Note: This requires capturing NAME in the lambda or passing it as a parameter.

src/plugin/physics/src/component/ConstraintSettings.hpp (1)

26-27: Unused includes.

Neither <cstdint> nor <limits> appear to be used in this file. Consider removing them to keep the header minimal.

♻️ Suggested fix

 #pragma once
 
-#include <cstdint>
-#include <limits>
-
 namespace Physics::Component {

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

48-76: Same null check concern; cleanup logic is otherwise sound.

The same defensive check for the core pointer should be applied here. The exception handling approach of logging and continuing is reasonable for cleanup paths to prevent cascading failures.

Suggested defensive check

 void DestroyConstraint(entt::registry &registry, entt::entity entity, const char *constraintName)
 {
     try
     {
-        auto &coreRef = *registry.ctx().get<Engine::Core *>();
+        auto *corePtr = registry.ctx().get<Engine::Core *>();
+        if (!corePtr)
+            return;
+        auto &coreRef = *corePtr;
         auto &physicsManagerRef = coreRef.GetResource<Resource::PhysicsManager>();

src/plugin/physics/src/component/DistanceConstraint.hpp (2)

288-292: Floating-point equality comparison may be fragile.

IsFixedDistance() uses direct equality comparison of floats. While this works when values are set via factory methods, it could be unreliable if distances are computed or modified at runtime.

Consider using epsilon comparison

-[[nodiscard]] bool IsFixedDistance() const { return minDistance == maxDistance; }
+[[nodiscard]] bool IsFixedDistance() const { 
+    return std::abs(minDistance - maxDistance) < 1e-6f; 
+}

---

201-216: Consider validating that min <= max in CreateWithRange.

The factory method accepts min and max parameters but doesn't validate that min <= max. Invalid input could create constraints with undefined behavior.

Add validation

 [[nodiscard]] static DistanceConstraint
 CreateWithRange(Engine::Entity a, Engine::Entity b, float min, float max, const glm::vec3 &pointA = glm::vec3(0.0f),
                 const glm::vec3 &pointB = glm::vec3(0.0f),
                 const ConstraintSettings &constraintSettings = ConstraintSettings::Soft())
 {
+    if (min > max)
+    {
+        std::swap(min, max); // Or throw/assert
+    }
     DistanceConstraint constraint;

src/plugin/physics/src/system/ConstraintHelpers.hpp (1)

34-50: Remove redundant static on template functions in header.

Template functions in headers have internal linkage semantics by default when instantiated. The static keyword is redundant and could be misleading. Consider removing it for clarity.

Remove static keyword

 template <typename ConstraintT>
-static bool ValidateConstraint(const ConstraintT &constraint, const char *constraintName)
+bool ValidateConstraint(const ConstraintT &constraint, const char *constraintName)

The same applies to CreateJoltConstraint (line 56) and CreateConstraintGeneric (line 103).

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between 85ce8fd and c7fdccf.

• src/plugin/physics/src/Physics.hpp
• src/plugin/physics/src/component/ConstraintInternal.hpp
• src/plugin/physics/src/component/ConstraintSettings.hpp
• src/plugin/physics/src/component/DistanceConstraint.hpp
• src/plugin/physics/src/component/FixedConstraint.hpp
• src/plugin/physics/src/component/PointConstraint.hpp
• src/plugin/physics/src/exception/ConstraintError.hpp
• src/plugin/physics/src/plugin/PhysicsPlugin.cpp
• src/plugin/physics/src/system/ConstraintHelpers.cpp
• src/plugin/physics/src/system/ConstraintHelpers.hpp
• src/plugin/physics/src/system/ConstraintSystem.cpp
• src/plugin/physics/src/system/ConstraintSystem.hpp
• src/plugin/physics/src/utils/JoltConversions.hpp

src/plugin/physics/src/system/ConstraintHelpers.hpp (4)

• registry (31-31)
• CreateConstraintGeneric (103-141)
• CreateConstraintGeneric (103-104)
• DestroyConstraint (99-99)

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

• DestroyConstraint (48-76)
• DestroyConstraint (48-48)

src/plugin/physics/src/system/ConstraintHelpers.hpp (1)

• registry (31-31)

src/engine/src/entity/Entity.hpp (1)

• static_cast (65-65)

src/plugin/physics/src/component/ConstraintInternal.hpp (2)

• nodiscard (94-94)
• nodiscard (100-100)

src/plugin/physics/src/system/ConstraintHelpers.cpp (6)

• GetBodyInternal (19-29)
• GetBodyInternal (19-20)
• Create (5-17)
• Create (5-5)
• FinalizeConstraint (31-46)
• FinalizeConstraint (31-33)

src/plugin/physics/src/plugin/PhysicsPlugin.cpp (1)

19-22: LGTM!

The initialization order correctly ensures InitConstraintSystem runs after InitRigidBodySystem and InitPhysicsManager, matching the documented dependency requirements in the ConstraintSystem header.

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

41-46: LGTM!

The conversion is consistent with the existing pattern. The implicit float-to-double promotion is appropriate here.

src/plugin/physics/src/system/ConstraintSystem.hpp (1)

30-60: LGTM!

The documentation is comprehensive, clearly specifying initialization order requirements and registered hooks. The example usage is helpful for consumers of this API.

src/plugin/physics/src/exception/ConstraintError.hpp (1)

37-39: LGTM!

Clean exception class design with appropriate constructor inheritance and well-documented throw scenarios.

src/plugin/physics/src/Physics.hpp (1)

7-33: LGTM!

The header organization is clean with clear section comments. All new constraint-related components, exceptions, and systems are properly exposed through the umbrella header.

src/plugin/physics/src/component/ConstraintSettings.hpp (1)

51-196: LGTM!

Well-designed settings struct with excellent documentation. The factory methods provide a clean API for common use cases, and the query methods (IsBreakable, IsRigid) offer convenient introspection.

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

119-137: LGTM!

The initialization logic correctly stores the Core pointer in the registry context and registers all construct/destroy hooks. The logging at the end provides good observability.

Regarding hook disconnection: EnTT automatically cleans up all signals and connected slots when the registry is destroyed. Since the registry is owned by Core via unique_ptr and destroyed with the Core destructor, explicit disconnection is unnecessary. Additionally, the on_destroy hooks already handle cleanup of Jolt physics objects, ensuring resources are released when constraints are removed.

src/plugin/physics/src/component/FixedConstraint.hpp (1)

71-191: Well-structured constraint component with clear factory methods.

The component design is clean with proper separation between body-to-body and body-to-world constraints. The factory methods correctly initialize all fields, and the IsWorldConstraint() helper provides a clear API for checking constraint type.

src/plugin/physics/src/component/PointConstraint.hpp (1)

76-215: Consistent design with useful additional factory method.

The CreateToWorldWithOffset factory method provides useful flexibility for attaching off-center points on bodies to world anchors. The component follows the same well-structured pattern as FixedConstraint.

src/plugin/physics/src/component/DistanceConstraint.hpp (1)

79-293: Comprehensive API with useful convenience methods.

The variety of factory methods (Create, CreateWithRange, CreateToWorld, CreateAutoDistance) and query helpers (IsWorldConstraint, IsAutoDistance, IsFixedDistance) provide a flexible and ergonomic API for different use cases.

src/plugin/physics/src/system/ConstraintHelpers.hpp (2)

55-93: Proper body locking pattern for constraint creation.

The CreateJoltConstraint function correctly acquires body locks before creating constraints, handles both world and body-to-body cases, and properly checks lock success. This is essential for thread-safe physics operations.

---

101-141: Generic constraint factory is well-designed.

CreateConstraintGeneric provides a clean abstraction for constraint creation with proper error handling. The configurator/extraValidate pattern allows type-specific customization while sharing common validation and finalization logic.

src/plugin/physics/src/component/ConstraintInternal.hpp (1)

52-101: Clarify the constraint ownership model and note that the breaking mechanism is incomplete.

The raw JPH::Constraint* pointer ownership is sound—Jolt takes ownership via AddConstraint and it's properly removed in DestroyConstraint.

However, the broken flag exists in both ConstraintInternal and the public constraint components (FixedConstraint, PointConstraint, DistanceConstraint), but it's initialized to false and never updated. The breakForce and breakTorque thresholds are stored but not used—no constraint monitoring system reads actual forces from Jolt to compare against these thresholds and set the broken flag. Consider implementing the force monitoring logic or removing the incomplete breaking mechanism to avoid confusion.

Actionable comments posted: 2

In @src/plugin/physics/src/system/ConstraintSystem.cpp:
- Around line 119-137: InitConstraintSystem currently always connects entt hooks
and can register them multiple times on repeated initialization; add a ctx
sentinel (e.g., a distinct tag type or a bool like ConstraintSystemInitialized)
in the registry context and check registry.ctx().contains<YourSentinel>() at the
start of InitConstraintSystem to return early if already initialized, and after
successfully connecting the hooks
(OnFixedConstraintConstruct/OnFixedConstraintDestroy,
OnDistanceConstraintConstruct/OnDistanceConstraintDestroy,
OnPointConstraintConstruct/OnPointConstraintDestroy) emplace the sentinel into
the ctx to mark initialization; this prevents duplicate hook connections on
hot-reload/multiple worlds while keeping the existing hook registration logic
intact.

src/plugin/physics/src/component/PointConstraint.hpp (1)

76-92: Prefer explicit default initialization for bodyA/bodyB to avoid “uninitialized owner” states.

If someone default-constructs PointConstraint (not via factories), bodyA/bodyB rely on Engine::Entity default ctor behavior. Consider making the intent explicit.

Proposed diff

 struct PointConstraint {
@@
-    Engine::Entity bodyA;
+    Engine::Entity bodyA{};
@@
-    Engine::Entity bodyB;
+    Engine::Entity bodyB{};
@@
     bool broken = false;

Also applies to: 129-130

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

38-53: Consider guarding against double-finalization on the same entity.

If CreateConstraintGeneric can run twice for the same entity (e.g., component emplace in a prefab load path + later patching), FinalizeConstraint will unconditionally emplace<ConstraintInternal> (Line 49-50) which can assert in entt if it already exists. A cheap defensive guard (any_of/try_get) would make this path more robust.

src/plugin/physics/src/system/ConstraintSystem.hpp (1)

30-61: Docs are clear; consider stating whether init is idempotent / safe to call twice.

Given hook connections are additive in entt, it’d help to document (or enforce) “only call once” vs “safe to call multiple times”.

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

63-75: Centralize/justify spring scaling constants (stiffness * 10000, damping * 100).

The mapping in Line 72-73 is “magic-number tuned”; if it’s intentional, consider moving it into a shared helper (or documenting the rationale) so all constraint types stay consistent and future SoftBody work doesn’t drift.

src/plugin/physics/src/component/ConstraintSettings.hpp (1)

136-184: Clamp/validate documented ranges in factories to avoid invalid physics inputs.

Factories accept arbitrary stiff/damp (Line 176-184) even though docs say [0, 1]. A small clamp makes behavior predictable and avoids feeding out-of-range values into Jolt mapping.

Proposed diff

-#include <cmath>
+#include <algorithm>
+#include <cmath>
@@
     [[nodiscard]] static ConstraintSettings Soft(float stiff = 0.5f, float damp = 0.1f)
     {
         ConstraintSettings settings;
-        settings.stiffness = stiff;
-        settings.damping = damp;
+        settings.stiffness = std::clamp(stiff, 0.0f, 1.0f);
+        settings.damping = std::clamp(damp, 0.0f, 1.0f);
         settings.breakForce = 0.0f;
         settings.breakTorque = 0.0f;
         return settings;
     }

Also applies to: 201-202

src/plugin/physics/src/component/FixedConstraint.hpp (1)

172-184: Consider adding semantic clarity for world constraints.

The CreateToWorld method stores worldPoint in localPointB, which is documented as "local body space" (line 104-109). While the comment on line 180 acknowledges this dual use, the semantic mismatch could confuse implementers of the constraint system.

Additionally, localPointA is hardcoded to the origin, limiting attachment to the center of mass of bodyA. Consider adding an overload that accepts a local attachment point for more flexibility:

[[nodiscard]] static FixedConstraint
CreateToWorld(Engine::Entity body, const glm::vec3 &localPoint, const glm::vec3 &worldPoint,
              const ConstraintSettings &constraintSettings = ConstraintSettings::Rigid())

♻️ Optional: Clarify world constraint storage

If the dual meaning of localPointB becomes problematic, consider adding a dedicated field or using a std::variant to explicitly differentiate between local points and world positions. However, the current approach is acceptable if clearly documented in the constraint system implementation.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between 3ccc246 and 256573e.

• src/plugin/physics/src/Physics.hpp
• src/plugin/physics/src/component/ConstraintInternal.hpp
• src/plugin/physics/src/component/ConstraintSettings.hpp
• src/plugin/physics/src/component/DistanceConstraint.hpp
• src/plugin/physics/src/component/FixedConstraint.hpp
• src/plugin/physics/src/component/PointConstraint.hpp
• src/plugin/physics/src/exception/ConstraintError.hpp
• src/plugin/physics/src/plugin/PhysicsPlugin.cpp
• src/plugin/physics/src/system/ConstraintHelpers.cpp
• src/plugin/physics/src/system/ConstraintHelpers.hpp
• src/plugin/physics/src/system/ConstraintSystem.cpp
• src/plugin/physics/src/system/ConstraintSystem.hpp
• src/plugin/physics/src/utils/JoltConversions.hpp

• src/plugin/physics/src/exception/ConstraintError.hpp
• src/plugin/physics/src/plugin/PhysicsPlugin.cpp
• src/plugin/physics/src/component/DistanceConstraint.hpp
• src/plugin/physics/src/component/ConstraintInternal.hpp
• src/plugin/physics/src/Physics.hpp
• src/plugin/physics/src/system/ConstraintHelpers.hpp

src/plugin/physics/src/component/ConstraintSettings.hpp (5)

• nodiscard (136-144)
• nodiscard (156-164)
• nodiscard (176-184)
• nodiscard (190-195)
• nodiscard (201-201)

src/plugin/physics/src/system/ConstraintHelpers.hpp (1)

• registry (31-31)

src/engine/src/entity/Entity.hpp (1)

• static_cast (65-65)

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

41-46: LGTM! Clean implementation following existing patterns.

The new ToJoltRVec3 conversion function correctly mirrors the ToJoltVec3 pattern and completes the bidirectional conversion support (since FromJoltRVec3 already exists). The implicit float→double conversion is safe and lossless, and the documentation clearly explains its purpose for large-world constraint positions.

src/plugin/physics/src/component/ConstraintSettings.hpp (1)

115-123: Verify enableCollision is actually applied to Jolt constraint settings (and polarity is correct).

The setting is exposed publicly (Line 115-123) but I don’t see it used in the handlers here; if it’s handled inside CreateConstraintGeneric, please double-check it’s wired for all constraint types and matches Jolt’s “disable collision between bodies” semantics.

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

55-86: [Rewritten review comment]
[Classification tag]

---

5-24: Use ctx().contains<Engine::Core*>() to check key existence before ctx().get().

The concern about ctx().get<Engine::Core*>() lacking a presence check is valid—the code only null-checks the pointer but doesn't verify the key exists in the context. However, the proposed solution using ctx().find<Engine::Core*>() does not exist in entt's API used by this codebase. The correct pattern, used in Core.inl, is:

if (!registry.ctx().contains<Engine::Core *>())
{
    Log::Error(fmt::format("Cannot create {}: Engine::Core not available", constraintName));
    return std::nullopt;
}
auto *corePtr = registry.ctx().get<Engine::Core *>();

The ctx().get() call will throw an exception if the key is missing, which can occur if ConstraintContext::Create is called before context initialization. Add the contains() check to match the defensive pattern already established in Core.inl.

Likely an incorrect or invalid review comment.

src/plugin/physics/src/component/PointConstraint.hpp (1)

84-112: World-constraint space is correctly handled in code but lacks test coverage.

localPointB has dual semantics (local for body-to-body, world for body-to-world), which is documented in the component (lines 105–111) and correctly handled in CreateJoltConstraint (ConstraintHelpers.hpp:69), which explicitly overrides mSpace to WorldSpace for world constraints. However, this critical path is not covered by any runtime test. Add at least one test for CreateToWorldWithOffset (or similar body-to-world constraint with an offset) to ensure the world-space conversion works as intended.

Also applies to: 171-215

src/plugin/physics/src/component/FixedConstraint.hpp (5)

1-31: LGTM: Clean header structure and includes.

The copyright header, include guards, and dependencies are well-organized.

---

33-70: LGTM: Excellent documentation with practical examples.

The struct documentation clearly explains the fixed constraint behavior and provides comprehensive usage examples.

---

72-131: LGTM: Well-structured member variables with clear documentation.

All member variables have sensible defaults and comprehensive documentation. The encoding of world constraints via invalid bodyB is clearly documented.

---

186-190: LGTM: IsWorldConstraint correctly identifies world constraints.

The helper method is consistent with the CreateToWorld implementation.

---

147-160: Entity validation is properly handled by ConstraintSystem during registration.

The factory method correctly leaves validation to the ConstraintSystem. When a FixedConstraint component is added to an entity, the on_construct hook triggers OnFixedConstraintConstruct, which calls ValidateConstraint to check that both entities are valid and prevents self-constraints (where bodyA == bodyB). Invalid constraints fail gracefully with error logging without creating the underlying Jolt constraint.

Actionable comments posted: 3

In @src/plugin/physics/src/component/DistanceConstraint.hpp:
- Around line 201-216: In DistanceConstraint::CreateWithRange, validate the
provided min and max so the created constraint never has minDistance >
maxDistance; if min > max swap them (or optionally assert) before assigning to
constraint.minDistance and constraint.maxDistance so callers cannot silently
create an inverted range—implement the swap in the CreateWithRange factory
(refer to the CreateWithRange static method and the min/max parameters) and then
continue assigning settings and returning the constraint.

In @src/plugin/physics/src/system/ConstraintHelpers.cpp:
- Around line 38-53: FinalizeConstraint currently calls
ctx.physicsSystem.AddConstraint before
ctx.registry.emplace<Component::ConstraintInternal>, risking a leak if emplace
throws; change the order to emplace the Component::ConstraintInternal into
ctx.registry first (using the same constructor args: entity, joltConstraint,
type, settings.breakForce, settings.breakTorque) and only after emplace succeeds
call ctx.physicsSystem.AddConstraint(joltConstraint) and Log::Debug; if emplace
fails, ensure you clean up the joltConstraint (destroy/delete or call the
appropriate Jolt release method) and log the error so the physics system is not
left with an untracked constraint.
- Around line 5-24: The code uses registry.ctx().get<Engine::Core *>() which is
undefined behavior if the type wasn't emplaced; replace those calls with
registry.try_ctx<Engine::Core *>() in ConstraintContext::Create (and the
analogous call in DestroyConstraint) so you receive a safe pointer that can be
nullptr-checked, or alternatively use registry.ctx().contains<Engine::Core *>()
before calling get; update the null-checks to validate the result of try_ctx and
keep the existing error logging and early returns.

src/plugin/physics/src/component/ConstraintInternal.hpp (1)

100-100: Consider aligning IsBreakable() logic with ConstraintSettings.

The IsBreakable() implementation checks if thresholds are > 0.0f, but ConstraintSettings::IsBreakable() (shown in relevant snippets) additionally verifies std::isfinite() to guard against infinity/NaN values. For consistency and robustness, consider matching that logic.

♻️ Proposed refinement

-    [[nodiscard]] bool IsBreakable() const { return breakForce > 0.0f || breakTorque > 0.0f; }
+    [[nodiscard]] bool IsBreakable() const {
+        const bool forceBreakable = breakForce > 0.0f && std::isfinite(breakForce);
+        const bool torqueBreakable = breakTorque > 0.0f && std::isfinite(breakTorque);
+        return forceBreakable || torqueBreakable;
+    }

src/plugin/physics/src/component/PointConstraint.hpp (2)

105-112: Clarify/guard the “localPointB stores worldPoint” convention

localPointB sometimes means “local on bodyB” and sometimes “world anchor”; that’s easy to accidentally treat as always-local in downstream code. At least consider renaming (e.g., anchorPointBOrWorld) or adding an explicit glm::vec3 worldPoint field for world constraints to make misuse harder.

Also applies to: 171-208

---

145-208: Reduce factory duplication (less drift risk)

All three factories manually assign the same fields; aggregate initialization makes it harder to accidentally forget a field when the struct evolves.

Proposed refactor

@@
     [[nodiscard]] static PointConstraint
     Create(Engine::Entity a, Engine::Entity b, const glm::vec3 &pointA = glm::vec3(0.0f),
            const glm::vec3 &pointB = glm::vec3(0.0f),
            const ConstraintSettings &constraintSettings = ConstraintSettings::Rigid())
     {
-        PointConstraint constraint;
-        constraint.bodyA = a;
-        constraint.bodyB = b;
-        constraint.localPointA = pointA;
-        constraint.localPointB = pointB;
-        constraint.settings = constraintSettings;
-        constraint.broken = false;
-        return constraint;
+        return PointConstraint{
+            .bodyA = a,
+            .bodyB = b,
+            .localPointA = pointA,
+            .localPointB = pointB,
+            .settings = constraintSettings,
+            .broken = false,
+        };
     }
@@
     CreateToWorld(Engine::Entity body, const glm::vec3 &worldPoint,
                   const ConstraintSettings &constraintSettings = ConstraintSettings::Rigid())
     {
-        PointConstraint constraint;
-        constraint.bodyA = body;
-        constraint.bodyB = Engine::Entity();      // Invalid entity = world
-        constraint.localPointA = glm::vec3(0.0f); // Center of body
-        constraint.localPointB = worldPoint;      // World position
-        constraint.settings = constraintSettings;
-        constraint.broken = false;
-        return constraint;
+        return PointConstraint{
+            .bodyA = body,
+            .bodyB = Engine::Entity(),       // Invalid entity = world
+            .localPointA = glm::vec3(0.0f),  // Center of body
+            .localPointB = worldPoint,       // World position
+            .settings = constraintSettings,
+            .broken = false,
+        };
     }
@@
     CreateToWorldWithOffset(Engine::Entity body, const glm::vec3 &worldPoint, const glm::vec3 &localPoint,
                             const ConstraintSettings &constraintSettings = ConstraintSettings::Rigid())
     {
-        PointConstraint constraint;
-        constraint.bodyA = body;
-        constraint.bodyB = Engine::Entity(); // Invalid entity = world
-        constraint.localPointA = localPoint;
-        constraint.localPointB = worldPoint; // World position
-        constraint.settings = constraintSettings;
-        constraint.broken = false;
-        return constraint;
+        return PointConstraint{
+            .bodyA = body,
+            .bodyB = Engine::Entity(), // Invalid entity = world
+            .localPointA = localPoint,
+            .localPointB = worldPoint, // World position
+            .settings = constraintSettings,
+            .broken = false,
+        };
     }

src/plugin/physics/src/component/DistanceConstraint.hpp (1)

286-293: Avoid float == for IsFixedDistance()

If minDistance/maxDistance are ever derived (e.g., from world positions), exact equality will be flaky.

Proposed fix (epsilon compare)

@@
 #include "ConstraintSettings.hpp"
 #include <Engine.hpp>
 #include <glm/glm.hpp>
+ #include <cmath>
@@
-    [[nodiscard]] bool IsFixedDistance() const { return minDistance == maxDistance; }
+    [[nodiscard]] bool IsFixedDistance() const { return std::fabs(minDistance - maxDistance) < 1e-5f; }

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between 256573e and 4e6e327.

• src/plugin/physics/src/Physics.hpp
• src/plugin/physics/src/component/ConstraintInternal.hpp
• src/plugin/physics/src/component/ConstraintSettings.hpp
• src/plugin/physics/src/component/DistanceConstraint.hpp
• src/plugin/physics/src/component/FixedConstraint.hpp
• src/plugin/physics/src/component/PointConstraint.hpp
• src/plugin/physics/src/exception/ConstraintError.hpp
• src/plugin/physics/src/plugin/PhysicsPlugin.cpp
• src/plugin/physics/src/system/ConstraintHelpers.cpp
• src/plugin/physics/src/system/ConstraintHelpers.hpp
• src/plugin/physics/src/system/ConstraintSystem.cpp
• src/plugin/physics/src/system/ConstraintSystem.hpp
• src/plugin/physics/src/utils/JoltConversions.hpp

• src/plugin/physics/src/system/ConstraintSystem.cpp
• src/plugin/physics/src/component/ConstraintSettings.hpp
• src/plugin/physics/src/system/ConstraintSystem.hpp
• src/plugin/physics/src/system/ConstraintHelpers.hpp
• src/plugin/physics/src/utils/JoltConversions.hpp

src/plugin/physics/src/component/ConstraintSettings.hpp (5)

• nodiscard (136-144)
• nodiscard (156-164)
• nodiscard (176-184)
• nodiscard (190-195)
• nodiscard (201-201)

src/plugin/physics/src/system/ConstraintHelpers.hpp (1)

• registry (31-31)

src/engine/src/entity/Entity.hpp (1)

• static_cast (65-65)

src/plugin/physics/src/component/ConstraintSettings.hpp (6)

• force (156-156)
• nodiscard (136-144)
• nodiscard (156-164)
• nodiscard (176-184)
• nodiscard (190-195)
• nodiscard (201-201)

• GitHub Check: Run tests (windows-latest)
• GitHub Check: Run tests (macos-latest)
• GitHub Check: Build and verify binaries (windows-latest)
• GitHub Check: Run tests (ubuntu-latest)

src/plugin/physics/src/exception/ConstraintError.hpp (1)

37-39: LGTM! Clean exception design.

The exception class follows standard C++ practices with appropriate inheritance from std::runtime_error and constructor forwarding. The documentation clearly outlines when this exception should be thrown.

src/plugin/physics/src/component/ConstraintInternal.hpp (2)

38-42: LGTM! Well-designed enum.

The enum uses strong typing (enum class) with an appropriate underlying type (uint8_t) and includes helpful documentation about degrees of freedom for each constraint type.

---

54-54: Constraint cleanup is properly handled via entt destroy hooks.

The raw pointer to JPH::Constraint is correctly managed by the Jolt physics system. The codebase registers on_destroy hooks for all constraint types (FixedConstraint, DistanceConstraint, PointConstraint) that automatically call RemoveConstraint() when entities are destroyed. This prevents dangling pointers and ensures proper cleanup without requiring an explicit destructor in ConstraintInternal.

src/plugin/physics/src/plugin/PhysicsPlugin.cpp (2)

8-8: LGTM! Proper include placement.

The include is correctly positioned with other system headers.

---

22-22: LGTM! Appropriate registration order.

Registering InitConstraintSystem after InitRigidBodySystem ensures the necessary dependencies are initialized first, which is the correct ordering for constraint setup.

src/plugin/physics/src/Physics.hpp (1)

8-8: LGTM! Well-organized includes.

The new constraint-related includes are properly organized into their respective categories (exceptions, components, systems) and follow the existing organizational pattern in the file.

Also applies to: 20-25, 31-31

src/plugin/physics/src/component/FixedConstraint.hpp (4)

82-131: LGTM! Well-structured component.

The struct members are appropriately documented, typed, and initialized with sensible defaults. The separation between configuration data and runtime state (broken flag) is clear.

---

147-160: LGTM! Clean factory method.

The factory method properly initializes all members with sensible defaults. The explicit initialization of broken = false is good for clarity even though it matches the member default.

---

172-184: LGTM! World constraint encoding is clear.

The factory method cleverly encodes world constraints by using an invalid bodyB entity. The comment on line 180 clearly documents that localPointB is repurposed to store the world position in this case, which is a reasonable design trade-off to avoid creating a separate component type.

---

190-190: LGTM! Consistent helper method.

The helper method correctly identifies world constraints by checking the validity of bodyB, consistent with the CreateToWorld() factory pattern.

Actionable comments posted: 5

In @src/plugin/physics/src/component/DistanceConstraint.hpp:
- Around line 234-249: The CreateToWorld factory stores a world-space position
into the member localPointB when bodyB is the invalid/world entity (see
DistanceConstraint::CreateToWorld), but the member comment currently says "local
body space" unconditionally; update the DistanceConstraint declaration comment
for localPointB to state that it is interpreted as a local-space point for
normal constraints and as a world-space point when IsWorldConstraint() returns
true, and optionally add a short note on how IsWorldConstraint() is determined
(e.g., bodyB == Engine::Entity()) so callers don’t mistakenly treat localPointB
as always local.
- Around line 80-153: DistanceConstraint can be default-constructed by the ECS
so bodyA, bodyB and settings need safe defaults; add an explicit default
constructor for DistanceConstraint that initializes bodyA and bodyB to a
known-invalid/null Entity (e.g., Engine::Entity{} or your engine's
invalid/entity sentinel) and initializes settings to a default-constructed
ConstraintSettings (ConstraintSettings{}), leaving other members as-is; update
any serialization/placement code to rely on this constructor so
emplace<DistanceConstraint>(e) produces a safe, initialized instance.

In @src/plugin/physics/src/system/ConstraintHelpers.cpp:
- Around line 5-24: ConstraintContext::Create uses
registry.ctx().get<Engine::Core *>() which will assert/crash if the context
value is missing; replace that call with registry.ctx().find<Engine::Core *>()
and null-check the returned pointer before dereferencing. Update the variable
name (e.g., corePtr) to be the pointer returned by find, keep the existing
Log::Error and early return behavior when null, and otherwise proceed to
dereference corePtr into coreRef and use it as before.
- Around line 55-86: In DestroyConstraint replace the unsafe
registry.ctx().get<Engine::Core *>() usage with registry.ctx().find<Engine::Core
*>() and null-check the returned pointer before dereferencing to avoid EnTT
context precondition violations; keep the existing checks for
Component::ConstraintInternal and IsValid(), call
physicsManagerRef.GetPhysicsSystem().RemoveConstraint(internal->constraint) as
before, and expand exception handling by adding a catch(const std::exception &e)
(after the specific ConstraintError and bad_alloc catches) to log any other
std::exception-derived errors so RemoveConstraint and its callees cannot throw
unhandled exceptions.

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

26-36: Minor ergonomics: entity conversion and log spam

You can likely pass entity directly to try_get thanks to Engine::Entity::operator entt::entity(); also consider whether Log::Error here will be too noisy in expected “body missing during authoring” cases.

src/plugin/physics/src/component/DistanceConstraint.hpp (1)

284-297: IsFixedDistance() float equality is brittle for authored/serialized values

For factory-created fixed constraints it’s fine, but for data that might be edited/serialized, consider an epsilon comparison.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between 4e6e327 and 0263366.

• src/plugin/physics/src/component/DistanceConstraint.hpp
• src/plugin/physics/src/system/ConstraintHelpers.cpp
• src/plugin/physics/src/system/ConstraintSystem.cpp

• src/plugin/physics/src/system/ConstraintSystem.cpp

src/plugin/physics/src/system/ConstraintHelpers.hpp (1)

• registry (31-31)

src/engine/src/entity/Entity.hpp (1)

• static_cast (65-65)

• GitHub Check: Build and verify binaries (windows-latest)
• GitHub Check: Run tests (ubuntu-latest)
• GitHub Check: Build and verify binaries (ubuntu-latest)
• GitHub Check: Run tests (windows-latest)
• GitHub Check: Run tests (macos-latest)

src/plugin/physics/src/component/DistanceConstraint.hpp (1)

202-220: Remove concern about mixed sentinel/non-sentinel values—the system intentionally supports them

The runtime validation in ConstraintSystem.cpp (line 77) explicitly allows mixed sentinel (< 0) and non-sentinel (>= 0) combinations by only checking maxDistance < minDistance when both are non-negative. The IsAutoDistance() method uses OR logic (minDistance < 0.0f || maxDistance < 0.0f), confirming mixed modes are expected. The swap logic also handles sentinels correctly—comparing -1.0f > value will always be false for positive values, preserving the intended order. No action needed.

Actionable comments posted: 2

In @src/plugin/physics/src/system/ConstraintHelpers.cpp:
- Around line 14-21: The Create() function calls
coreRef.GetResource<Resource::PhysicsManager>() unguarded which can throw
Exception::MissingResourceError; wrap that call in the same protection used
elsewhere (either a try-catch that catches MissingResourceError and logs a clear
message including constraintName before returning std::nullopt, or perform a
pre-check on resource presence like the Core lookup pattern) so Create() mirrors
the guarded behavior used in DestroyConstraint(); reference
GetResource<Resource::PhysicsManager>(), IsPhysicsActivated(), Create(),
DestroyConstraint(), and constraintName when implementing the change.
- Around line 38-68: In FinalizeConstraint, when
ctx.physicsSystem.AddConstraint(joltConstraint) throws, ensure you call
ctx.physicsSystem.RemoveConstraint(joltConstraint) before removing the
Component::ConstraintInternal from the registry so Jolt releases its reference
and avoids a leak; update both catch blocks to call
RemoveConstraint(joltConstraint) then
ctx.registry.remove<Component::ConstraintInternal>(entity) (mirroring the
cleanup order used in DestroyConstraint) and retain the existing error logging
and return.

src/plugin/physics/src/component/ConstraintSettings.hpp (1)

58-79: Consider adding parameter validation for stiffness and damping.

The documentation states that stiffness and damping should be in the range [0.0, 1.0], but there's no validation enforcing these bounds. The Soft() factory method accepts any float values, which could lead to invalid configurations.

🛡️ Suggested validation approach

Consider adding validation in the factory methods or providing a separate validation function:

 [[nodiscard]] static ConstraintSettings Soft(float stiff = 0.5f, float damp = 0.1f)
 {
+    // Clamp to valid ranges
+    stiff = std::clamp(stiff, 0.0f, 1.0f);
+    damp = std::clamp(damp, 0.0f, 1.0f);
+    
     ConstraintSettings settings;
     settings.stiffness = stiff;
     settings.damping = damp;
     settings.breakForce = 0.0f;
     settings.breakTorque = 0.0f;
     return settings;
 }

Alternatively, add a Validate() method that can be called before use.

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

69-74: Document the spring settings scaling factors.

The stiffness and damping values are multiplied by magic numbers (10000.0f and 100.0f) when mapping to Jolt's spring settings. These scaling factors should be documented to explain the rationale and make future maintenance easier.

📝 Suggested documentation

             if (!constraint.settings.IsRigid())
             {
                 joltSettings.mLimitsSpringSettings.mMode = JPH::ESpringMode::StiffnessAndDamping;
+                // Scale to Jolt's expected ranges: stiffness [0.0, 1.0] -> [0, 10000], damping [0.0, 1.0] -> [0, 100]
                 joltSettings.mLimitsSpringSettings.mStiffness = constraint.settings.stiffness * 10000.0f;
                 joltSettings.mLimitsSpringSettings.mDamping = constraint.settings.damping * 100.0f;
             }

Alternatively, define these as named constants at the file or namespace level.

src/plugin/physics/src/component/FixedConstraint.hpp (1)

71-90: Consider consistent default initialization for entity members.

Unlike DistanceConstraint (lines 88, 93), FixedConstraint does not provide default initializers for bodyA and bodyB. While factory methods always assign these, adding explicit defaults would be more defensive and consistent across constraint types.

Suggested change for consistency

 struct FixedConstraint {
     //========================================================================
     // Constraint Bodies
     //========================================================================

     /**
      * @brief First body entity (the entity this component is attached to)
      *
      * This should be the entity that owns this constraint component.
      * World constraints are encoded by setting `bodyB` invalid (see CreateToWorld()).
      */
-    Engine::Entity bodyA;
+    Engine::Entity bodyA = Engine::Entity{};

     /**
      * @brief Second body entity (the entity to connect to)
      *
      * The other entity involved in the constraint.
      */
-    Engine::Entity bodyB;
+    Engine::Entity bodyB = Engine::Entity{};

src/plugin/physics/src/component/DistanceConstraint.hpp (1)

297-301: Float equality comparison may cause unexpected behavior.

IsFixedDistance() uses direct float equality (minDistance == maxDistance). This works correctly when values are assigned programmatically from the same source (e.g., in Create), but could be fragile if distances are computed or modified. Consider if an epsilon comparison is warranted for robustness.

Optional: Use epsilon comparison

-    [[nodiscard]] bool IsFixedDistance() const { return minDistance == maxDistance; }
+    [[nodiscard]] bool IsFixedDistance() const { 
+        return std::abs(minDistance - maxDistance) < 1e-6f; 
+    }

src/plugin/physics/src/system/ConstraintHelpers.hpp (2)

101-141: Consider catching std::exception for consistency with other helper functions.

FinalizeConstraint (in .cpp) catches both ConstraintError and std::exception, but CreateConstraintGeneric only catches ConstraintError and std::bad_alloc. If configurator or extraValidate callbacks throw other exceptions, they would propagate uncaught.

Add std::exception catch block

     catch (const std::bad_alloc &e)
     {
         Log::Critical(fmt::format("{} bad alloc: {}", constraintName, e.what()));
     }
+    catch (const std::exception &e)
+    {
+        Log::Error(fmt::format("{} unexpected error: {}", constraintName, e.what()));
+    }
 }

---

34-36: Minor: static keyword on template functions in headers is redundant.

Template functions in headers have internal linkage by default. The static keyword on lines 35, 56, and 103 is unnecessary, though harmless.

Also applies to: 55-57, 101-104

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between 0263366 and 484aa60.

• src/plugin/physics/src/Physics.hpp
• src/plugin/physics/src/component/ConstraintInternal.hpp
• src/plugin/physics/src/component/ConstraintSettings.hpp
• src/plugin/physics/src/component/DistanceConstraint.hpp
• src/plugin/physics/src/component/FixedConstraint.hpp
• src/plugin/physics/src/component/PointConstraint.hpp
• src/plugin/physics/src/exception/ConstraintError.hpp
• src/plugin/physics/src/plugin/PhysicsPlugin.cpp
• src/plugin/physics/src/system/ConstraintHelpers.cpp
• src/plugin/physics/src/system/ConstraintHelpers.hpp
• src/plugin/physics/src/system/ConstraintSystem.cpp
• src/plugin/physics/src/system/ConstraintSystem.hpp
• src/plugin/physics/src/utils/JoltConversions.hpp

• src/plugin/physics/src/plugin/PhysicsPlugin.cpp
• src/plugin/physics/src/component/ConstraintInternal.hpp
• src/plugin/physics/src/Physics.hpp
• src/plugin/physics/src/system/ConstraintSystem.hpp

src/plugin/physics/src/component/ConstraintSettings.hpp (5)

• nodiscard (136-144)
• nodiscard (156-164)
• nodiscard (176-184)
• nodiscard (190-195)
• nodiscard (201-201)

src/plugin/physics/src/system/ConstraintHelpers.hpp (4)

• registry (31-31)
• CreateConstraintGeneric (103-141)
• CreateConstraintGeneric (103-104)
• DestroyConstraint (99-99)

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

• DestroyConstraint (70-105)
• DestroyConstraint (70-70)

src/plugin/physics/src/system/ConstraintHelpers.hpp (1)

• registry (31-31)

src/engine/src/entity/Entity.hpp (1)

• static_cast (65-65)

src/plugin/physics/src/system/ConstraintHelpers.cpp (6)

• GetBodyInternal (26-36)
• GetBodyInternal (26-27)
• Create (5-24)
• Create (5-5)
• FinalizeConstraint (38-68)
• FinalizeConstraint (38-40)

src/plugin/physics/src/component/FixedConstraint.hpp (3)

• nodiscard (147-160)
• nodiscard (172-184)
• nodiscard (190-190)

src/plugin/physics/src/component/ConstraintInternal.hpp (2)

• nodiscard (94-94)
• nodiscard (100-100)

• GitHub Check: Run tests (windows-latest)
• GitHub Check: Build and verify binaries (macos-latest)
• GitHub Check: Run tests (macos-latest)
• GitHub Check: Build and verify binaries (windows-latest)
• GitHub Check: Run tests (ubuntu-latest)
• GitHub Check: Build and verify binaries (ubuntu-latest)

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

41-46: LGTM: Clean conversion utility addition.

The new ToJoltRVec3 function follows the existing conversion pattern and is well-documented. The note about RVec3 usage for large worlds is helpful context.

src/plugin/physics/src/component/ConstraintSettings.hpp (1)

201-201: Potential inconsistency in IsRigid() implementation.

The IsRigid() method checks stiffness >= 1.0f, but the documentation states stiffness should be in the range [0.0, 1.0]. This allows stiffness values greater than 1.0 to pass the rigid check, which may be intentional but seems inconsistent with the documented range.

Should stiffness values > 1.0 be considered valid? If not, consider changing the check to stiffness == 1.0f or adding validation to prevent out-of-range values.

src/plugin/physics/src/exception/ConstraintError.hpp (1)

27-39: LGTM: Clean exception type definition.

The ConstraintError exception class follows standard C++ exception patterns and provides clear documentation of the scenarios where it should be thrown.

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

119-142: LGTM: Solid initialization pattern with proper guards.

The InitConstraintSystem function uses a robust initialization pattern:

• Stores the Core pointer in the registry context for later access
• Uses a tag type to prevent duplicate initialization
• Registers all constraint lifecycle hooks

The one-time initialization guard is a clean approach to prevent double-registration.

---

76-84: The distance constraint validation is intentionally correct and does not require changes.

The validation logic properly accounts for the -1.0f sentinel value used for auto-detect functionality. Negative distance values (specifically -1.0f) are semantically valid and documented as "Auto-detect from initial body positions" in the DistanceConstraint component. The validation correctly skips the minDistance < maxDistance comparison when either value is negative, allowing auto-detect to work as designed.

The IsAutoDistance() helper method in DistanceConstraint explicitly supports this by returning true when either value is negative. The proposed fix would incorrectly reject valid auto-detect configurations.

Likely an incorrect or invalid review comment.

src/plugin/physics/src/component/PointConstraint.hpp (1)

76-215: LGTM: Well-structured constraint component with excellent documentation.

The PointConstraint component is well-designed:

• Clear separation of concerns (bodies, attachment points, settings, runtime state)
• Comprehensive documentation with practical examples (pendulum, ragdoll, elbow joint)
• Consistent factory method pattern matching other constraint types
• Proper use of [[nodiscard]] attributes

The examples in the documentation are particularly helpful for understanding the use cases.

src/plugin/physics/src/component/FixedConstraint.hpp (2)

147-160: LGTM!

The Create factory method is well-designed with sensible defaults and explicit initialization of all fields.

---

172-184: LGTM!

The CreateToWorld factory method correctly encodes a world constraint by using an invalid entity for bodyB and repurposing localPointB as a world-space anchor point. The documentation clearly explains this convention.

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

70-105: LGTM!

The DestroyConstraint function has comprehensive exception handling with appropriate log levels (Warn, Critical, Error) for different exception types. The early returns for missing core/physics are correct defensive patterns.

src/plugin/physics/src/component/DistanceConstraint.hpp (2)

176-191: LGTM!

The Create factory method correctly sets both minDistance and maxDistance to the same value for a fixed-distance (rope) constraint.

---

207-225: Good defensive handling of inverted min/max values.

The std::swap on line 212-213 ensures the constraint is always configured correctly even if the caller provides inverted arguments. This aligns with the commit message mentioning "ensure min and max values are swapped in DistanceConstraint."

src/plugin/physics/src/system/ConstraintHelpers.hpp (2)

34-50: LGTM!

The ValidateConstraint template provides essential validation: checking that bodyA is valid and preventing self-constraint. The error messages are clear and include the constraint name for debugging.

---

55-93: LGTM!

The CreateJoltConstraint template correctly handles both world constraints and body-to-body constraints with proper body locking. The use of JPH::BodyLockWrite for single-body and JPH::BodyLockMultiWrite for two-body scenarios follows Jolt's threading guidelines.

Actionable comments posted: 2

In @src/plugin/physics/src/system/ConstraintHelpers.cpp:
- Around line 105-109: The current sequence calls
physicsManagerRef.GetPhysicsSystem().RemoveConstraint(internal->constraint) and
then registry.remove<Component::ConstraintInternal>(entity), which can leave
ConstraintInternal stale if RemoveConstraint throws; fix by ensuring the
component is removed regardless — either remove the
Component::ConstraintInternal from the registry before calling RemoveConstraint,
or wrap the RemoveConstraint call in a try/catch (or RAII/finally-style) and
always call registry.remove<Component::ConstraintInternal>(entity) in the
cleanup path, keeping the Log::Debug(fmt::format("Destroyed {} for entity {}",
constraintName, entt::to_integral(entity))) as appropriate after successful
removal or inside the protected cleanup to reflect final state.
- Around line 64-83: The component is emplaced into the registry before
ctx.physicsSystem.AddConstraint succeeds, causing RemoveConstraint to be called
on a never-added constraint and leaking the Jolt constraint reference; move
ctx.registry.emplace_or_replace<Component::ConstraintInternal>(...) to after
ctx.physicsSystem.AddConstraint(joltConstraint) completes successfully, and
ensure the constraint reference is released on error by either calling
joltConstraint->Release() in the error handlers or converting the raw pointer to
a JPH::Ref<> (or implementing Release() in Component::ConstraintInternal
destructor) so the constraint is properly reference-counted and freed when the
component is removed.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between 484aa60 and 902a01b.

• src/plugin/physics/src/system/ConstraintHelpers.cpp

src/plugin/physics/src/system/ConstraintHelpers.hpp (1)

• registry (31-31)

src/engine/src/entity/Entity.hpp (1)

• static_cast (65-65)

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

5-40: LGTM!

The ConstraintContext::Create function demonstrates solid defensive programming with proper null checks, specific exception handling for MissingResourceError, and validation of physics system activation before returning the context.

---

42-52: LGTM!

The function safely retrieves the internal rigid body component with proper validation. The explicit cast to entt::entity is acceptable, though Engine::Entity already provides an implicit conversion operator.

Actionable comments posted: 2

In @src/plugin/physics/src/system/ConstraintHelpers.cpp:
- Around line 106-108: The component is removed before the physics system
cleanup, risking leaks if RemoveConstraint throws; change the order so
physicsManagerRef.GetPhysicsSystem().RemoveConstraint(internal->constraint) is
called before registry.remove<Component::ConstraintInternal>(entity), and wrap
the RemoveConstraint call in a try/catch (or check its failure return) to avoid
dropping the component on error so you can retry or log and preserve state.
- Around line 54-87: FinalizeConstraint currently emplaces
Component::ConstraintInternal into the registry before calling
ctx.physicsSystem.AddConstraint, so if AddConstraint throws the constraint is
released but the component remains pointing to a freed pointer; remove the
premature ctx.registry.emplace_or_replace<Component::ConstraintInternal>(entity,
joltConstraint, type, settings.breakForce, settings.breakTorque) call (the first
one) and only emplace the Component::ConstraintInternal after AddConstraint
succeeds (keep the existing emplace after the try/catch), ensuring any exception
path releases the Jolt constraint and does not leave a dangling component.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between 902a01b and e30a15c.

• src/plugin/physics/src/system/ConstraintHelpers.cpp

src/plugin/physics/src/system/ConstraintHelpers.hpp (1)

• registry (31-31)

src/engine/src/entity/Entity.hpp (1)

• static_cast (65-65)

• GitHub Check: Run tests (macos-latest)
• GitHub Check: Build and verify binaries (ubuntu-latest)
• GitHub Check: Run tests (windows-latest)
• GitHub Check: Build and verify binaries (windows-latest)
• GitHub Check: Run tests (ubuntu-latest)
• GitHub Check: Build and verify binaries (macos-latest)

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

5-40: LGTM: Robust error handling for constraint context creation.

The validation chain (Core availability → PhysicsManager access → physics activation) is thorough, with specific exception handling and descriptive error messages.

---

42-52: LGTM: Clean validation with appropriate error handling.

The function correctly validates the rigid body component and provides helpful context in error messages.

Actionable comments posted: 2

In @src/plugin/physics/src/system/ConstraintHelpers.cpp:
- Around line 5-40: The logs in ConstraintContext::Create call fmt::format with
constraintName which may be nullptr; guard against this by introducing a safe
name (e.g., const char* safeName = constraintName ? constraintName : "<null>")
and use safeName in all fmt::format calls (the Log::Error messages and any other
places in this function) so no nullptr is passed into fmt's C-string formatting.
- Around line 54-85: In FinalizeConstraint, emplace_or_replace can throw after
ctx.physicsSystem.AddConstraint succeeds, leaking the newly added Jolt
constraint and double-registering on updates; fix by first checking for an
existing Component::ConstraintInternal on the entity and if present remove its
constraint from ctx.physicsSystem and release it before adding/replacing, then
wrap the ctx.registry.emplace_or_replace<Component::ConstraintInternal>(...)
call in a try-catch that on any exception removes the newly added joltConstraint
from ctx.physicsSystem, calls joltConstraint->Release(), and rethrows or logs
the error; apply these changes around the symbols FinalizeConstraint,
ctx.physicsSystem.AddConstraint / RemoveConstraint, and
emplace_or_replace<Component::ConstraintInternal> to ensure rollback on
allocation/registry failures.

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

42-52: Avoid double-casting Engine::Entity (prefer explicit conversion helper / entt::to_entity-style).
static_cast<entt::entity>(entity) works, but it’s easy to get wrong if Engine::Entity later changes semantics (and it reads a bit “forceful” given the implicit conversion operator in Entity.hpp).

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between e30a15c and bda1623.

• src/plugin/physics/src/system/ConstraintHelpers.cpp

src/plugin/physics/src/system/ConstraintHelpers.hpp (1)

• registry (31-31)

src/engine/src/entity/Entity.hpp (1)

• static_cast (65-65)

• GitHub Check: Run tests (ubuntu-latest)
• GitHub Check: Run tests (macos-latest)
• GitHub Check: Build and verify binaries (macos-latest)
• GitHub Check: Build and verify binaries (windows-latest)
• GitHub Check: Build and verify binaries (ubuntu-latest)
• GitHub Check: Run tests (windows-latest)

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

87-122: Confirm Jolt constraint lifetime: asymmetric Release() calling pattern suggests potential leak.

The creation path calls joltConstraint->Release() on errors (lines 71, 77), but DestroyConstraint() calls only RemoveConstraint() without ever calling Release(). This asymmetry is suspicious—if RemoveConstraint() doesn't internally release the refcount, this leaks memory. The behavior of Jolt's RemoveConstraint() is unclear from the codebase and needs verification against Jolt documentation.

Additionally, DestroyConstraint() duplicates the Core/PhysicsManager retrieval logic that already exists in ConstraintContext::Create() (lines 5–40). Refactor to reuse ConstraintContext::Create() for consistency and to reduce error-prone duplication.

Actionable comments posted: 6

In @src/plugin/physics/src/exception/ConstraintUnexpectedError.hpp:
- Around line 23-36: ConstraintUnexpectedError should inherit from
std::runtime_error so it can store and return messages; change the class base
from std::exception to std::runtime_error and provide/enable the same
message-bearing constructors (i.e., forward constructors or add an explicit
constructor taking const std::string&/const char*), mirroring how
ConstraintError and RigidBodyError are implemented so call sites in
ConstraintHelpers.cpp using .what() receive meaningful output.

In @src/plugin/physics/src/system/ConstraintHelpers.cpp:
- Around line 41-51: The log call in GetBodyInternal uses constraintName
directly which can be a null C-string; null-guard constraintName (and similarly
bodyName if desired) before formatting to avoid UB — e.g., compute a safe string
like safeConstraint = constraintName ? constraintName : "<unknown>" (matching
the existing safeName pattern used elsewhere) and use that in the fmt::format
call inside GetBodyInternal to produce the error message.
- Around line 110-145: DestroyConstraint leaks the Jolt constraint because after
physicsManagerRef.GetPhysicsSystem().RemoveConstraint(constraint) you must call
constraint->Release() (matching the creation path) and also guard logging
against a null constraintName by introducing a local safeName (e.g., const char
*safeName = constraintName ? constraintName : "<constraint>") and using safeName
in all fmt::format() calls; update the block handling
Component::ConstraintInternal (and the Registry removal via
registry.remove<Component::ConstraintInternal>) to call Release() before or
after removal as appropriate and replace constraintName usages with safeName in
all logs and exception handlers.
- Around line 53-108: The FinalizeConstraint flow currently only catches
Physics::Exception::ConstraintUnexpectedError around
ctx.registry.emplace_or_replace<Component::ConstraintInternal>, which can let
other exceptions (e.g., std::bad_alloc) leak a registered constraint; wrap the
emplace_or_replace call in a try/catch that catches std::exception (or at
minimum std::bad_alloc) in addition to the Physics exceptions, and in those
catch blocks call ctx.physicsSystem.RemoveConstraint(joltConstraint) and
joltConstraint->Release() before returning; ensure the same cleanup is performed
for any non-Physics exception thrown after ctx.physicsSystem.AddConstraint(...)
succeeds so the constraint is not left registered without a ConstraintInternal
component.

In @src/plugin/physics/src/system/ConstraintHelpers.hpp:
- Around line 35-51: ValidateConstraint currently formats constraintName
directly which is unsafe if null; add a local safeName (e.g., use constraintName
if non-null else a fallback like "<unnamed constraint>") at the top of the
function and use safeName in the Log::Error calls instead of constraintName so
null pointers cannot cause UB; keep the template and checks (bodyA.IsValid,
bodyA == bodyB) unchanged and only replace the formatted name usage in the
Log::Error invocations.

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

5-39: ConstraintContext::Create: catch set looks suspicious; consider catching std::exception instead of ConstraintUnexpectedError here.

coreRef.GetResource<Resource::PhysicsManager>() plausibly throws MissingResourceError (already handled) and other standard exceptions; catching ConstraintUnexpectedError here seems unlikely unless GetResource is wrapped to throw it.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between bda1623 and 1a27215.

• src/plugin/physics/src/Physics.hpp
• src/plugin/physics/src/exception/ConstraintUnexpectedError.hpp
• src/plugin/physics/src/system/ConstraintHelpers.cpp
• src/plugin/physics/src/system/ConstraintHelpers.hpp

• src/plugin/physics/src/Physics.hpp

src/plugin/physics/src/system/ConstraintHelpers.cpp (6)

• GetBodyInternal (41-51)
• GetBodyInternal (41-42)
• Create (5-39)
• Create (5-5)
• FinalizeConstraint (53-108)
• FinalizeConstraint (53-55)

src/plugin/physics/src/system/ConstraintHelpers.hpp (1)

56-94: No changes needed — BodyLockMultiWrite handles lock ordering internally.

Jolt's BodyLockMultiWrite prevents deadlocks by computing a MutexMask from the provided BodyIDs and acquiring the underlying mutexes in a consistent, deterministic order (by mutex index). This internal mechanism ensures safe locking regardless of the BodyID array order, so sorting is unnecessary and was based on a misunderstanding of the API. The current code is safe.

Likely an incorrect or invalid review comment.