Add service and GUI to configure physics parameters (step size and real time factor)

[luca-della-vedova]

This PR addresses #489 introducing a service in UserCommands advertised under /world/world_name/set_physics to set physics parameters (for now only step size and maximum real time factor), as well as updates to the ComponentInspector GUI to set them.

An example of the GUI is below:

There are a few parts of the implementation that I found suspicious while writing, specifically:

• Is the placement of the parameters updates in SimulationRunner OK? It is currently placed in the main Step() loop.
• There is a lot of code duplication for creating the physics parameters, they are currently initialized in the SimulationRunner constructor, SdfEntityCreator world creation function and LevelManager initialization.
• I created both a Physics and a PhysicsCmd component to hold the current physics data (the first) and send updates (the second), similar to what is done in Add Light Usercommand and include Light parameters in the componentInspector #482 with Light and LightCmd, as well as Pose and PoseCmd.
• I added the plot icon but there is no plotting feature yet, I suspect for these parameters it doesn't make much sense to plot them so I'll probably remove it.
• Also there was some discussion about allowing reconfiguration of Gravity and MagneticField. Right now however, they have separate components, and they are not stored in the sdf::Physics object, hence I suspect it might not be possible if we stick to having the Physics component store the sdf::Physics object.

Most importantly, it seems the physics component that is now updated in SimulationRunner isn't updated properly when the ComponentInspector field is deselected by moving the mouse away, instead of pressing return. It seems the value in the physics simulation is updated but the one in the GUI isn't, and there is a delay between the value displayed in the GUI and the one being actually used in the simulation (shown below):

I could only get this to work by updating the Physics component in the UserCommands service callback but that sounded like a very hacky fix, I left it commented just to have an idea of what it would look like.

[ahcorde]

You have included the two elements as drawable (at least I can see the icon close to the name). You should include these new parameters to the Plotting class (src/gui/plugins/plotting/Plotting.hh, src/gui/plugins/plotting/Plotting.cc)

[luca-della-vedova]

You have included the two elements as drawable (at least I can see the icon close to the name). You should include these new parameters to the Plotting class (src/gui/plugins/plotting/Plotting.hh, src/gui/plugins/plotting/Plotting.cc)

Yea it was one of the parts I was not super sure about, I was not sure whether it would be useful to have the parameters drawable since, unlike other drawable parameters like Pose they might not be that significant to have on a plot. I was leaning more towards removing the plot icon but if you think there might be some cases in which a user might want to plot the maximum RTF / physics step size over time I can add the changes to the plotting system.

[chapulina]

cases in which a user might want to plot the maximum RTF / physics step size over time

I can imagine someone plotting that together with other data to see how it's affected. I don't think it's extremely necessary though, so it's up to you if you want to remove the plot icons or add full plot support.

[luca-della-vedova]

@mrushyendra can you sync your fork to this branch and sign-off your commits for the DCO error? I'll resync this branch afterwards.

[mrushyendra]

@mrushyendra can you sync your fork to this branch and sign-off your commits for the DCO error? I'll resync this branch afterwards.

@luca-della-vedova Yep sure! Will do by the end of today

Edit: Done

[ahcorde]

Do you mind to add some tests to test/integration/user_commands.cc ?

[codecov]

Codecov Report

Merging #536 (c997f06) into ign-gazebo4 (d8b9d0c) will decrease coverage by 0.21%.
The diff coverage is 67.96%.

@@               Coverage Diff               @@
##           ign-gazebo4     #536      +/-   ##
===============================================
- Coverage        77.36%   77.14%   -0.22%     
===============================================
  Files              215      215              
  Lines            12029    12026       -3     
===============================================
- Hits              9306     9278      -28     
- Misses            2723     2748      +25     

Impacted Files	Coverage Δ
src/SimulationRunner.hh	100.00% <ø> (ø)
.../plugins/component_inspector/ComponentInspector.cc	8.61% <0.00%> (-0.65%)	⬇️
.../plugins/component_inspector/ComponentInspector.hh	28.57% <ø> (ø)
src/LevelManager.cc	88.62% <80.00%> (-0.12%)	⬇️
src/SdfEntityCreator.cc	93.19% <80.00%> (-0.23%)	⬇️
src/systems/user_commands/UserCommands.cc	79.66% <86.20%> (+0.69%)	⬆️
src/SimulationRunner.cc	91.81% <92.59%> (-2.23%)	⬇️
include/ignition/gazebo/components/Physics.hh	100.00% <100.00%> (ø)
include/ignition/gazebo/components/PhysicsCmd.hh	100.00% <100.00%> (ø)
src/Conversions.cc	81.89% <100.00%> (+0.25%)	⬆️
... and 8 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d8b9d0c...263bf77. Read the comment docs.

[luca-della-vedova]

Do you mind to add some tests to test/integration/user_commands.cc ?

Done! A simple "create world, check physics properties, set them through service and check them again".

[ahcorde]

LGTM, just a minor comment

[ahcorde]

LGTM

[chapulina]

I haven't tried it out yet, but here's some feedback 😉

---

There is a lot of code duplication for creating the physics parameters

Yeah it would be nice to clean this up a bit... It would take some refactoring. I think for now this PR is ok.

discussion about allowing reconfiguration of Gravity and MagneticField.

Yeah I think these can be handled separately

[chapulina]

This is a really useful feature! I just have a small usability suggestion, and then feel free to merge 😉

[peci1]

Thanks for the feature. One more usability problem is that you cannot set rtf without also setting time step via the service. I.e. a service call like this

ign service -s /world/simple_cave_01/set_physics --reqtype ignition.msgs.Physics --reptype ignition.msgs.Boolean --timeout 2000 --req "real_time_factor: 1"

results in stopped (or broken?) simulation, because time step is read as zero from the request.

Setting both rtf and step size works, but I see this in the console:

[Dbg] [SimulationFeatures.cc:48] Simulation timestep set to: 0.004
Warning [World.cpp:139] [World] Attempting to set negative timestep. Ignoring this request because it can lead to undefined behavior.

Which is weird because I can't figure out where does the World.cpp file come from (I haven't found it anywhere).

Would it be possible to check whether both rtf and time step are non-zero before using them? I think a zero value is wrong for both of them.

[chapulina]

I can't figure out where does the World.cpp file come from

That's coming from DART.

Would it be possible to check whether both rtf and time step are non-zero before using them?

+1 - On Gazebo classic a zero "real time update rate" means "run as fast as possible", but we don't have that on Ignition, so I think it's safe to assume a value of zero is unset or user error.

[luca-della-vedova]

Would it be possible to check whether both rtf and time step are non-zero before using them? I think a zero value is wrong for both of them.

Hi @peci1 and thanks for the feedback!
This should be fixed in #740 , can you check if it works for you?