refactor: enhanced vehicle handling - #443
Conversation
Sorry, something went wrong.
|
Warning Rate limit exceeded@ripel2 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 14 minutes and 7 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR. We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughAdds transmission modes, torque and friction curve types, per-wheel brake/hand-brake torque fields, a controller settings wrapper, and swaps chassis MeshCollider for ConvexHullMeshCollider; maps new settings into the vehicle controller and updates builder/controller wiring and tests accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Game
participant VB as VehicleBuilder
participant ECS as Entity/Chassis
participant CTRL as Utils::WheeledVehicleControllerSettings
participant JPH as JPH VehicleController
participant PW as Physics World
Game->>VB: Build(entity, mesh, VehicleSettings)
VB->>ECS: Add ConvexHullMeshCollider
VB->>ECS: Attach RigidBody
VB->>CTRL: Populate settings (torque curve, gearbox, friction curves, brake torques)
CTRL->>JPH: ConstructController(constraint)
JPH->>PW: Register vehicle/controller
PW-->>Game: Vehicle ready
sequenceDiagram
autonumber
actor Input
participant VCS as VehicleControlSystem
participant CTRL as JPH VehicleController
participant PW as Physics World
participant RPM as VehicleRPMUpdate
Input->>VCS: throttle/brake/shift
VCS->>CTRL: Apply inputs (mode, clutch, latency)
CTRL->>PW: Integrate step (using torque curve, friction curves, gear ratios)
PW-->>RPM: Wheel/engine RPMs
RPM-->>CTRL: Sync RPM metrics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labelsenhancement Suggested reviewers
Poem🚥 Pre-merge checks | ✅ 2 | ❌ 1 ❌ Failed checks (1 warning)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agentsIn `@src/plugin/physics/src/builder/VehicleBuilder.hpp`:
- Around line 185-188: The validation in VehicleBuilder.hpp currently checks
_vehicle.gearbox.forwardGearRatios.size() < 2 but throws
Exception::VehicleBuilderError("Gearbox must have at least one forward gear"),
which is inconsistent with VehicleSystem.cpp that uses
forwardGearRatios.empty(); update the check and/or message so they agree: either
change the condition to _vehicle.gearbox.forwardGearios.empty() (or .size() < 1)
to require one forward gear and keep the message, or keep the size() < 2 logic
and change the Exception::VehicleBuilderError message to state "at least two
forward gears"; ensure forwardGearRatios and Exception::VehicleBuilderError are
the referenced symbols you modify so behavior matches VehicleSystem.cpp's
validation.
src/plugin/physics/src/component/WheelSettings.hpp (1)src/plugin/physics/src/system/VehicleSystem.cpp (1)70-88: Consider avoiding per-instance heap allocations for default curves.
std::vector defaults allocate for every WheelSettings instance. If this struct is created frequently, prefer std::array (fixed 3-point curves) or static defaults copied on demand to reduce allocations.40-40: Dead code: mMaxHandBrakeTorque is assigned twice.
Line 40 sets mMaxHandBrakeTorque based on isRear, but line 50 unconditionally overwrites it with wheelSettings.maxHandBrakeTorque. The assignment on line 40 has no effect.
Proposed fix- joltWheel.mMaxHandBrakeTorque = isRear ? 1500.0f : 0.0f; joltWheel.mRadius = wheelSettings.radius; joltWheel.mWidth = wheelSettings.width;Then ensure WheelSettings::CreateFrontWheel() and CreateRearWheel() set appropriate maxHandBrakeTorque defaults (0 for front, non-zero for rear).
Also applies to: 47-50
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)src/plugin/physics/src/system/VehicleSystem.cpp (1)40-50: Duplicate assignment to mMaxHandBrakeTorque - first value is overwritten.
Line 40 sets mMaxHandBrakeTorque based on whether the wheel is rear (1500.0f) or front (0.0f), but line 50 unconditionally overwrites this with wheelSettings.maxHandBrakeTorque. The isRear logic becomes dead code.
Clarify the intent:
Option A: Remove the dead code at line 40
- If wheelSettings.maxHandBrakeTorque already encodes front/rear differences, remove line 40.
- If the isRear logic should take precedence, make line 50 conditional or remove it.
Option B: Use isRear as fallback when wheelSettings value is zerojoltWheel.mMaxSteerAngle = wheelSettings.maxSteerAngle; - joltWheel.mMaxHandBrakeTorque = isRear ? 1500.0f : 0.0f; joltWheel.mRadius = wheelSettings.radius;- joltWheel.mMaxHandBrakeTorque = isRear ? 1500.0f : 0.0f; joltWheel.mRadius = wheelSettings.radius; ... - joltWheel.mMaxHandBrakeTorque = wheelSettings.maxHandBrakeTorque; + joltWheel.mMaxHandBrakeTorque = (wheelSettings.maxHandBrakeTorque > 0.0f) + ? wheelSettings.maxHandBrakeTorque + : (isRear ? 1500.0f : 0.0f);
In `@src/plugin/physics/src/system/VehicleSystem.cpp`: - Around line 141-145: Add validation for reverseGearRatios similar to the forward-gear check: in the code that populates controllerSettings.mTransmission.mReverseGearRatios from vehicle.gearbox.reverseGearRatios (the loop referencing controllerSettings.mTransmission.mReverseGearRatios and vehicle.gearbox.reverseGearRatios), check if reverseGearRatios is empty and either emit a warning via the existing logging mechanism or provide a sensible default (e.g., copy a single negative of first forward ratio or push_back a fallback ratio) before populating the vector, so the vehicle can always reverse or the absence is clearly logged.
src/plugin/physics/src/utils/WheeledVehicleControllerSettings.hpp (3)58-58: Explicit copy constructor may prevent expected copy semantics.
Marking the copy constructor as explicit is unusual and will prevent implicit copy construction (e.g., WheeledVehicleControllerSettings s2 = s1; won't compile). If this is intentional to force explicit copies, it's fine, but typically copy constructors are non-explicit to allow natural copy semantics.
Consider removing explicit if standard copy semantics are desired- explicit WheeledVehicleControllerSettings(const WheeledVehicleControllerSettings &other) = default; + WheeledVehicleControllerSettings(const WheeledVehicleControllerSettings &other) = default;
47-87: Consider adding move operations for efficiency.
The class declares a copy constructor and copy assignment operator but no move constructor or move assignment operator. Due to the user-declared copy constructor, move operations will be implicitly deleted, potentially causing unnecessary copies when moving would be more efficient.
Add move constructor and move assignment operatorWheeledVehicleControllerSettings &operator=(const WheeledVehicleControllerSettings &other) = default; + /** + * `@brief` Move constructor + * `@param` other The settings to move from + */ + WheeledVehicleControllerSettings(WheeledVehicleControllerSettings &&other) noexcept = default; + + /** + * `@brief` Move assignment operator + * `@param` other The settings to move from + * `@return` Reference to this object + */ + WheeledVehicleControllerSettings &operator=(WheeledVehicleControllerSettings &&other) noexcept = default; + /** * `@brief` Construct a controller from these settings
34-35: Unused forward declaration.
WheeledVehicleController is forward-declared but not referenced anywhere in this header file. If it's for future use, consider adding a comment; otherwise, it can be removed.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agentsIn `@src/plugin/physics/src/system/VehicleSystem.cpp`: - Around line 46-63: The code sets a rear-only handbrake default then immediately overwrites it unconditionally; modify the assignment to joltWheel.mMaxHandBrakeTorque so it preserves the rear-only behavior by only applying wheelSettings.maxHandBrakeTorque when the wheel is rear-mounted (use the existing isRear flag), e.g. wrap the joltWheel.mMaxHandBrakeTorque = wheelSettings.maxHandBrakeTorque assignment in a conditional that checks isRear (or remove the initial rear-only default if you prefer front wheels to follow WheelSettings), referencing joltWheel, wheelSettings, WheelSettings and the isRear flag to locate the change.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agentsIn `@src/plugin/physics/src/system/VehicleSystem.cpp`: - Around line 114-120: The loop copies vehicle.engine.normalizedTorque into controllerSettings.mEngine.mNormalizedTorque but doesn't guard against an explicitly empty user vector; update the logic around controllerSettings.mEngine.mNormalizedTorque (in VehicleSystem.cpp) to check vehicle.engine.normalizedTorque.empty() and if empty either populate controllerSettings.mEngine.mNormalizedTorque with the default EngineSettings curve points (the three defaults defined on EngineSettings) or log/throw an error and fall back to those defaults; ensure the check precedes Clear/Reserve/AddPoint so the controller always ends up with at least one torque point.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agentsIn `@src/plugin/physics/src/system/VehicleSystem.cpp`: - Around line 148-151: The warning text in VehicleSystem.cpp is misleading because it says "based on first forward gear" while the code uses a hardcoded -2.90f; update the logic in the block that sets controllerSettings.mTransmission.mReverseGearRatios (inside the function handling gearbox defaults) to derive the reverse ratio from the first forward gear instead: read the first entry from vehicle.gearbox.forwardGearRatios (e.g., vehicle.gearbox.forwardGearRatios[0] or the appropriate variable holding forward gears), set defaultReverseRatio = -forwardRatio, push that into mReverseGearRatios, and keep or adjust the Log::Warn message to reflect that the default is derived from the first forward gear. In `@src/plugin/physics/src/utils/WheeledVehicleControllerSettings.hpp`: - Line 58: The copy constructor for WheeledVehicleControllerSettings is declared explicit which breaks normal copy-initialization and STL expectations; remove the explicit specifier from the copy constructor declaration (i.e., change "explicit WheeledVehicleControllerSettings(const WheeledVehicleControllerSettings &other) = default;" to "WheeledVehicleControllerSettings(const WheeledVehicleControllerSettings &other) = default;") so the class restores standard copy semantics used by assignments, container operations, and algorithms.
src/plugin/physics/src/utils/WheeledVehicleControllerSettings.hpp (1)src/plugin/physics/src/system/VehicleSystem.cpp (1)34-35: Unused forward declaration.
WheeledVehicleController is forward-declared but not referenced in this header. Consider removing it unless it's planned for future use.
Proposed fix-// Forward declaration -class WheeledVehicleController; - /** * `@brief` Custom wrapper for Jolt's WheeledVehicleControllerSettings47-68: Consider adding an empty-vector guard for friction curves.
While WheelSettings initializes both longitudinalFriction and lateralFriction with default values, the code in lines 56-68 clears the Jolt friction curves without checking if the source vectors are empty. This is unlikely to occur in normal usage due to the defaults, but explicitly clearing these vectors would clear Jolt's curves with no points added. A defensive guard (e.g., if (!wheelSettings.longitudinalFriction.empty())) before adding friction points would follow the principle of defensive programming and prevent unexpected behavior if defaults are ever explicitly cleared.
Sorry, something went wrong.
|
Sorry, something went wrong.
Not related to any issues
Two changes that can later be used to enhance vehicle handling:
Also fixed the crashing that can occur on some systems because of the mesh-mesh collisions (unsupported by Jolt). Now the car chassis uses a convex hull collider, so that the map can use a full mesh itself.
Summary by CodeRabbit
New Features
Changes
✏️ Tip: You can customize this high-level summary in your review settings.