Actionable comments posted: 5

In @src/plugin/physics/src/builder/VehicleBuilder.hpp:
- Around line 264-269: The wheel position defaults in VehicleBuilder.hpp
(_wheelPositions) do not match the hardcoded values in VehicleSystem.cpp causing
visual/physics mismatch; change _wheelPositions to use the same default
coordinates (±0.9f, -0.3f, ±1.2f) or, better, refactor both
VehicleBuilder::_wheelPositions and the logic in VehicleSystem.cpp to read a
single source of truth (e.g., a shared VehicleConfig/getWheelOffsets function)
so both builder and system use identical configured wheel offsets.
- Around line 218-230: The current use of
registry.ctx().emplace<std::array<Engine::Entity, 4>>(wheelEntities) /
registry.ctx().erase(...) to pass wheelEntities into OnVehicleConstruct is
thread-unsafe; instead store the wheel entity references on the Vehicle
component itself (or another per-entity structure) before calling
chassis.AddComponent<Component::Vehicle>(core, _vehicle) so OnVehicleConstruct
can read them from that component (or a per-entity holder) rather than global
registry context; remove the temporary ctx emplace/erase and ensure any logic
that previously read registry.ctx() uses the Vehicle component (or a per-entity
key) so Build() is safe for concurrent calls.

In @src/plugin/physics/src/component/VehicleController.hpp:
- Around line 29-47: The setters (SetForward, SetSteering, SetBrake,
SetHandBrake) use std::clamp but the header lacks the required <algorithm>
include; add the #include <algorithm> at the top of VehicleController.hpp so
std::clamp is defined and the file compiles.

In @src/plugin/physics/src/component/WheelSettings.hpp:
- Around line 8-14: The header defines enum class WheelIndex using uint8_t but
does not include the fixed-width integer header; add the missing include for
<cstdint> at the top of the WheelSettings.hpp file so uint8_t is guaranteed to
be defined and the WheelIndex enum compiles on all platforms.

In @src/plugin/physics/src/system/VehicleSystem.cpp:
- Around line 162-169: The code in VehicleSystem.cpp currently uses a hardcoded
wheelPositions array which overrides per-vehicle configuration; replace this by
sourcing positions from the Vehicle component or wheel entity transforms so
VehicleBuilder::SetWheelPositions() and custom setups are respected. Modify the
loop that builds constraintSettings.mWheels (where
CreateJoltWheelSettings(vehicle.wheels[i], wheelPositions[i], isRear) is called)
to fetch each wheel position from the Vehicle component (e.g.,
vehicle.wheelPositions[i]) or by querying the wheel entity transform from the
registry/context and passing that position into CreateJoltWheelSettings instead
of the hardcoded wheelPositions. Ensure fallback behavior if positions are
missing (e.g., compute default from transform or keep existing relative
offsets).

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

96-98: Clarify "set as embedded" comment.

The comment states "set as embedded" but these are std::shared_ptr, not embedded objects. Consider revising to clarify the intended meaning, such as "Anti-rollbar instances (managed via shared_ptr for Jolt integration)" or similar.

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

26-27: Consider using proper API for time manipulation.

The test directly accesses _elapsedTime, which appears to be a private member (underscore prefix). If a public setter exists, prefer using it to maintain encapsulation.

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

25-31: Inconsistent test initialization pattern.

This test uses a different initialization sequence compared to other vehicle tests in this PR:

• This test: core.GetScheduler<Engine::Scheduler::Startup>().RunSystems() (Line 31)
• Other tests (e.g., VehicleForwardMovementTest.cpp, VehicleWheelTransformSyncTest.cpp): core.RunSystems()

Both approaches may work, but the inconsistency could lead to confusion or mask subtle timing issues. Consider aligning the initialization pattern across all vehicle tests for consistency and easier maintenance.

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

87-93: wheelBodyIDs array is never populated.

The wheelBodyIDs array is initialized empty and stored in VehicleInternal without being populated with valid body IDs. Per the comment on line 62-63, Jolt wheeled vehicles use raycasts rather than separate wheel bodies, but if wheelBodyIDs is truly unused, consider removing it from VehicleInternal to avoid confusion.

---

253-258: Potential double-delete risk if RemoveStepListener or RemoveConstraint throws.

If RemoveStepListener or RemoveConstraint fails or throws before reaching line 256, the constraint won't be deleted, causing a leak. Conversely, if they succeed but deletion fails, the pointers are left dangling. Consider using RAII or ensuring exception safety.

src/plugin/physics/src/builder/VehicleBuilder.hpp (1)

171-178: Consider using WheelIndex enum names in error messages.

The error message uses numeric indices which are less informative. Consider mapping to enum names for better developer experience.

💡 Example improvement

static constexpr const char* WheelIndexNames[] = {"FrontLeft", "FrontRight", "RearLeft", "RearRight"};
// ...
throw std::runtime_error("VehicleBuilder: Wheel mesh not set for " + std::string(WheelIndexNames[i]));

Configuration used: defaults

Review profile: CHILL

Plan: Pro

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

• src/plugin/object/src/utils/ShapeGenerator.cpp
• src/plugin/object/src/utils/ShapeGenerator.hpp
• src/plugin/physics/src/Physics.hpp
• src/plugin/physics/src/builder/VehicleBuilder.hpp
• src/plugin/physics/src/component/RigidBody.hpp
• src/plugin/physics/src/component/Vehicle.hpp
• src/plugin/physics/src/component/VehicleController.hpp
• src/plugin/physics/src/component/VehicleInternal.hpp
• src/plugin/physics/src/component/WheelSettings.hpp
• src/plugin/physics/src/plugin/PhysicsPlugin.cpp
• src/plugin/physics/src/system/VehicleControlSystem.cpp
• src/plugin/physics/src/system/VehicleControlSystem.hpp
• src/plugin/physics/src/system/VehicleSystem.cpp
• src/plugin/physics/src/system/VehicleSystem.hpp
• src/plugin/physics/src/system/WheelTransformSyncSystem.cpp
• src/plugin/physics/src/system/WheelTransformSyncSystem.hpp
• src/plugin/physics/tests/VehicleCreationTest.cpp
• src/plugin/physics/tests/VehicleForwardMovementTest.cpp
• src/plugin/physics/tests/VehicleGravityDropTest.cpp
• src/plugin/physics/tests/VehicleSteeringTest.cpp
• src/plugin/physics/tests/VehicleWheelTransformSyncTest.cpp

src/plugin/physics/src/builder/VehicleBuilder.hpp (2)

• core (164-233)
• core (164-164)

src/plugin/object/src/utils/ShapeGenerator.hpp (1)

• GenerateCylinderMesh (78-79)

src/plugin/physics/src/builder/VehicleBuilder.hpp (2)

• core (164-233)
• core (164-164)

src/plugin/physics/src/builder/VehicleBuilder.hpp (4)

• core (164-233)
• core (164-164)
• chassisMesh (54-65)
• chassisMesh (54-57)

src/plugin/object/src/utils/ShapeGenerator.hpp (2)

• GenerateCubeMesh (42-42)
• GenerateWheelMesh (92-92)

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

• vehicle (103-120)

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

• InitVehicleSystem (270-276)
• InitVehicleSystem (270-270)

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

• InitVehicleSystem (13-13)

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

• PhysicsUpdate (15-15)

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

• VehicleControlSystem (14-14)

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

