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

Add joint position controller GUI, also enable tests for GUI plugins #534

Merged
merged 7 commits into from
Mar 17, 2021

Conversation

chapulina
Copy link
Contributor

You can try it out with this demo world:

ign gazebo -v 4 "https://fuel.ignitionrobotics.org/1.0/OpenRobotics/worlds/NAO joint control"

nao_joint_control4

Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
@chapulina chapulina added GUI Gazebo's graphical interface (not pure Ignition GUI) tests Broken or missing tests / testing infra labels Jan 8, 2021
@github-actions github-actions bot added the 🏰 citadel Ignition Citadel label Jan 8, 2021
@chapulina chapulina linked an issue Jan 8, 2021 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Jan 8, 2021

Codecov Report

Merging #534 (08904d0) into ign-gazebo3 (6603455) will decrease coverage by 0.15%.
The diff coverage is 61.79%.

Impacted file tree graph

@@               Coverage Diff               @@
##           ign-gazebo3     #534      +/-   ##
===============================================
- Coverage        77.87%   77.71%   -0.16%     
===============================================
  Files              212      214       +2     
  Lines            11719    11897     +178     
===============================================
+ Hits              9126     9246     +120     
- Misses            2593     2651      +58     
Impacted Files Coverage Δ
...int_position_controller/JointPositionController.cc 61.14% <61.14%> (ø)
...int_position_controller/JointPositionController.hh 100.00% <100.00%> (ø)
src/systems/physics/Physics.cc 70.16% <100.00%> (+0.03%) ⬆️
src/SimulationRunner.cc 92.27% <0.00%> (-1.13%) ⬇️
src/gui/GuiRunner.cc 98.36% <0.00%> (+26.22%) ⬆️

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 6603455...08904d0. Read the comment docs.

Copy link
Contributor

@nkoenig nkoenig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to have the widget consume the available space? Not sure if this is related to this plugin, or ign-gui in general. See image:

joint_controller_fill

I also tried this with the double pendulum, and I wasn't able to control the joints. I assume this is because the pendulum model doesn't have the joint position controller plugin. Is it possible for this widget to know if the plugin has been specified, and display some kind of warning when the plugin is missing?

@chapulina
Copy link
Contributor Author

Is it possible to have the widget consume the available space? Not sure if this is related to this plugin, or ign-gui in general.

Yeah that annoys me too. That's on ign-gui's side. I'll take a stab at this soon, and if I can't fix it, I'll ticket an issue.

Is it possible for this widget to know if the plugin has been specified, and display some kind of warning when the plugin is missing?

At the moment I think that would be difficult. Some thoughts:

  • Once System inspector #191 is addressed, the GUI should have a way to tell if an entity has certain plugins attached. Once that information is available, this plugin could use it to determine if the joints have the correct controllers.
  • Once Insert system plugins from the GUI #190 is addressed, we should have a mechanism to add system plugins on demand from the GUI. In this case, the joint controller could be spawned as needed.
  • One temporary solution could be to check if the required topics have subscribers. But I think that's not possible at the moment due to Topic info command line doesn't list subscribers gz-transport#39.

@chapulina chapulina added the close the gap Features from Gazebo-classic label Jan 26, 2021
@chapulina
Copy link
Contributor Author

Is it possible to have the widget consume the available space? Not sure if this is related to this plugin, or ign-gui in general.

Yeah that annoys me too. That's on ign-gui's side. I'll take a stab at this soon, and if I can't fix it, I'll ticket an issue.

By "soon" I meant in 2 months 😬 Here we go: gazebosim/gz-gui#194

@chapulina
Copy link
Contributor Author

display some kind of warning when the plugin is missing?

For now, I added a help popup explaining to users that they should add the joint controllers:

nao_help

Copy link
Contributor

@jennuine jennuine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I left a minor suggestion for the spinbox.

What do you think about having a reset button that resets all the joints back to default?

Comment on lines 80 to 82
onEditingFinished: {
joint.value = spin.value
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
onEditingFinished: {
joint.value = spin.value
}
stepSize: 0.1
onValueChanged: {
joint.value = spin.value
}

Currently, the stepSize is 1 but I think 0.1 would be a better increment. Also, I think using onValueChanged instead of onEditingFinished is more user friendly, the user doesn't have to hit enter every time they want to update a joint when using the spinbox

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both are very good suggestions. I started with both of them, but I had to revert to using onEditingFinished when I worked on the reset button.

While working on reset, I noticed that the values on the GUI were not being updated to reflect the actual state of the robot. When I fixed that, I realized I couldn't use onValueChanged, because that signal doesn't differentiate the value being changed by the user, or from the ECM. So what was happening was that the intermediate state of a joint moving would be sent as a new command, and the joint would tend to return to its original position in a negative feedback loop. It's hard to explain with words, see what happened with onValueChanged:

jointfeedback

This is all on 08904d0, let me know what you think.

@chapulina chapulina requested a review from azeey as a code owner March 17, 2021 01:57
@chapulina
Copy link
Contributor Author

What do you think about having a reset button that resets all the joints back to default?

Good idea, added on 08904d0, and also fixed updating the GUI according to the robot's real state. Let me know what you think:

reset

Copy link
Contributor

@jennuine jennuine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 😄

@chapulina chapulina merged commit 29683a8 into ign-gazebo3 Mar 17, 2021
@chapulina chapulina deleted the chapulina/3/joint_gui branch March 17, 2021 16:44
@mjcarroll
Copy link
Contributor

This caused a test failure in CI: https://build.osrfoundation.org/job/ignition_gazebo-ci-ign-gazebo4-bionic-amd64/54/testReport/(root)/JointPositionControllerGui/Load/

/home/jenkins/workspace/ignition_gazebo-ci-ign-gazebo4-bionic-amd64/ign-gazebo/src/gui/plugins/joint_position_controller/JointPositionController_TEST.cc:90
Expected equality of these values:
  plugin->ModelName()
    Which is: {}
  QString("No model selected")
    Which is: { 2-byte object <4E-00>, 2-byte object <6F-00>, 2-byte object <20-00>, 2-byte object <6D-00>, 2-byte object <6F-00>, 2-byte object <64-00>, 2-byte object <65-00>, 2-byte object <6C-00>, 2-byte object <20-00>, 2-byte object <73-00>, 2-byte object <65-00>, 2-byte object <6C-00>, 2-byte object <65-00>, 2-byte object <63-00>, 2-byte object <74-00>, 2-byte object <65-00>, 2-byte object <64-00> }
     ```

@chapulina chapulina mentioned this pull request Mar 19, 2021
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel close the gap Features from Gazebo-classic GUI Gazebo's graphical interface (not pure Ignition GUI) tests Broken or missing tests / testing infra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Joint controller GUI
4 participants