gz-sim icon indicating copy to clipboard operation
gz-sim copied to clipboard

Change various CMDs to use `std::optional`

Open arjo129 opened this issue 3 years ago • 4 comments

Desired behavior

LinearVelocityCmds are reset to zero at every time step instead of being removed. This means that once a user sets a Link to have a certain predefined velocity, then it is assumed in all subsequent updates that the link will have zero velocity (https://github.com/ignitionrobotics/ign-gazebo/blob/77dd8551e942c6f2a9dfbd6360a51cab9185ebab/src/systems/physics/Physics.cc#L3068-L3073). This is somewhat counter intuitive as it is not documented. Furthermore it also prevents users from setting an initial velocity on a certain link which may be useful at certain times for testing purposes (particularly with systems like LiftDrag and Hydrodynamics).

Alternatives considered

Removing the component altogether. This however would have a performance penalty as it may lead to memory deallocation.

Implementation suggestion

Recommend making the following section: https://github.com/ignitionrobotics/ign-gazebo/blob/77dd8551e942c6f2a9dfbd6360a51cab9185ebab/include/ignition/gazebo/components/LinearVelocityCmd.hh#L38-L42

Into:

using LinearVelocityCmd = Component<
  std::optional<math::vector3d>, class LinearVelocityCmdTag>;
  IGN_GAZEBO_REGISTER_COMPONENT(
      "ign_gazebo_components.LinearVelocityCmd", LinearVelocityCmd)

arjo129 avatar Feb 11 '22 06:02 arjo129

It would be nice if we can somehow use the idea of std::optional but bake into the ECM so that removing a component is equivalent to resetting a std::optional. And hopefully, this can be done without breaking API/ABI.

azeey avatar Feb 11 '22 16:02 azeey

Alternatives considered

Removing the component altogether. This however would have a performance penalty as it may lead to memory deallocation.

I'm actually not sure if this is true - when I worked on #856, I created a "removed components" mechanism which is used internally by the ECM to mimic components being added/removed without actually removing them. The reason why I did this is because I found that the pre-existing approach of actually removing components from memory was a costly operation. If a downstream user calls APIs to remove a component from the ECM, internally, the ECM should ignore the corresponding component, even though this component still exists. Then, if the user adds this component again, the component data is updated accordingly and is no longer ignored. So, from the user's perspective, the component has been added/removed, but internally, the component is never actually deallocated from memory (it's just being ignored).

@arjo129 have you tried removing the *Cmd components you don't need after applying the command to see if there's any noticeable performance impact?

adlarkin avatar Feb 11 '22 19:02 adlarkin

  • when I worked on https://github.com/ignitionrobotics/ign-gazebo/pull/856, I created a "removed components" mechanism which is used internally by the ECM to mimic components being added/removed without actually removing them.

Ah, this is already solved then. That's great! We just need to update the behavior of the physics system to remove the components instead of resetting them. I think that's feasible for Garden.

azeey avatar Feb 11 '22 20:02 azeey

Ah, this is already solved then. That's great! We just need to update the behavior of the physics system to remove the components instead of resetting them. I think that's feasible for Garden.

this is similar to (if not a duplicate of) #1926 and an implementation attempt is in #2228

scpeters avatar Nov 04 '23 08:11 scpeters