• SyncTransformWithPhysics (42-42)

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

• WheelTransformSyncSystem (14-14)

src/plugin/physics/src/builder/VehicleBuilder.hpp (2)

• core (164-233)
• core (164-164)

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

• vehicle (103-120)

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

• vehicleConstraint (46-46)

src/plugin/physics/src/builder/VehicleBuilder.hpp (4)

• core (164-233)
• core (164-164)
• chassisMesh (54-65)
• chassisMesh (54-57)

src/plugin/object/src/utils/ShapeGenerator.cpp (4)

• GenerateCubeMesh (8-132)
• GenerateCubeMesh (8-8)
• GenerateWheelMesh (359-384)
• GenerateWheelMesh (359-359)

src/plugin/object/src/utils/ShapeGenerator.hpp (2)

• GenerateCubeMesh (42-42)
• GenerateWheelMesh (92-92)

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

• vehicle (103-120)

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

• static_cast (65-65)

src/plugin/physics/src/builder/VehicleBuilder.hpp (2)

• core (164-233)
• core (164-164)

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

• static_cast (65-65)

src/plugin/physics/src/builder/VehicleBuilder.hpp (2)

• core (164-233)
• core (164-164)

src/plugin/object/src/utils/ShapeGenerator.cpp (4)

• GenerateCubeMesh (8-132)
• GenerateCubeMesh (8-8)
• GenerateWheelMesh (359-384)
• GenerateWheelMesh (359-359)

src/plugin/object/src/utils/ShapeGenerator.hpp (2)

• GenerateCubeMesh (42-42)
• GenerateWheelMesh (92-92)

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

• vehicle (103-120)

src/plugin/physics/src/builder/VehicleBuilder.hpp (2)

• core (164-233)
• core (164-164)

src/plugin/object/src/utils/ShapeGenerator.hpp (2)

• GenerateCubeMesh (42-42)
• GenerateWheelMesh (92-92)

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

• vehicle (103-120)

src/plugin/physics/src/builder/VehicleBuilder.hpp (4)

• core (164-233)
• core (164-164)
• chassisMesh (54-65)
• chassisMesh (54-57)

src/plugin/object/src/utils/ShapeGenerator.cpp (4)

• GenerateCubeMesh (8-132)
• GenerateCubeMesh (8-8)
• GenerateWheelMesh (359-384)
• GenerateWheelMesh (359-359)

src/plugin/object/src/utils/ShapeGenerator.hpp (2)

• GenerateCubeMesh (42-42)
• GenerateWheelMesh (92-92)

src/plugin/physics/src/builder/VehicleBuilder.hpp (2)

• core (164-233)
• core (164-164)

src/plugin/physics/src/builder/VehicleBuilder.hpp (2)

• core (164-233)
• core (164-164)

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

• static_cast (65-65)

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

• settings (55-60)
• settings (65-70)

• GitHub Check: Agent

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

112-114: LGTM! Clear documentation improvement.

The added documentation clarifies the object layer values and appropriately directs users to the helper methods for correct defaults.

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

1-13: LGTM! Clean system initialization API.

The function signature and documentation are clear. The requirement to call during Scheduler::Startup is well-documented.

src/plugin/object/src/utils/ShapeGenerator.hpp (1)

81-92: LGTM! Well-documented wheel mesh generator.

The function signature follows the existing pattern of other shape generators, and the documentation clearly explains the X-axis orientation which is appropriate for vehicle wheels.

src/plugin/object/src/utils/ShapeGenerator.cpp (1)

359-384: LGTM! Correct rotation implementation.

The transformation (x, y, z) -> (y, -x, z) correctly rotates the cylinder 90° around the Z-axis to align the wheel along the X-axis. Both vertices and normals are consistently transformed, and the efficient reuse of texture coordinates and indices is appropriate.

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

14-21: Excellent vehicle configuration design.

The component structure is well-organized with clear separation of concerns:

• Drivetrain, engine, gearbox, and rollbar settings are logically grouped
• Default values are reasonable for vehicle simulation
• The use of an array indexed by WheelIndex ensures type safety

Also applies to: 26-41, 46-61, 66-72, 80-94

---

103-120: No action needed. The anti-roll bars are properly initialized by VehicleSystem::OnVehicleConstruct() using the vehicle.rollbar settings. The system creates JPH::VehicleAntiRollBar objects from vehicle.rollbar.frontStiffness and vehicle.rollbar.rearStiffness and adds them to the physics constraint, which is exactly what happens when CreateDefaultCar() sets vehicle.rollbar.rearStiffness = 8000.0f. The frontAntiRollBar and rearAntiRollBar shared pointers in the Vehicle struct are unused and do not affect system initialization or cause runtime errors.

Likely an incorrect or invalid review comment.

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

1-23: LGTM!

The component structure is well-designed with clear documentation and appropriate default initialization of input fields.

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

15-18: LGTM!

The vehicle-related header includes are properly organized and grouped by category (components, systems, builders).

Also applies to: 29-34

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

24-24: LGTM!

Registering InitVehicleSystem at startup is appropriate for initializing vehicle lifecycle hooks.

---

27-29: LGTM!

The system registration order is correct: vehicle control applies inputs after physics updates, transform sync propagates physics state to entities, and wheel transform sync updates wheel positions last.

src/plugin/physics/tests/VehicleGravityDropTest.cpp (3)

32-48: LGTM!

The test setup correctly creates a static floor and a vehicle at sufficient height (5m) with appropriate mass (1000kg) for gravity testing.

---

50-60: LGTM!

The simulation runs for an appropriate duration (200 steps = 3.2 seconds) to allow gravity to affect the vehicle.

---

62-66: LGTM!

The validation appropriately checks that the vehicle falls at least 2 meters, which is a reasonable expectation for a 3.2-second simulation under gravity.

src/plugin/physics/tests/VehicleCreationTest.cpp (3)

32-34: LGTM!

Using GenerateCubeMesh and GenerateWheelMesh to create simple test geometry is appropriate for unit testing vehicle creation.

---

36-44: LGTM!

The fluent builder API usage is clean and comprehensive, setting all required vehicle components (chassis, wheels, drivetrain) before building.

---

46-60: LGTM!

Comprehensive validation ensures the vehicle entity has all required components and that the internal vehicle state is properly initialized with non-null constraint and controller pointers.

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

1-16: LGTM!

Clean header declaration with appropriate documentation. The scheduling requirement (FixedTimeUpdate) is clearly documented.

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

23-68: LGTM!

Good integration test that validates the wheel transform synchronization pipeline. The test appropriately:

1. Sets up a complete vehicle with floor
2. Runs enough frames (50) to let the vehicle settle
3. Validates that all 4 wheel entities have valid transforms within reasonable bounds

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

1-16: LGTM!

Clean header with clear documentation. The execution order requirement (after PhysicsUpdate) is properly documented.

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

54-65: LGTM!

Good test coverage for the VehicleController input handling:

• Validates clamping behavior for out-of-range inputs
• Validates ResetInputs() correctly zeroes all inputs

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

14-44: LGTM!

Implementation correctly syncs wheel transforms from Jolt constraints:

• Proper validation checks for VehicleInternal, constraint, wheel entities, and transforms
• Uses appropriate Jolt APIs for retrieving wheel world transforms
• Correctly converts Jolt types to engine types via utility functions

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

14-40: LGTM!

Well-implemented control system:

