Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change various CMDs to use std::optional #1334

Open
arjo129 opened this issue Feb 11, 2022 · 4 comments
Open

Change various CMDs to use std::optional #1334

arjo129 opened this issue Feb 11, 2022 · 4 comments
Labels
enhancement New feature or request help wanted We accept pull requests!

Comments

@arjo129
Copy link
Contributor

arjo129 commented Feb 11, 2022

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 arjo129 added the enhancement New feature or request label Feb 11, 2022
@azeey
Copy link
Contributor

azeey commented Feb 11, 2022

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.

@adlarkin
Copy link
Contributor

adlarkin commented Feb 11, 2022

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?

@azeey
Copy link
Contributor

azeey commented Feb 11, 2022

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.

@chapulina chapulina added the help wanted We accept pull requests! label Mar 1, 2022
@scpeters
Copy link
Member

scpeters commented Nov 4, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted We accept pull requests!
Projects
None yet
Development

No branches or pull requests

5 participants