Added Reset Simulation

[triccyx]

Added to clock gazebo plugin the possibility to reset simulation via rpc call

[traversaro]

I guess you want to merge the modification in the devel branch, right?
You can change the target branch of the PR directly from the GitHub web user interface.

[traversaro]

Modifying this thrift file is unfortunately extremely tricky, because we have multiple copies of this file disseminated across several repository, see for example robotology/wb-toolbox@af1fa41 . I guess @diegoferigo and @S-Dafarra are the maintainers of the repositories affected by this.

Before merging this modification, we should make sure that the client generated with the old version of the thrift file are still compatible with the server exposed by the new version of the thrift.

The clean solution is probably to avoid this duplication. This involves either:

• Add to gazebo-yarp-plugins all the CMake machinery necessary for making sure that the thrift/clock/clock_rpc.thrift file is installed and can be found by the system. The problem of this solution would be that all the repository that use that file will now depend on gazebo-yarp-plugins and thus will depend indirectly on Gazebo. This is definitely problematic, think for example on how this will affect Windows.
• Add this interface to YARP: if we are able to ensure that this interface is relatively simulator-independent, it would make sense to have this in YARP. On a positive note, at the moment the interface is quite independent from the underlying simulator, as long as it is a fixed time-step simulator as the "Step" functions take the number of steps as input, so it could make sense to have this in YARP.

[diegoferigo]

@triccyx I remember a discussion with @francesco-romano (previous WB-Toolbox maintainer) about removing that thrift-related sources as soon as YARP would have installed them in the system. I'd go as well in this direction rather than storing it into gazebo-yarp-plugins.

[diegoferigo]

Cross ref robotology/wb-toolbox#62

[triccyx]

What is the best way to resolve the problem? Without this merge the tests won't work

[diegoferigo]

I'd like to see this thrift to end into yarp. Without spending too much time to figure out if it is or isn't good enough, since it seems already sufficiently generic, why not opening a PR and ask for its inclusion? It might be a quick process. @traversaro

[traversaro]

I agree with @diegoferigo .

[drdanz]

I think we need to discuss this... At the moment I wouldn't know

• where to put the thrift interfaces
• whether we want to include the thrift only, or to generate a library as well
• In case we want to add a library, we need to decide if we want it static or shared
• if shared we need to modify the thrift generator to allow to handle the dll import/export
• Also we need to commit the generated files in the repository

I suggest to merge this PR and to have a meeting about this after YARP 3 release

[triccyx]

Ok for Add ClockServer thrift file #1724, but this doesn't solve the problem.
The client at the moment can't execute the tests. Perhaps we can merge these modifications in a branch in master and ask the client to use that branch.

[traversaro]

If clients with the old interface are still able to call the methods in the new interface (I think should be the case, is anyone is able to check this?), I don't think it would be too problematic to merge this PR.

However, I think the current method added is a bit misleading, as it is called reset, but it actually just reset the state of the simulation, and not the time. I think resetSimulation should reset both time and state of the simulation, and eventually a resetSimulationStateshould just reset the state without resetting the time.

[triccyx]

Any news/solutions regarding this pull request?

[traversaro]

Can you address my comments from the 4th of June?

However, I think the current method added is a bit misleading, as it is called reset, but it actually just reset the state of the simulation, and not the time. I think resetSimulation should reset both time and state of the simulation, and eventually a resetSimulationStateshould just reset the state without resetting the time.

[triccyx]

Ok I change the method name to ResetSimulation as it resets both state and time.

[triccyx]

ping

[traversaro]

Ok for me, but I would prefer if @diegoferigo could check this as well.

[traversaro]

Small leftover comment.

[triccyx]

@diegoferigo friendly ping

[diegoferigo]

I'm not very familiar with gazebo classic plugins. This PR looks good to me, but I have few questions that do not block merging:

1. Does this PR break the compatibility with the current version of wb-toolbox? I mean, I can update the thrift there, but would users that have an old version of wb-toolbox and the new version of gazebo-yarp-plugins (or the other way around) face any incompatibilities? Since this PR edits methods in a thrift service and these methods are not called, I do not think so. Can you confirm this?
2. Just a curiosity. Would the signal to reset the world be propagated to all the other plugins? E.g. if there are controllers, their state should be reset too (I think yes, just checking).

Btw, I am still looking for storing all these thrift files somewhere else, in a centralized place in order to avoid to keep vendoring these files in other projects.

[traversaro]

Can you confirm this?

Exactly. As you can check in the generated code for the server https://github.com/robotology/gazebo-yarp-plugins/pull/345/files#diff-9e60d5f99e3cf3eb4705af443b70c244R395 , nothing in the code changes for methods different from resetSimulation.
The only possible incompatibility is related to some new software that tries to call resetSimulation on an old version of gazebo-yarp-plugins.

Would the signal to reset the world be propagated to all the other plugins? E.g. if there are controllers, their state should be reset too (I think yes, just checking).

Plugins can handle the reset even by attaching a callback to the WorldReset event, see how the controlboard plugin handle this:

• gazebo-yarp-plugins/plugins/controlboard/src/ControlBoardDriver.cpp

  Line 342 in 01ad1d1

  gazebo::event::Events::ConnectWorldReset(boost::bind(&GazeboYarpControlBoardDriver::onReset, this));

• gazebo-yarp-plugins/plugins/controlboard/src/ControlBoardDriver.cpp

  Line 624 in 01ad1d1

  m_previousTime = gazebo::common::Time::Zero;

  
  old related issue and PR : Gazebo Reset World causes unrealistic joint torques #92 and [controlboard] Fix issue with ResetWorld #247

Btw, I am still looking for storing all these thrift files somewhere else, in a centralized place in order to avoid to keep vendoring these files in other projects.

I agree (remainder of YARP issue: robotology/yarp#1724).

[diegoferigo]

Ack. I'll keep in mind to update the wb-toolbox thrift in any case 👍