• Proper physics activation check prevents processing when physics is inactive
• Input application to WheeledVehicleController is straightforward
• Body activation when inputs are applied ensures sleeping vehicles wake up correctly

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

24-105: LGTM!

Comprehensive integration test for vehicle forward movement:

• Proper setup with floor and vehicle configuration
• Custom wheel settings with high friction ensure consistent traction
• Appropriate settling period (50 frames) before applying input
• Sufficient simulation time (200 frames ≈ 3.2s) for meaningful movement
• Reasonable movement threshold (3 meters) with supplementary directional check

The redundant movement checks (horizontal distance and X/Z check) provide good diagnostics for test failures.

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

270-276: LGTM!

The system initialization correctly registers construct and destroy callbacks for the Vehicle component.

---

137-158: AWD differential configuration is correct; only the center differential requires mEngineTorqueRatio.

The center differential (index 2) correctly receives mEngineTorqueRatio = 1.0f while the front (index 0) and rear (index 1) axle differentials are passive. In Jolt's differential hierarchy, the engine torque sum across all differentials must equal 1.0 (currently: 0 + 0 + 1.0), and only the active torque input requires this setting. The center differential's mLeftRightSplit = 0.5f controls the front/rear torque distribution, which is the correct architecture for this AWD configuration.

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

21-57: LGTM!

The component structure is well-documented with clear ownership semantics. The use of JPH::Ref for the collision tester ensures proper lifetime management, and raw pointers for constraint/controller are appropriate given the documented manual management by VehicleSystem.

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

22-71: LGTM!

The WheelSettings struct provides sensible defaults with clear documentation. The factory methods CreateFrontWheel() and CreateRearWheel() offer convenient presets for common configurations.

src/plugin/physics/src/builder/VehicleBuilder.hpp (2)

36-39: LGTM on the static_assert approach.

Using static_assert in the primary template to restrict to 4-wheel vehicles is a clean compile-time check with a helpful error message.

---

54-65: LGTM!

The fluent builder API is well-designed with clear method names, appropriate defaults, and proper return of *this for chaining.

Also applies to: 73-78, 86-99, 104-126, 141-149, 238-251

Pull request overview

This PR adds vehicle support to the Physics plugin using the Jolt Physics engine's WheeledVehicle system. The implementation provides a builder pattern for creating 4-wheel vehicles with configurable engine, gearbox, drivetrain, and wheel properties.

Key changes:

• Introduces VehicleBuilder for creating vehicles with customizable chassis and wheel configurations
• Adds vehicle control system that applies user inputs (throttle, steering, braking) to the physics simulation
• Implements wheel transform synchronization to update visual wheel positions based on physics

Reviewed changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 16 comments.

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

Actionable comments posted: 1

src/plugin/physics/src/builder/VehicleBuilder.hpp (1)

199-200: Wheel scale is hardcoded to 1.0.

Wheels always get glm::vec3(1.0f) scale regardless of chassis scale or any wheel-specific settings. Consider adding a SetWheelScale method or deriving from chassis scale if wheel meshes should match chassis scaling.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between 4deb191 and d301657.

• src/plugin/physics/src/builder/VehicleBuilder.hpp
• src/plugin/physics/src/exception/VehicleBuilderError.hpp

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

• static_cast (65-65)

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

• settings (55-60)
• settings (65-70)

• GitHub Check: Build and verify binaries (macos-latest)
• 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/exception/VehicleBuilderError.hpp (1)

1-11: LGTM!

Clean and idiomatic custom exception class. Inheriting constructors from std::runtime_error is the correct pattern for simple exception types.

src/plugin/physics/src/builder/VehicleBuilder.hpp (5)

38-41: Good use of static_assert for compile-time restriction.

Clear error message guides future developers if other wheel counts become needed.

---

56-67: LGTM!

Fluent API with proper validation flag tracking.

---

184-215: Consider entity cleanup on build failure.

If any AddComponent call throws after entities are created (lines 185-201), the chassis and wheel entities remain orphaned in the registry. Document whether callers are responsible for cleanup, or implement rollback logic.

---

257-276: LGTM!

Sensible defaults for vehicle configuration. The default chassis mass (1500kg) and dimensions match typical passenger car specifications.

---

75-92: WheelIndex enum values are safely contiguous 0-3.

The WheelIndex enum (FrontLeft=0, FrontRight=1, RearLeft=2, RearRight=3) maps correctly to array indices. All related arrays (_wheelMeshes, _hasWheelMesh, _wheelPositions, and Vehicle.wheels) are sized to 4 elements, making the static_cast<size_t>(index) operations safe from out-of-bounds access. No changes needed.

Actionable comments posted: 3

In @src/plugin/physics/src/system/VehicleSystem.cpp:
- Around line 108-113: Validate the gearbox gearRatios before using them: in
VehicleSystem.cpp where you populate controllerSettings.mTransmission (the loop
that reads vehicle.gearbox.gearRatios and the assignment to mReverseGearRatios),
first check vehicle.gearbox.gearRatios.size() >= 2; if empty, avoid accessing
[0] and either leave transmission lists empty or set sensible defaults and
log/error; if size == 1, treat that single element as reverse only (or as
forward only based on spec) and avoid the forward loop; ensure you handle the
empty/single cases consistently and add a guard so the for loop and the
mReverseGearRatios assignment never index out of bounds.
- Around line 27-52: The CreateJoltWheelSettings function currently returns a
raw JPH::WheelSettingsWV*; change its signature to return
JPH::Ref<JPH::WheelSettingsWV> (and construct/return a Ref-wrapped instance) so
ownership matches Jolt patterns and avoids raw pointers; likewise, where
controller settings are allocated (the allocation around line 171), wrap that
allocation in JPH::Ref<...> instead of leaving a raw pointer so both wheel
settings and controller settings use JPH::Ref and are passed into
VehicleConstraint consistently with the existing Ref usage at lines ~207-209.

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

87-88: Consider removing unused wheelBodyIDs array.

The wheelBodyIDs array is declared but never populated (remains all invalid), and is then stored in vehicleInternal.wheelBodyIDs. Since Jolt's wheeled vehicle system uses raycasts rather than separate wheel bodies (as noted in line 62-63), this array serves no purpose and may confuse future maintainers.

♻️ Proposed cleanup

         std::array<Engine::Entity, 4> wheelEntities{};
-        std::array<JPH::BodyID, 4> wheelBodyIDs{};
 
         if (auto *storedWheelEntities = registry.ctx().find<std::array<Engine::Entity, 4>>())
         {
             wheelEntities = *storedWheelEntities;
         }

And remove the assignment on line 221:

         vehicleInternal.vehicleController = controller;
         vehicleInternal.collisionTester = collisionTester;
         vehicleInternal.wheelEntities = wheelEntities;
-        vehicleInternal.wheelBodyIDs = wheelBodyIDs;
         vehicleInternal.chassisBodyID = chassisInternal->bodyID;

Also applies to: 221-221

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between d301657 and efea0a2.

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

src/plugin/physics/src/builder/VehicleBuilder.hpp (2)

• core (168-237)
• core (168-168)

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

• vehicle (103-120)

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

• vehicleConstraint (46-46)

src/plugin/event/src/resource/EventManager.hpp (5)

• lock (114-114)
• lock (140-140)
• lock (154-154)
• lock (181-181)
• lock (202-202)

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

• static_cast (65-65)

src/plugin/physics/src/system/VehicleSystem.cpp (4)

