Repository navigation
Moved PID computation code to On Scene update. - #1096
Open
michalpelka wants to merge 1 commit into
Open
michalpelka wants to merge 1 commit into
michalpelka wants to merge 1 commit into
Conversation
Signed-off-by: Michał Pełka <michal.pelka@robotec.ai>
michalpelka
requested review from
jhanca-robotecai
and
a lite review from Copilot
September 7, 2026 15:14
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Current changes introduce cases where controllers can become inert (no steering / no motor updates) and risk duplicate scene handler registrations without proper lifecycle handling.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR moves PID control-loop updates for steering (Ackermann drive model) and motorized joints onto the fixed-timestep physics sub-step callback (OnSceneSimulationFinish) instead of the variable render/tick update, aiming to make PID integration time-stable and reduce sensitivity to launcher-specific joint readout instability (ref. #1079).
Changes:
- Registers
AzPhysics::SceneEvents::OnSceneSimulationFinishHandlerto run steering PID once per physics sub-step inAckermannDriveModel. - Latches steering command in
ApplyState()and applies steering during the physics callback. - Updates
PidMotorControllerComponentto stop using TickBus and instead run its PID loop on the physics sub-step callback.
File summaries
| File | Description |
|---|---|
| Gems/ROS2Controllers/Code/Source/VehicleDynamics/DriveModels/AckermannDriveModel.h | Adds physics scene callback handler/member for per-substep steering PID execution. |
| Gems/ROS2Controllers/Code/Source/VehicleDynamics/DriveModels/AckermannDriveModel.cpp | Registers scene-finish handler and moves steering PID execution to the physics loop. |
| Gems/ROS2Controllers/Code/Source/Manipulation/MotorizedJoints/PidMotorControllerComponent.cpp | Moves joint PID updates from TickBus to physics scene-finish callback. |
| Gems/ROS2Controllers/Code/Include/ROS2Controllers/Manipulation/MotorizedJoints/PidMotorControllerComponent.h | Declares the physics scene-finish callback and handler member. |
Review details
Suppressed comments (1)
Gems/ROS2Controllers/Code/Source/VehicleDynamics/DriveModels/AckermannDriveModel.cpp:106
- If the physics-scene finish handler fails to register (e.g. no default scene), ApplyState() no longer calls ApplySteering(), so steering will never be applied. Consider falling back to the old tick-driven steering update when the handler isn't connected.
const auto jointPositions = inputs.m_jointRequestedPosition;
// The steering PID consumes this on the physics step; only the target is latched here.
m_steeringCommand = jointPositions.empty() ? 0 : jointPositions.front();
ApplySpeed(inputs.m_speed.GetX(), deltaTimeNs);
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+54
to
+56
| float m_steeringCommand = 0.0f; //!< Steering angle requested by the last input state, in radians. | ||
| AckermannModelLimits m_limits; | ||
| AzPhysics::SceneEvents::OnSceneSimulationFinishHandler m_sceneFinishSimHandler; //!< Handler called after every physics sub-step |
Comment on lines
+61
to
82
| // This controller closes its own loop on the physics step, so the base class' tick-driven | ||
| // update is not used. | ||
| AZ::TickBus::Handler::BusDisconnect(); | ||
|
|
||
| auto* sceneInterface = AZ::Interface<AzPhysics::SceneInterface>::Get(); | ||
| AZ_Assert(sceneInterface, "No scene interface"); | ||
| const AzPhysics::SceneHandle defaultSceneHandle = | ||
| sceneInterface ? sceneInterface->GetSceneHandle(AzPhysics::DefaultPhysicsSceneName) : AzPhysics::InvalidSceneHandle; | ||
| AZ_Assert(defaultSceneHandle != AzPhysics::InvalidSceneHandle, "Invalid default physics scene handle"); | ||
| if (defaultSceneHandle == AzPhysics::InvalidSceneHandle) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| m_sceneFinishSimHandler = AzPhysics::SceneEvents::OnSceneSimulationFinishHandler( | ||
| [this]([[maybe_unused]] AzPhysics::SceneHandle sceneHandle, float fixedDeltaTime) | ||
| { | ||
| OnSceneSimulationFinish(fixedDeltaTime); | ||
| }, | ||
| aznumeric_cast<int32_t>(AzPhysics::SceneEvents::PhysicsStartFinishSimulationPriority::Components)); | ||
| sceneInterface->RegisterSceneSimulationFinishHandler(defaultSceneHandle, m_sceneFinishSimHandler); | ||
| } |
Comment on lines
+70
to
+77
|
|
||
| m_sceneFinishSimHandler = AzPhysics::SceneEvents::OnSceneSimulationFinishHandler( | ||
| [this]([[maybe_unused]] AzPhysics::SceneHandle sceneHandle, float fixedDeltaTime) | ||
| { | ||
| OnSceneSimulationFinish(fixedDeltaTime); | ||
| }, | ||
| aznumeric_cast<int32_t>(AzPhysics::SceneEvents::PhysicsStartFinishSimulationPriority::Components)); | ||
| sceneInterface->RegisterSceneSimulationFinishHandler(defaultSceneHandle, m_sceneFinishSimHandler); |
byrcolin
added
the
sig/simulation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Moved PID computatio to timestamp stable physics loop.
Simulate PID is executed with large frequency.
Potential fix for #1079
How was this PR tested?