Physics: remove *VelocityCmd at each time step #2228

scpeters · 2023-11-04T08:16:50Z

🎉 New feature

Part of #1926.

Summary

This implements the suggestion from #1926 (comment) to delete all *VelocityCmd components after each time step.

Test it

I'm still figuring out how to test it.

Checklist

Signed all commits for DCO
Added tests
Added example and/or tutorial
Updated documentation (as needed)
Updated migration guide (as needed)
Consider updating Python bindings (if the library has them)
codecheck passed (See contributing)
All tests passed (See test coverage)
While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Part of #1926. Signed-off-by: Steve Peters <[email protected]>

Signed-off-by: Steve Peters <[email protected]>

Since JointVelocityCmd components are deleted after each timestep, don't skip updating forces and moments if the component does not exist, and use the SetComponent API to more cleanly handle the component creation logic. Signed-off-by: Steve Peters <[email protected]>

Signed-off-by: Steve Peters <[email protected]>

codecov · 2023-11-05T17:57:53Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.84%. Comparing base (ae75c25) to head (714e948).
Report is 73 commits behind head on main.

❗ Current head 714e948 differs from pull request most recent head f4ffd4a. Consider uploading reports for the commit f4ffd4a to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2228      +/-   ##
==========================================
+ Coverage   65.66%   65.84%   +0.18%     
==========================================
  Files         324      323       -1     
  Lines       30938    30669     -269     
==========================================
- Hits        20315    20194     -121     
+ Misses      10623    10475     -148

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Steve Peters <[email protected]>

scpeters · 2023-11-06T05:11:53Z

cleaned up some code and improved coverage with SetComponentData in b145c05

azeey

Looks good so far!

It would be good to characterize which systems are changing behavior due to this. For example, it looks like the DiffDrive system won't change behavior because it'll constantly be sending the last twist command it received.

azeey · 2023-11-10T19:57:16Z

src/systems/multicopter_motor_model/MulticopterMotorModel.cc