192-205: LGTM: Proper body locking pattern.

The body locking is correctly scoped and includes proper error handling. The VehicleConstraint is created while holding the lock, which ensures thread-safe access to the chassis body.

---

253-258: LGTM: Proper resource cleanup.

The constraint is correctly removed from the physics system before deletion, and pointers are nulled afterward. The collisionTester uses JPH::Ref (smart pointer), so it cleans itself up automatically.

---

270-276: LGTM: Standard system initialization.

The vehicle system is properly registered with EnTT's component lifecycle signals for both construction and destruction.

---

171-171: This is correct code; no memory leak exists here.

mController is a Ref<VehicleControllerSettings> — Jolt's reference-counted smart pointer — not a raw pointer. When the allocated WheeledVehicleControllerSettings is assigned to mController, the Ref automatically takes ownership and manages the object's lifetime. This is the standard and safe pattern in Jolt Physics for managing settings objects.

Likely an incorrect or invalid review comment.

Actionable comments posted: 1

In @src/plugin/physics/src/builder/VehicleBuilder.hpp:
- Around line 217-229: The temporary use of registry.ctx().emplace/erase to hand
wheelEntities to the vehicle system is not thread-safe; instead store the wheel
entity array on the entity-specific component (e.g., add a wheelEntities member
to VehicleInternal or include it in Component::Vehicle) and set that member on
the chassis before calling chassis.AddComponent<Component::Vehicle>(core,
_vehicle) so OnVehicleConstruct can read it directly from the component
instance; remove the registry.ctx().emplace/erase usage and any code that
expects the context key.

src/plugin/physics/src/builder/VehicleBuilder.hpp (1)

199-201: Consider making wheel scale configurable.

Wheel scale is hardcoded to glm::vec3(1.0f) while chassis scale is configurable via SetChassisMesh. If wheel meshes need different scales (e.g., different sized wheels for front/rear), this would require modification.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between efea0a2 and 8eed12f.

• src/plugin/physics/src/builder/VehicleBuilder.hpp
• src/plugin/physics/src/component/Vehicle.hpp

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

• static_cast (65-65)

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

• settings (55-60)
• settings (65-70)

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

1-8: LGTM!

The includes are appropriate. The header uses #pragma once for include guard, includes necessary Jolt physics headers for anti-roll bar components, and the local WheelSettings.hpp for wheel configuration.

---

14-21: LGTM!

The enum class with uint8_t underlying type is a good choice for type safety and memory efficiency. Documentation clearly explains each option.

---

26-41: LGTM!

The gear ratio defaults and documentation are consistent. Starting in first gear (index 1) with reverse at index 0 is a sensible convention.

---

46-61: LGTM!

Engine settings have reasonable defaults for a vehicle simulation.

---

66-72: LGTM!

Simple and clean anti-roll bar configuration.

---

80-117: LGTM!

The Vehicle struct and CreateDefaultCar() factory are well-designed. The factory correctly initializes front wheels with steering capability and rear wheels without, using the appropriate helper methods from WheelSettings.

src/plugin/physics/src/builder/VehicleBuilder.hpp (7)

1-19: LGTM!

Includes are well-organized with appropriate headers for the builder's functionality.

---

38-41: LGTM!

The static_assert provides clear compile-time enforcement with an informative error message about the current limitation.

---

56-67: LGTM!

Fluent API implementation with sensible defaults for position, rotation, and scale.

---

75-92: LGTM!

The wheel configuration methods correctly use the WheelIndex enum for type-safe indexing. The fluent API pattern is consistent.

---

97-128: LGTM!

Configuration setters are concise and follow the fluent builder pattern consistently.

---

143-153: LGTM!

Good use of using enum for cleaner code when accessing enum values.

---

252-271: LGTM!

Private members have sensible defaults. The 1500kg mass and half-extents represent a reasonable mid-size car configuration.

Actionable comments posted: 3

In @src/plugin/physics/src/component/VehicleInternal.hpp:
- Around line 21-53: The field wheelBodyIDs in struct VehicleInternal appears
unused; either remove wheelBodyIDs from VehicleInternal to avoid dead state or
explicitly document it as reserved for future use and ensure it's intentionally
initialized/maintained (e.g., note in the VehicleInternal comment and in
VehicleSystem where vehicle bodies are handled) so reviewers understand it's not
required by Jolt wheeled vehicles; update references to VehicleInternal and any
VehicleSystem comments to reflect the chosen approach.

In @src/plugin/physics/src/system/WheelTransformSyncSystem.cpp:
- Around line 34-35: WheelTransformSyncSystem calls GetWheelWorldTransform with
inWheelRight = sAxisY and inWheelUp = sAxisX, which inverts the wheel axes
relative to VehicleSystem (which sets mWheelUp = Y and mWheelForward = Z);
update the GetWheelWorldTransform invocation in WheelTransformSyncSystem to pass
Right = X and Up = Y so the vectors match VehicleSystem’s orientation (use
JPH::Vec3::sAxisX() for inWheelRight and JPH::Vec3::sAxisY() for inWheelUp),
ensuring wheel transforms align with mWheelUp and mWheelForward.

src/plugin/physics/src/builder/VehicleBuilder.hpp (1)

75-80: Consider adding bounds check for robustness.

If WheelIndex is ever extended or a malformed value is passed, the static_cast<size_t>(index) could exceed array bounds. Consider adding a debug assertion.

🛡️ Optional defensive check

 VehicleBuilder &SetWheelMesh(Component::WheelIndex index, const Object::Component::Mesh &wheelMesh)
 {
+    assert(static_cast<size_t>(index) < 4 && "Invalid wheel index");
     _wheelMeshes[static_cast<size_t>(index)] = wheelMesh;
     _hasWheelMesh[static_cast<size_t>(index)] = true;
     return *this;
 }

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

24-42: Hardcoded wheel count and axis configuration.

The loop uses magic number 4 and hardcoded axis vectors (sAxisY, sAxisX). Consider extracting the wheel count as a constant for consistency with VehicleInternal::wheelEntities size, and document the axis choice.

♻️ Suggested improvement

