Support editing air pressure sensor in the GUI

[nkoenig]

Signed-off-by: Nate Koenig [email protected]

🎉 New feature

Summary

Adds support for editing the noise properties of the air pressure sensor in the component inspector.

Requires:

• Get updated GUI ECM info when a user presses 'play' #1109
• Support accessing mutable sensor types sdformat#737

I've made some changes to the component inspector in an attempt to make it more modular, these include:

1. We have ability to (mostly) encapsulate an inspector element into a set of three files consisting of a .cc, .hh, and .qml. See Added altimeter sensor inspector #1172 for an example of adding a new inspector element.
2. There is a bit of ugliness where an inspector element has to callout to the componentInspector in order to then call the c++ function. Any help here would be appreciated.
3. The header for a element can be customized. By default the header name is based on the QML filename.

Test it

1. ign gazebo -v 4 sensors.sdf
2. Take note of the output from ign model -m sensors_box -l link -s air_pressure
3. Select the altimeter sensor in the entity tree.
4. Make some changes to the values in the component inspector.
5. Rerun ign model -m sensors_box -l link -s air_pressure and you should see the changes.

Checklist

• Signed all commits for DCO
• Added tests
• Added example and/or tutorial
• Updated documentation (as needed)
• Updated migration guide (as needed)
• 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

[mjcarroll]

There is a bit of ugliness where an inspector element has to callout to the componentInspector in order to then call the c++ function. Any help here would be appreciated.

Here is an attempt at a different implementation that works by overloading the function protoype.

37eade4

[nkoenig]

There is a bit of ugliness where an inspector element has to callout to the componentInspector in order to then call the c++ function. Any help here would be appreciated.

Here is an attempt at a different implementation that works by overloading the function protoype.

37eade4

Thanks, but I think the ugliness is still there. The componentInspector is still required in your approach. I can't figure out ho to call AirPressure::OnAirPressureNoise from AirPressure.qml. I'm only able to call the function from ComponentInspector.qml. Also, @chapulina mentioned in a separate conversation that slots/signals are preferred to function overloading. If it's okay with you, I'll revert this change.

[mjcarroll]

If it's okay with you, I'll revert this change.

I didn't put it on your branch, made a new branch, so should still be good.

[chapulina]

There is a bit of ugliness where an inspector element has to callout to the componentInspector in order to then call the c++ function. Any help here would be appreciated.

I think I got it, see #1179

---

I think the new sensor noise is not taking effect. The ign-sensor objects are held at system::AirPressure:

https://github.com/ignitionrobotics/ign-gazebo/blob/ee8c3fd6e1a2cef3d999370033b9484b9d537f53/src/systems/air_pressure/AirPressure.cc#L53-L54

I checked it by plotting the sensor's readings and setting very high noise.

We should propagate the changes to the sensor there. The pattern we've been using is to create a service on UserCommands, which sets a component called XXXCmd, that's processed by the responsible system, in this case system::AirPressure. See an example in #536

[nkoenig]

There is a bit of ugliness where an inspector element has to callout to the componentInspector in order to then call the c++ function. Any help here would be appreciated.

I think I got it, see #1179

Thanks!

I think the new sensor noise is not taking effect. The ign-sensor objects are held at system::AirPressure:

https://github.com/ignitionrobotics/ign-gazebo/blob/ee8c3fd6e1a2cef3d999370033b9484b9d537f53/src/systems/air_pressure/AirPressure.cc#L53-L54

I checked it by plotting the sensor's readings and setting very high noise.

We should propagate the changes to the sensor there. The pattern we've been using is to create a service on UserCommands, which sets a component called XXXCmd, that's processed by the responsible system, in this case system::AirPressure. See an example in #536

Yeah, the noise values will not take effect until: #1170. You should see the changes in the ECM, just not in simulation output. I'm trying not to utilize additional services via the UserCommands pattern.

[chapulina]

Yeah, the noise values will not take effect until: #1170. You should see the changes in the ECM, just not in simulation output. I'm trying not to utilize additional services via the UserCommands pattern.

I see, so is the idea that the component inspector will add the Recreate component to the parent model whenever the user edits a value? Is that going to happen only while paused or also while playing?

I understand that the *Cmd pattern involves a lot of boilerplate and bloats the list of services, but that's efficient and works at runtime. Recreating the model just to edit a few sensor properties will be extremely inefficient if we do it every time the user clicks the spin arrows. For this particular use case, I think that a single components::SensorCmd and a ~/sensor_config service that handles all sensor modifications.

[nkoenig]