-  {
-    _ecm.CreateComponent(this->dataPtr->jointEntity,
-        components::JointVelocityCmd({0}));
-    doUpdateForcesAndMoments = false;


Do we need to preserve the doUpdateForcesAndMoments = false;?

no, the other places that variable is set to false is when a physics state / sensor reading component is unavailable that is needed for the control calculation, but JointVelocityCmd is an actuator command written by this system, so it's ok if it's missing

scpeters · 2023-11-11T00:56:54Z

It would be good to characterize which systems are changing behavior due to this. For example, it looks like the DiffDrive system won't change behavior because it'll constantly be sending the last twist command it received.

the following systems reference a *VelocityCmd component:

$ grep -rlI VelocityCmd src/systems | sort 
src/systems/ackermann_steering/AckermannSteering.cc
src/systems/battery_plugin/LinearBatteryPlugin.cc
src/systems/diff_drive/DiffDrive.cc
src/systems/joint_controller/JointController.cc
src/systems/joint_position_controller/JointPositionController.cc
src/systems/mecanum_drive/MecanumDrive.cc
src/systems/multicopter_motor_model/MulticopterMotorModel.cc
src/systems/physics/Physics.cc
src/systems/thruster/Thruster.cc
src/systems/trajectory_follower/TrajectoryFollower.cc
src/systems/velocity_control/VelocityControl.cc

The LinearBatteryPlugin only reads the content of a JointVelocityCmd; it does not modify them, so it's behavior will not change
The Physics system does not set the content of any *VelocityCmd components; it only removes them via the changes in this PR

The following systems are already modified by this pull request to use the SetComponentData API, and I have checked them to verify that their behavior will not change.

src/systems/ackermann_steering/AckermannSteering.cc
src/systems/diff_drive/DiffDrive.cc
src/systems/joint_controller/JointController.cc
src/systems/joint_position_controller/JointPositionController.cc
src/systems/multicopter_motor_model/MulticopterMotorModel.cc
src/systems/thruster/Thruster.cc
src/systems/velocity_control/VelocityControl.cc

I still need to review the following two systems:

src/systems/mecanum_drive/MecanumDrive.cc
src/systems/trajectory_follower/TrajectoryFollower.cc

Signed-off-by: Steve Peters <[email protected]>

Now that the physics system is removing AngularVelocityCmd components at every timestep, that logic can be removed from the trajectory follower system. Signed-off-by: Steve Peters <[email protected]>

scpeters · 2023-11-11T01:35:49Z

I still need to review the following two systems:

src/systems/mecanum_drive/MecanumDrive.cc

src/systems/trajectory_follower/TrajectoryFollower.cc

Upon further review, I found that the MecanumDrive system's behavior is unchanged, though I took the opportunity to use the SetComponentData API in 7dc33fe. The TrajectoryFollower system was already removing AngularVelocityCmd components at every timestep that it didn't want to set them, which is now not necessary, so I removed that logic in 120e5fe

scpeters · 2023-11-11T01:42:23Z

I also noticed that the MecanumDrive and TrajectoryFollower systems don't have integration tests, so I filed #2238 and #2239.

mjcarroll · 2023-11-27T14:35:51Z

So not something that needs to be addressed here, but I believe that adding and removing components is relatively expensive (compared to setting/unsetting them) as they can incur allocation/deallocation as well as additional bookkeeping inside the ECM.

I think in the future, we should consider having an optional<Component> type that remains attached to an entity, but isn't created/destroyed each iteration.

scpeters · 2023-11-27T19:00:04Z

I think in the future, we should consider having an optional<Component> type that remains attached to an entity, but isn't created/destroyed each iteration.

That could make sense if it makes a significant performance improvement. We could define each component as Component<std::optional<T> ..., but I'm guessing your proposal sounds a little different than that

mjcarroll · 2023-11-27T19:08:42Z

That could make sense if it makes a significant performance improvement.

I don't have a good gauge of the impact. I'm going to assume that not allocating/bookkeeping are always going to be better than allocation/bookkeeping. I think in this case, the trade-off would be a small amount of memory overhead, because you would be keeping the components in question (or a pointer to them).

We could define each component as Component<std::optional ..., but I'm guessing your proposal sounds a little different than that

That's basically what I'm suggesting. We may be able to do a little bit of syntactic sugar to make an OptionalComponent<T>.
That could enable letting Each calls only iterate over components that are set/unset.

mjcarroll · 2023-11-27T20:27:16Z

as a followup on the performance side of things, the concept of "optional components" was mostly added already: #1334 (comment)

So that is one less thing to worry about here.

azeey · 2024-03-18T18:46:23Z

@scpeters what's the status of this PR?

scpeters · 2024-04-04T07:14:14Z

@scpeters what's the status of this PR?

sorry I missed this notification until now. I thought it was good to go in November, but it may have gone out of date since then. I can open a separate pull request for the SetComponentData changes in the other systems to reduce the noise if that would help

scpeters · 2024-04-05T07:09:40Z

I can open a separate pull request for the SetComponentData changes in the other systems to reduce the noise if that would help

#2360

scpeters · 2024-04-05T22:46:57Z

I can open a separate pull request for the SetComponentData changes in the other systems to reduce the noise if that would help

#2360

I merged these changes to gz-sim8 and then forward to main and merged into this branch, so the diff is a bit smaller now. This is ready for review again

iche033

looks good to me

iche033 · 2024-04-06T00:40:37Z

Migration.md

+## Gazebo Sim 8.x to 9.0
+
+ * **Modified**:
+   + In the Physics system, all `*VelocityCmd` components are deleted after


mention that previously it was setting them to zero?

Signed-off-by: Steve Peters <[email protected]>

azeey · 2024-04-08T14:10:43Z

Migration.md

@@ -5,6 +5,14 @@ Deprecated code produces compile-time warnings. These warning serve as
 notification to users that their code should be upgraded. The next major
 release will remove the deprecated code.

+## Gazebo Sim 8.x to 9.0
+
+ * **Modified**:


Thanks for adding this!

azeey · 2024-04-08T14:20:26Z

test/worlds/quadcopter.sdf

@@ -91,7 +91,7 @@
          </geometry>
        </collision>
        <visual name="rotor_0_visual">
-          <pose>0 0 0 1.57 0 0 0</pose>
+          <pose>0 0 0 1.57 0 0</pose>


I realized last week while trying to fix another test that the quadcopter test is passing even though this SDF was invalid. I'll be making the same changes in other branches.

This was referenced Nov 4, 2023

Clearing pending commands - unexpected behavior #1926

Closed

Change various CMDs to use std::optional #1334

Open

Physics: remove *VelocityCmd at each time step

4070cac

Part of #1926. Signed-off-by: Steve Peters <[email protected]>

scpeters force-pushed the scpeters/physics_remove_velocity_cmds branch from 28bccb3 to 4070cac Compare November 5, 2023 06:04

scpeters added 4 commits November 5, 2023 01:06

Fix pose syntax error in quadcopter test worlds

740071a

Signed-off-by: Steve Peters <[email protected]>

wheel_slip: use SetComponentData

c963956

Signed-off-by: Steve Peters <[email protected]>

Fix gz-sensors branch in custom sensor example

f6ecc61

Signed-off-by: Steve Peters <[email protected]>

scpeters marked this pull request as ready for review November 5, 2023 17:11

scpeters requested review from azeey and mjcarroll as code owners November 5, 2023 17:12

Use SetComponentData to improve coverage, simplify

b145c05

Signed-off-by: Steve Peters <[email protected]>

azeey reviewed Nov 10, 2023

View reviewed changes

scpeters added 2 commits November 10, 2023 17:27

Use SetComponentData in MecanumDrive as well

7dc33fe

Signed-off-by: Steve Peters <[email protected]>

TrajectoryFollower: don't need to remove commands

120e5fe

Now that the physics system is removing AngularVelocityCmd components at every timestep, that logic can be removed from the trajectory follower system. Signed-off-by: Steve Peters <[email protected]>

scpeters mentioned this pull request Dec 12, 2023

velocity_control: allow non-persistent initial velocity commands #2265

Closed

9 tasks

azeey added the 🏛️ ionic Gazebo Ionic label Feb 6, 2024

scpeters mentioned this pull request Apr 5, 2024

Use SetComponentData to simplify code and improve coverage #2360

Merged

8 tasks

Merge branch 'main' into scpeters/physics_remove_velocity_cmds

be5214f

iche033 approved these changes Apr 6, 2024

View reviewed changes

Update Migration.md

f4ffd4a

Signed-off-by: Steve Peters <[email protected]>

azeey approved these changes Apr 8, 2024

View reviewed changes

scpeters merged commit 77f4c00 into main Apr 9, 2024
3 of 6 checks passed

scpeters deleted the scpeters/physics_remove_velocity_cmds branch April 9, 2024 19:08

scpeters mentioned this pull request Jul 30, 2024

Physics: set link velocity from *VelocityReset components #2489

Merged

9 tasks

scpeters mentioned this pull request Oct 23, 2024

bullet-featherstone: JointVelocityCmd components remain in effect even after being deleted #2654

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Physics: remove *VelocityCmd at each time step #2228

Physics: remove *VelocityCmd at each time step #2228

scpeters commented Nov 4, 2023

codecov bot commented Nov 5, 2023 •

edited

Loading

scpeters commented Nov 6, 2023

azeey left a comment

azeey Nov 10, 2023

scpeters Nov 11, 2023

scpeters commented Nov 11, 2023

scpeters commented Nov 11, 2023

scpeters commented Nov 11, 2023

mjcarroll commented Nov 27, 2023

scpeters commented Nov 27, 2023

mjcarroll commented Nov 27, 2023

mjcarroll commented Nov 27, 2023

azeey commented Mar 18, 2024

scpeters commented Apr 4, 2024

scpeters commented Apr 5, 2024

scpeters commented Apr 5, 2024

iche033 left a comment

iche033 Apr 6, 2024

scpeters Apr 7, 2024

azeey Apr 8, 2024

azeey Apr 8, 2024

Physics: remove *VelocityCmd at each time step #2228

Physics: remove *VelocityCmd at each time step #2228

Conversation

scpeters commented Nov 4, 2023

🎉 New feature

Summary

Test it

Checklist

codecov bot commented Nov 5, 2023 • edited Loading

Codecov Report

scpeters commented Nov 6, 2023

azeey left a comment

Choose a reason for hiding this comment

azeey Nov 10, 2023

Choose a reason for hiding this comment

scpeters Nov 11, 2023

Choose a reason for hiding this comment

scpeters commented Nov 11, 2023

scpeters commented Nov 11, 2023

scpeters commented Nov 11, 2023

mjcarroll commented Nov 27, 2023

scpeters commented Nov 27, 2023

mjcarroll commented Nov 27, 2023

mjcarroll commented Nov 27, 2023

azeey commented Mar 18, 2024

scpeters commented Apr 4, 2024

scpeters commented Apr 5, 2024

scpeters commented Apr 5, 2024

iche033 left a comment

Choose a reason for hiding this comment

iche033 Apr 6, 2024

Choose a reason for hiding this comment

scpeters Apr 7, 2024

Choose a reason for hiding this comment

azeey Apr 8, 2024

Choose a reason for hiding this comment

azeey Apr 8, 2024

Choose a reason for hiding this comment

codecov bot commented Nov 5, 2023 •

edited

Loading