+        constexpr size_t WHEEL_COUNT = 4;
-        for (size_t i = 0; i < 4; ++i)
+        for (size_t i = 0; i < WHEEL_COUNT; ++i)
         {

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

5-6: Remove unused Jolt includes.

VehicleAntiRollBar.h and VehicleConstraint.h are included but not referenced anywhere in this file. Remove these includes to reduce compilation dependencies.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between 8eed12f and 6ea8c4e.

• src/plugin/physics/src/builder/VehicleBuilder.hpp
• src/plugin/physics/src/component/Vehicle.hpp
• src/plugin/physics/src/component/VehicleInternal.hpp
• src/plugin/physics/src/system/VehicleControlSystem.cpp
• src/plugin/physics/src/system/VehicleSystem.cpp
• src/plugin/physics/src/system/WheelTransformSyncSystem.cpp
• src/plugin/physics/tests/VehicleCreationTest.cpp

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

src/plugin/physics/src/builder/VehicleBuilder.hpp (2)

• core (168-227)
• core (168-168)

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

• static_cast (65-65)

src/plugin/physics/src/builder/VehicleBuilder.hpp (2)

• core (168-227)
• core (168-168)

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

• vehicle (103-120)

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

• vehicleConstraint (42-42)

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

• static_cast (65-65)

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

• settings (55-60)
• settings (65-70)

src/plugin/physics/src/builder/VehicleBuilder.hpp (3)

38-41: LGTM!

Good use of static_assert in the primary template to enforce the 4-wheel constraint at compile time with a helpful error message explaining future extensibility.

---

168-227: LGTM!

The Build() method correctly validates required inputs, creates entities in the proper order (chassis RigidBody before Vehicle component), and uses exception handling for validation failures. The wheel positioning calculation using quaternion rotation is correct.

---

247-266: LGTM!

Sensible default values for a typical car configuration. The wheel position layout (front at +Z, rear at -Z) follows a standard automotive convention.

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

21-61: LGTM!

Comprehensive test that validates the full vehicle creation flow: component presence (Vehicle, VehicleInternal, VehicleController, RigidBody, Transform) and internal state validity (non-null constraint and controller). Good coverage for the builder's primary use case.

src/plugin/physics/src/system/VehicleSystem.cpp (4)

132-153: AWD differential configuration looks correct.

The center differential using negative wheel indices (-1, -2) to reference front and rear differentials follows Jolt's convention. The mEngineTorqueRatio = 1.0f on the center differential ensures proper torque distribution.

---

231-259: LGTM!

Proper cleanup order: remove step listener before constraint, then let JPH::Ref handle deallocation. Exception handling prevents crashes during destruction.

---

261-267: LGTM!

Clean initialization that registers lifecycle hooks for Vehicle component construction and destruction.

---

27-52: No action needed. The use of raw new for JPH::WheelSettingsWV is the correct pattern in Jolt Physics. Reference-counted objects like wheel settings are designed to be created with new and passed to Jolt containers (such as constraintSettings.mWheels), which take ownership via internal reference counting. No manual AddRef/Release is required by the caller.

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

27-42: LGTM!

Well-documented gearbox configuration with sensible defaults. The gear ratio layout (index 0 = reverse) is consistent with usage in VehicleSystem.cpp.

---

81-121: LGTM!

Clean component structure with appropriate defaults. The CreateDefaultCar() factory provides a convenient starting point that properly differentiates front wheels (with steering) from rear wheels (no steering).

Actionable comments posted: 2

In @src/plugin/physics/src/builder/VehicleBuilder.hpp:
- Around line 130-142: The documentation in VehicleBuilder.hpp describing
default wheel offsets is out of sync with the implementation; update the comment
to match the values returned by GetDefaultWheelPositions() (use ±0.9 for X and
±1.2 for Z, and keep the Y offset as -0.3) so the brief and param descriptions
accurately reflect the actual defaults produced by GetDefaultWheelPositions().

In @src/plugin/physics/src/system/VehicleSystem.cpp:
- Around line 135-156: AWD differential setup is incorrect: mEngineTorqueRatio
is only set on controllerSettings.mDifferentials[2] and differential[2] uses
invalid wheel index -2, so engine torque is routed to non-existent wheels. Fix
by assigning sensible mEngineTorqueRatio values on
controllerSettings.mDifferentials[0] and [1] (e.g., split total 1.0 across the
three diffs or 0.5/0.5 for front/rear and 0.0 for center if intended), remove or
correct invalid wheel indices on controllerSettings.mDifferentials[2] (replace
-2 with -1 if intent is “no wheel” or set actual wheel indices), and ensure the
sum of all controllerSettings.mDifferentials[*].mEngineTorqueRatio equals 1.0 to
properly distribute engine torque to wheels 0,1,2,3.

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

83-84: wheelBodyIDs is declared but never populated.

The wheelBodyIDs array is initialized with default JPH::BodyID values and stored in VehicleInternal (line 220), but actual body IDs are never assigned. Since Jolt's WheeledVehicle uses raycasts rather than separate wheel bodies (as noted in the comment on lines 58-59), this array appears to be dead code.

Consider removing wheelBodyIDs from VehicleInternal if wheel bodies aren't used, or document why it's kept for future use.

---

167-171: Raw new for wheel settings is intentional but deserves a clearer comment.

The comment on line 167 mentions Jolt references make new safe, but could be more explicit that JPH::VehicleConstraintSettings::mWheels takes ownership of the pointer. Consider clarifying:

-            // Safety: mWheels is a vector of Jolt references, so new is safe here
+            // Jolt's mWheels takes ownership via Ref<WheelSettings>, so raw new is required

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

5-6: Unnecessary Jolt includes.

VehicleAntiRollBar.h and VehicleConstraint.h are included but no Jolt types are used in this header. Removing them would reduce compile-time dependencies and coupling.

♻️ Suggested fix

 #include "component/WheelSettings.hpp"
 #include "entity/Entity.hpp"
-#include <Jolt/Physics/Vehicle/VehicleAntiRollBar.h>
-#include <Jolt/Physics/Vehicle/VehicleConstraint.h>
 #include <array>
 #include <memory>

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

18-20: Minor redundancy in validity check.

Based on the snippet from VehicleInternal.hpp, IsValid() already checks vehicleConstraint != nullptr. The explicit !internal.vehicleConstraint check is redundant.

♻️ Suggested simplification

-        if (!internal.IsValid() || !internal.vehicleConstraint)
+        if (!internal.IsValid())
             return;

src/plugin/physics/src/builder/VehicleBuilder.hpp (1)

168-222: Entity leak on exception during Build().

If an exception occurs after entity creation (e.g., during AddComponent calls), the already-created entities are not cleaned up. Consider using RAII wrappers or a try-catch with rollback.

This is a low-priority concern since exceptions during component addition are unlikely, but worth noting for robustness.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between 6ea8c4e and 4fcc2fb.

• src/plugin/object/src/utils/ShapeGenerator.cpp
• src/plugin/physics/src/builder/VehicleBuilder.hpp
• src/plugin/physics/src/component/Vehicle.hpp
• src/plugin/physics/src/component/VehicleController.hpp
• src/plugin/physics/src/component/WheelSettings.hpp
• src/plugin/physics/src/system/VehicleSystem.cpp
• src/plugin/physics/src/system/WheelTransformSyncSystem.cpp

• src/plugin/physics/src/component/VehicleController.hpp
• src/plugin/physics/src/component/WheelSettings.hpp

src/plugin/object/src/utils/ShapeGenerator.hpp (1)

• GenerateCylinderMesh (78-79)

src/plugin/physics/src/builder/VehicleBuilder.hpp (2)

• core (168-222)
• core (168-168)

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

• vehicleConstraint (42-42)

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

• static_cast (65-65)

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

• vec3 (113-121)
• WheelIndex (126-144)

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

• static_cast (65-65)

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

• settings (57-62)
• settings (67-72)

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

src/plugin/object/src/utils/ShapeGenerator.cpp (1)

359-384: LGTM! Clean implementation with correct transformation math.

The wheel mesh generator correctly:

• Reuses GenerateCylinderMesh to avoid code duplication
• Applies the proper -90° rotation around Z axis to align the wheel along X
• Uses std::move for texCoords and indices to avoid unnecessary copies
• Includes clear comments explaining the transformation

The approach prioritizes code maintainability by reusing existing cylinder generation logic rather than duplicating it inline.

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

61-229: LGTM overall - well-structured lifecycle management.

The OnVehicleConstruct function properly validates preconditions (physics activation, valid chassis body, gearbox configuration), uses RAII-style body locking, and has comprehensive exception handling. The separation of controller settings, constraint settings, and component population is clean.

---

238-266: LGTM - proper cleanup sequence.

The destroy handler correctly removes the step listener before removing the constraint, nullifies the reference to trigger RAII cleanup, and removes the internal component. The early returns for invalid states are appropriate.

---

268-274: LGTM - clean system initialization.

The lifecycle hooks are properly connected to EnTT's signals.

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

27-42: LGTM - sensible gearbox defaults.

The gear ratio convention (index 0 = reverse) is clearly documented in the comment and enforced by validation in both VehicleBuilder::Build and OnVehicleConstruct.

---

81-145: LGTM - clean Vehicle component design.

The struct has reasonable defaults via CreateDefaultCar(), proper documentation, and the wheel positions helper provides consistent defaults. The use of static_cast<size_t>(WheelIndex::Count) for array sizing ensures compile-time safety.

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

34-46: LGTM - correct wheel transform extraction and rotation alignment.

The axis configuration (wheelUp=Y, wheelForward=Z) matches what's set in CreateJoltWheelSettings, and the 90° Z rotation is consistent with the initial rotation applied in VehicleBuilder::Build (line 200). This ensures mesh orientation stays consistent.

---

14-49: LGTM overall - clean sync implementation.

The function properly handles invalid entities and missing transforms with early returns, avoiding null pointer dereferences.

src/plugin/physics/src/builder/VehicleBuilder.hpp (3)

38-41: LGTM - good use of static_assert for compile-time constraint.

The primary template with static_assert provides a clear error message when attempting to use unsupported wheel counts, guiding users to the supported configuration.

---

183-186: LGTM - gearbox validation consistent with VehicleSystem.

The check for at least 2 gear ratios mirrors the validation in OnVehicleConstruct, ensuring the error is caught early at build time rather than during physics initialization.

---

242-257: LGTM - sensible defaults.

The builder initializes with CreateDefaultCar() and GetDefaultWheelPositions(), providing reasonable defaults while allowing full customization through the fluent API.

Actionable comments posted: 5

In @src/plugin/object/src/utils/ShapeGenerator.cpp:
- Around line 452-454: The function GenerateCapsuleMesh contains an early
"return mesh;" placed inside its outer loop (it returns after processing the
first ring pair), so remove that in-loop return and place a single "return
mesh;" after all loops complete; also add the missing closing brace to properly
end GenerateCapsuleMesh so GenerateWheelMesh is not treated as nested—locate the
in-loop "return mesh;" inside GenerateCapsuleMesh and move it to the function
end and ensure the function has its correct closing brace before the next
function.

In @src/plugin/physics/src/builder/VehicleBuilder.hpp:
- Around line 130-153: Update the SetWheelPositions doc comment to reflect the
real defaults used by Component::Vehicle::GetDefaultWheelPositions(): front
wheels at ±0.9 X, +1.2 Z; rear wheels at ±0.9 X, -1.2 Z (keeping the Y offset at
-0.3), and ensure the param descriptions still match the function signature
(VehicleBuilder::SetWheelPositions and the _wheelPositions indices
FrontLeft/FrontRight/RearLeft/RearRight).
- Around line 168-186: The Build method currently checks meshes and gearbox but
lacks validation of chassis physical properties; add checks in
VehicleBuilder::Build to ensure _chassisMass > 0 (throw
Exception::VehicleBuilderError with a clear message if zero/negative) and
validate each component of _chassisHalfExtents is > 0 (throw
Exception::VehicleBuilderError naming which half-extent is invalid if any
component is zero/negative) before proceeding to construct and return the
Engine::Entity.

In @src/plugin/physics/src/Physics.hpp:
- Around line 14-15: The header include for "component/RigidBody.hpp" is
duplicated; remove the redundant #include "component/RigidBody.hpp" (the second
occurrence) so the file only includes it once alongside
"component/RigidBodyInternal.hpp"; update the includes at the top of the file
(the duplicated include directive) and run a quick rebuild to ensure no other
references rely on the duplicate.