Yeah, the noise values will not take effect until: #1170. You should see the changes in the ECM, just not in simulation output. I'm trying not to utilize additional services via the UserCommands pattern.

I see, so is the idea that the component inspector will add the Recreate component to the parent model whenever the user edits a value? Is that going to happen only while paused or also while playing?

Yes , that's correct.

I understand that the *Cmd pattern involves a lot of boilerplate and bloats the list of services, but that's efficient and works at runtime.

We are currently only trying to get parameters editable by the GUI between play/pause. The service pattern would become quite large when we add in all the sensors and joints, and we don't want users to edit these parameters while simulation running (at least for right now). The ECM sync is a more efficient way to handle all of the parameter updating that we want to do via the GUI at the moment, and has a better user experience (see below). In the long run it would be nice to add in the concept of parameters, similar to ROS parameters.

Recreating the model just to edit a few sensor properties will be extremely inefficient if we do it every time the user clicks the spin arrows. For this particular use case, I think that a single components::SensorCmd and a ~/sensor_config service that handles all sensor modifications.

The ECM is only created/synced when a sensor is added or modified. Unless a user is making a change every step, then they shouldn't notice. An asynchronous service approach adds complexity and noticeable delays. For example, the pose update still uses the service method, and I see the following behavior:

1. Translate a model in the GUI.
2. The model bounces back to it's original position.
3. Server gets the new pose and sends back the new correct pose.
4. The model bounce to the location I set in step 1.

This is the type of user experience that I'd like to avoid.

[chapulina]

we don't want users to edit these parameters while simulation running

Got it. I think this PR is missing the concept of a read-only mode for the new widgets then, so the user doesn't edit the sensor while playing by mistake.

The ECM sync is a more efficient way to handle all of the parameter updating that we want to do via the GUI at the moment

I can see how that's efficient if lots of changes are queued while paused and only handled on play. This important detail is missing from this PR though.

An asynchronous service approach adds complexity and noticeable delays

Yeah it would be nice to avoid the delay. But I'm not sure how the proposed approach can avoid the asynchronous requests while unpaused since it still requires a round-trip to the server.

[codecov]

Codecov Report

Merging #1171 (26102a4) into model_editor (ba9403b) will decrease coverage by 0.20%.
The diff coverage is 16.32%.

❗ Current head 26102a4 differs from pull request most recent head f18db6c. Consider uploading reports for the commit f18db6c to get more accurate results

@@               Coverage Diff                @@
##           model_editor    #1171      +/-   ##
================================================
- Coverage         61.34%   61.14%   -0.21%     
================================================
  Files               262      264       +2     
  Lines             20931    21025      +94     
================================================
+ Hits              12841    12855      +14     
- Misses             8090     8170      +80     

Impacted Files	Coverage Δ
src/cmd/ModelCommandAPI.cc	0.00% <0.00%> (ø)
.../plugins/component_inspector/ComponentInspector.hh	28.57% <ø> (ø)
src/gui/plugins/component_inspector/Types.cc	0.00% <0.00%> (ø)
src/gui/plugins/component_inspector/AirPressure.cc	12.00% <12.00%> (ø)
.../plugins/component_inspector/ComponentInspector.cc	5.74% <13.63%> (+0.53%)	⬆️
src/systems/scene_broadcaster/SceneBroadcaster.cc	93.82% <100.00%> (+0.07%)	⬆️

---

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 ba9403b...f18db6c. Read the comment docs.

[nkoenig]

Got it. I think this PR is missing the concept of a read-only mode for the new widgets then, so the user doesn't edit the sensor while playing by mistake.

Yup, that is correct.

The ECM sync is a more efficient way to handle all of the parameter updating that we want to do via the GUI at the moment

I can see how that's efficient if lots of changes are queued while paused and only handled on play. This important detail is missing from this PR though.

I don't follow, what detail are you referring to?

An asynchronous service approach adds complexity and noticeable delays

Yeah it would be nice to avoid the delay. But I'm not sure how the proposed approach can avoid the asynchronous requests while unpaused since it still requires a round-trip to the server.

The problem is avoided because the GUI ECM has the most up-to-date ECM state, which is then sent to the server when the play button is pressed. The GUI registers the state change from pause to play when the server sends out a a new state message. The couple ECM from the server is the same as the ECM on the GUI. There is no visible jump.

[nkoenig]

Retargeting to the model_editor branch.

[nkoenig]

Added in a simulation state aware spin box that enables/disables based on the play/pause state of simulation.

[iche033]

did some quick tests changing the air pressure values and it's working for me as advertised. Just one minor comment