In @src/plugin/physics/src/system/VehicleSystem.cpp:
- Around line 83-84: Remove the unused wheelBodyIDs dead code or document its
purpose: either delete the std::array<JPH::BodyID, 4> wheelBodyIDs declaration
and its assignment into VehicleInternal, or if you want to keep it for future
use, add a clear comment where VehicleInternal::wheelBodyIDs (and the local
wheelBodyIDs) are declared/assigned explaining that Jolt's WheeledVehicle uses
raycasts (no separate wheel bodies) and this field is intentionally
unused/reserved; update references to wheelEntities and VehicleInternal
accordingly to avoid leaving an unpopulated array.

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

24-28: Replace magic number with WheelIndex::Count for consistency.

Using the enum constant improves maintainability and keeps the wheel count synchronized with WheelSettings.hpp.

♻️ Suggested fix

-        for (size_t i = 0; i < 4; ++i)
+        for (size_t i = 0; i < static_cast<size_t>(Component::WheelIndex::Count); ++i)

This requires including "component/WheelSettings.hpp".

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

29-30: Consider adding a null check or assertion before the static_cast.

If GetController() ever returns a different controller type or null, the static_cast would result in undefined behavior. While OnVehicleConstruct in VehicleSystem.cpp always creates a WheeledVehicleController, a defensive check improves robustness.

♻️ Optional defensive check

+            JPH::VehicleController *baseController = internal.vehicleConstraint->GetController();
+            if (!baseController)
+                return;
             auto *wheeledController =
-                static_cast<JPH::WheeledVehicleController *>(internal.vehicleConstraint->GetController());
+                static_cast<JPH::WheeledVehicleController *>(baseController);

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

37-39: Consider making handbrake torque and preload length configurable.

mSuspensionPreloadLength and mMaxHandBrakeTorque are hardcoded. Adding these to WheelSettings would allow fine-tuning without code changes.

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

5-8: Remove unused includes from Vehicle.hpp.

The includes <Jolt/Physics/Vehicle/VehicleAntiRollBar.h>, <Jolt/Physics/Vehicle/VehicleConstraint.h>, and <memory> are not used in this header. The Vehicle struct only defines configuration data structures that don't depend on these types. Removing them reduces compile-time overhead without affecting downstream files, which either include these headers directly or through Physics.pch.hpp.

src/plugin/physics/src/builder/VehicleBuilder.hpp (2)

183-186: Consider clarifying the gearbox validation message.

The validation checks for at least 2 gear ratios but the error message specifically mentions "one forward gear and one reverse gear". If the gearbox structure allows multiple forward gears (e.g., 3 forward + 1 reverse = 4 total), this message could be slightly misleading about the minimum requirement.

💬 Suggested message refinement

         if (_vehicle.gearbox.gearRatios.size() < 2)
         {
-            throw Exception::VehicleBuilderError("Gearbox must have at least one forward gear and one reverse gear");
+            throw Exception::VehicleBuilderError("Gearbox must have at least 2 gears (minimum: 1 forward and 1 reverse)");
         }

---

227-240: Optional: Consider early validation in setters.

While validation in the Build method (as suggested in the previous comment) is sufficient, adding validation directly in these setters would provide earlier error detection and clearer error context.

🔍 Optional early validation

     VehicleBuilder &SetChassisMass(float mass)
     {
+        if (mass <= 0.0f)
+        {
+            throw Exception::VehicleBuilderError("Chassis mass must be positive");
+        }
         _chassisMass = mass;
         return *this;
     }
     
     VehicleBuilder &SetChassisHalfExtents(const glm::vec3 &halfExtents)
     {
+        if (halfExtents.x <= 0.0f || halfExtents.y <= 0.0f || halfExtents.z <= 0.0f)
+        {
+            throw Exception::VehicleBuilderError("Chassis half-extents must have all positive components");
+        }
         _chassisHalfExtents = halfExtents;
         return *this;
     }

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between 4fcc2fb and b8c0aa7.

• src/plugin/object/src/utils/ShapeGenerator.cpp
• src/plugin/object/src/utils/ShapeGenerator.hpp
• src/plugin/physics/src/Physics.hpp
• src/plugin/physics/src/builder/VehicleBuilder.hpp
• src/plugin/physics/src/component/RigidBody.hpp
• src/plugin/physics/src/component/Vehicle.hpp
• src/plugin/physics/src/component/VehicleController.hpp
• src/plugin/physics/src/component/VehicleInternal.hpp
• src/plugin/physics/src/component/WheelSettings.hpp
• src/plugin/physics/src/exception/VehicleBuilderError.hpp
• src/plugin/physics/src/plugin/PhysicsPlugin.cpp
• src/plugin/physics/src/system/VehicleControlSystem.cpp
• src/plugin/physics/src/system/VehicleControlSystem.hpp
• src/plugin/physics/src/system/VehicleSystem.cpp
• src/plugin/physics/src/system/VehicleSystem.hpp
• src/plugin/physics/src/system/WheelTransformSyncSystem.cpp
• src/plugin/physics/src/system/WheelTransformSyncSystem.hpp
• src/plugin/physics/tests/VehicleCreationTest.cpp
• src/plugin/physics/tests/VehicleForwardMovementTest.cpp
• src/plugin/physics/tests/VehicleGravityDropTest.cpp
• src/plugin/physics/tests/VehicleSteeringTest.cpp
• src/plugin/physics/tests/VehicleWheelTransformSyncTest.cpp

• src/plugin/physics/src/system/VehicleControlSystem.hpp
• src/plugin/object/src/utils/ShapeGenerator.hpp
• src/plugin/physics/src/plugin/PhysicsPlugin.cpp
• src/plugin/physics/tests/VehicleForwardMovementTest.cpp
• src/plugin/physics/src/component/VehicleInternal.hpp
• src/plugin/physics/tests/VehicleWheelTransformSyncTest.cpp
• src/plugin/physics/src/system/VehicleSystem.hpp
• src/plugin/physics/src/component/RigidBody.hpp

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

• vehicleConstraint (42-42)

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

• static_cast (65-65)

src/plugin/object/src/utils/ShapeGenerator.hpp (1)

• GenerateCylinderMesh (78-79)

src/plugin/physics/src/builder/VehicleBuilder.hpp (4)

• core (168-222)
• core (168-168)
• chassisMesh (56-67)
• chassisMesh (56-59)

src/plugin/object/src/utils/ShapeGenerator.cpp (4)

• GenerateCubeMesh (9-133)
• GenerateCubeMesh (9-9)
• GenerateWheelMesh (456-481)
• GenerateWheelMesh (456-456)

src/plugin/object/src/utils/ShapeGenerator.hpp (2)

• GenerateCubeMesh (42-42)
• GenerateWheelMesh (103-103)

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

• vec3 (113-121)

src/plugin/physics/src/builder/VehicleBuilder.hpp (4)

• core (168-222)
• core (168-168)
• chassisMesh (56-67)
• chassisMesh (56-59)

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

• vec3 (113-121)

src/plugin/object/src/utils/ShapeGenerator.cpp (4)

• GenerateCubeMesh (9-133)
• GenerateCubeMesh (9-9)
• GenerateWheelMesh (456-481)
• GenerateWheelMesh (456-456)

src/plugin/object/src/utils/ShapeGenerator.hpp (2)

• GenerateCubeMesh (42-42)
• GenerateWheelMesh (103-103)

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

• vec3 (113-121)
• WheelIndex (126-144)

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

• static_cast (65-65)

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

• settings (57-62)
• settings (67-72)

[error] 457-457: function definition is not allowed here.

[error] 483-483: expected '}'

src/plugin/object/src/utils/ShapeGenerator.cpp (1)

456-481: Wheel mesh generation logic is sound.

The implementation correctly:

• Reuses cylinder mesh generation to avoid duplication
• Applies a -90° Z-axis rotation to transform the Y-oriented cylinder into an X-aligned wheel
• Moves texCoords and indices efficiently with std::move

Once the syntax error in lines 452-454 is resolved, this function will work correctly.

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

1-63: LGTM! Well-designed input controller component.

The component provides clean input handling with appropriate clamping for all vehicle controls. The documentation is clear, and the API is intuitive.

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

17-20: Vehicle infrastructure properly integrated.

The new vehicle-related components, systems, and builders are correctly included and organized.

Also applies to: 34-39

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

21-61: LGTM! Comprehensive vehicle creation test.

The test properly validates the VehicleBuilder workflow and verifies that all required components are correctly attached to the vehicle entity. The assertions on VehicleInternal ensure the underlying physics constraint is properly initialized.

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

22-67: LGTM! Solid gravity behavior test.

The test validates that vehicles respond correctly to gravity by simulating 200 physics steps and verifying a realistic drop distance. The floor collider setup and assertions are appropriate.

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

1-16: LGTM! Clear system declaration.

The header is well-documented with clear guidance on when the system should run and what it does. The API is straightforward and consistent with other system declarations.

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

1-11: LGTM!

Clean and minimal exception type that properly inherits from std::runtime_error and reuses its constructors. This follows standard C++ exception design patterns.

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

14-49: Good defensive programming with validation checks.

The function properly validates VehicleInternal, wheel entities, and transforms before accessing them. The early returns and continue statements prevent null pointer dereferences.

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

23-66: LGTM!

The test correctly validates input clamping behavior and reset functionality for the VehicleController. Good use of ASSERT_NE to fail early if the controller is missing.

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

14-41: LGTM!

The system correctly applies driver inputs to the Jolt physics controller and activates the chassis body when inputs are non-zero to ensure the physics simulation responds.

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

163-171: Wheel settings allocation follows Jolt's ownership pattern.

The new JPH::WheelSettingsWV() pattern is correct here. Jolt's constraintSettings.mWheels stores Ref<WheelSettings> which will take ownership and handle cleanup.

---

238-266: LGTM!

Proper cleanup sequence: step listener and constraint are removed before releasing the reference. The try-catch ensures exceptions during destruction don't crash the system.

---

268-274: LGTM!

Clean registration of lifecycle handlers for the Vehicle component.

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

10-16: Good use of Count in the enum for compile-time array sizing.

This allows static_cast<size_t>(WheelIndex::Count) to be used consistently throughout the codebase for wheel arrays.

---

24-73: LGTM!

Well-documented struct with sensible defaults. The static factory methods (CreateFrontWheel, CreateRearWheel) provide a clean API for common wheel configurations.

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

81-145: LGTM!

The Vehicle struct provides a clean, well-documented API with sensible defaults. CreateDefaultCar() and GetDefaultWheelPositions() offer convenient factory methods for common configurations.

src/plugin/physics/src/builder/VehicleBuilder.hpp (3)

38-41: LGTM! Clean compile-time constraint.

The primary template with static_assert effectively restricts instantiation to N=4 while providing a clear error message for unsupported configurations.

---

56-128: Setters look good overall.

The fluent API pattern is consistently implemented. Validation is deferred to the Build method, which is acceptable for this builder pattern.

---

242-257: Well-chosen defaults.

The private member defaults are sensible:

• 1500 kg mass is appropriate for a typical vehicle
• Half-extents of (1.0, 0.5, 2.0) represent reasonable car dimensions
• Wheel positions and vehicle configuration use factory methods for consistency

Actionable comments posted: 0

src/plugin/object/src/utils/ShapeGenerator.cpp (1)

457-482: LGTM! Clean implementation with correct transformation math.

The wheel mesh generation correctly reorients a cylinder from Y-axis to X-axis alignment using the transformation (x, y, z) → (y, -x, z), which represents a -90° rotation around the Z axis. The efficient reuse of texCoords and indices via move semantics is well done.

Optional: Consider adding input validation for consistency

Some functions in this file validate inputs (e.g., GenerateCapsuleMesh checks radius <= 0.0f at line 364), while others don't. For consistency and robustness, you could optionally add:

 Component::Mesh GenerateWheelMesh(float radius, float width, uint32_t segments)
 {
+    if (radius <= 0.0f || width <= 0.0f)
+    {
+        return Component::Mesh{};
+    }
+    segments = std::max(3u, segments);
+
     // Generate a cylinder mesh oriented along the Y axis, then rotate vertices

Note: This is purely optional since GenerateCylinderMesh already validates segments, and invalid radius/width values won't cause crashes.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between b8c0aa7 and a95decd.

• src/plugin/object/src/utils/ShapeGenerator.cpp

src/plugin/object/src/utils/ShapeGenerator.hpp (1)

• GenerateCylinderMesh (78-79)

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

Actionable comments posted: 2

In @src/plugin/physics/src/system/VehicleSystem.cpp:
- Around line 160-167: The code assumes vehicle.wheels has 4 elements but never
checks its size, risking out-of-bounds access when filling
constraintSettings.mWheels; before the for-loop validate vehicle.wheels.size()
(and wheelPositions.size()) and either early-return/log an error or compute
count = std::min({size_t(4), vehicle.wheels.size(), wheelPositions.size()}) and
iterate up to count, resizing constraintSettings.mWheels to count and calling
CreateJoltWheelSettings only for valid indices; include a clear error/log when
wheels are missing so misconfigured Vehicle components are detected.
- Around line 46-47: The friction curves only set a single point which yields
incorrect interpolation; add a second point for both
joltWheel.mLongitudinalFriction and joltWheel.mLateralFriction by writing
mPoints[1] = {1.0f, wheelSettings.longitudinalFriction} and mPoints[1] = {1.0f,
wheelSettings.lateralFriction} respectively, and ensure the friction curve point
count is updated (e.g., set the curve's point count/numPoints to 2) so Jolt will
interpolate/extrapolate correctly across slip ratios.

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

113-153: Consider making differential parameters configurable.

The differential ratios (3.42f), limited slip ratios (1.4f), and torque splits (0.5f) are hard-coded for all drivetrain types. While these are reasonable defaults, different vehicle types (sports car, truck, off-road) typically require different differential characteristics for realistic handling.

Consider adding differential configuration options to the Vehicle component or creating presets for different vehicle types.

---

82-82: Clarify wheelBodyIDs usage or remove if unused.

The wheelBodyIDs array is initialized but never populated with actual body IDs, then stored in VehicleInternal. The comment at lines 58-60 explains that Jolt's wheeled vehicles don't use separate physics bodies for wheels, making this array intentionally unused.

Consider either:

• Removing wheelBodyIDs entirely if it serves no purpose
• Adding a comment explaining why it exists but remains empty
• Using a different sentinel pattern to make the intent clearer

This would prevent confusion if future developers try to use these IDs.

Also applies to: 215-215

---

39-39: Consider making handbrake torque configurable.

The handbrake torque (1500.0f) is hard-coded. Different vehicle types (lightweight sports cars vs. heavy trucks) typically require different handbrake characteristics.

Consider adding this as a configurable parameter in the Vehicle or WheelSettings component for more flexibility.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between a95decd and bc70972.

• src/plugin/physics/src/system/VehicleSystem.cpp
• src/plugin/physics/src/system/WheelTransformSyncSystem.cpp

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

src/plugin/physics/src/builder/VehicleBuilder.hpp (2)

• core (168-222)
• core (168-168)

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

• vehicleConstraint (42-42)

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

• static_cast (65-65)

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

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

228-248: LGTM! Proper cleanup sequence.

The destruction logic correctly:

• Validates physics activation and component validity before cleanup
• Removes step listener before constraint (proper order)
• Nullifies references and removes components
• Uses early returns to handle edge cases safely

---

250-256: LGTM! Standard lifecycle registration.

The initialization correctly registers construct and destroy handlers using entt's signal system. Implementation follows standard patterns for component lifecycle management.

---

31-34: The axis vectors in lines 31-34 are specified in local vehicle space and work correctly for any chassis rotation. Jolt's VehicleConstraint automatically applies the chassis body's rotation to all local-space coordinates (positions and axes) when the constraint is attached. No transformation is needed; the hard-coded vectors represent standard vehicle orientation in the local frame.

Actionable comments posted: 0

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

37-38: Consider moving the constant mesh rotation outside the loop.

The 90-degree Z-axis rotation quaternion is constant and is currently recreated on every iteration. Moving it outside the loop would improve performance.

♻️ Proposed optimization

+    static const JPH::Quat meshRotation = JPH::Quat::sRotation(JPH::Vec3::sAxisZ(), JPH::DegreesToRadians(90.0f));
+
     for (size_t i = 0; i < 4; ++i)
     {
         Engine::Entity wheelEntity = internal.wheelEntities[i];
         if (core.IsEntityValid(wheelEntity) == false)
             continue;

         auto *wheelTransform = registry.try_get<Object::Component::Transform>(wheelEntity);
         if (!wheelTransform)
             continue;

         JPH::RMat44 wheelWorldTransform =
             constraint->GetWheelWorldTransform(static_cast<uint32_t>(i), JPH::Vec3::sAxisY(), JPH::Vec3::sAxisZ());

-        JPH::Quat meshRotation = JPH::Quat::sRotation(JPH::Vec3::sAxisZ(), JPH::DegreesToRadians(90.0f));
         JPH::Quat finalRot = wheelWorldTransform.GetQuaternion() * meshRotation;

---

27-27: Prefer idiomatic boolean check.

Using !core.IsEntityValid(wheelEntity) is more idiomatic than comparing with == false.

♻️ Proposed change

-        if (core.IsEntityValid(wheelEntity) == false)
+        if (!core.IsEntityValid(wheelEntity))
             continue;

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between bc70972 and 6b80d61.

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

src/plugin/physics/src/builder/VehicleBuilder.hpp (2)

• core (168-222)
• core (168-168)

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

• static_cast (65-65)

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

14-44: LGTM! Solid implementation with proper validation.

The system correctly:

• Validates the vehicle constraint before processing
• Checks each wheel entity and its Transform component
• Retrieves physics-driven wheel transforms from Jolt
• Applies the consistent 90-degree mesh rotation (matching VehicleBuilder initialization)
• Updates wheel positions and rotations

The quaternion multiplication order correctly applies the mesh rotation in the wheel's local space.