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

Get updated GUI ECM info when a user presses 'play' #1109

Merged
merged 18 commits into from
Nov 6, 2021

Conversation

adlarkin
Copy link
Contributor

@adlarkin adlarkin commented Oct 14, 2021

Signed-off-by: Ashton Larkin ashton@openrobotics.org

🎉 New feature

Part of #1106

Requires:

Summary

When simulation is paused, users might want to update the GUI's ECM directly. When simulation is resumed, the server's ECM needs to be updated to reflect any changes made in the GUI's ECM. This PR implements the server's ECM updating based on changes made in the GUI.

For a higher-level design discussion of how this functionality is implemented, see #1109 (comment).

Test it

Manual testing may need to be done in order to run a gui plugin that updates the GUI's ECM. Does anyone know how automated testing can be done for this, since it relies on GUI/Server interaction?

There are also some notes about testing the PRs new service from the command line in the PR description of gazebosim/gz-gui#306.

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

Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
@adlarkin adlarkin added the needs upstream release Blocked by a release of an upstream library label Oct 14, 2021
@github-actions github-actions bot added 🌱 garden Ignition Garden 🏯 fortress Ignition Fortress labels Oct 14, 2021
src/SimulationRunner.hh Outdated Show resolved Hide resolved
Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
@adlarkin adlarkin marked this pull request as ready for review October 22, 2021 16:38
@adlarkin adlarkin changed the title get updated GUI ECM info in world control CB Get updated GUI ECM info when a user presses 'play' Oct 26, 2021
@adlarkin adlarkin force-pushed the adlarkin/process_gui_ecm_changes branch 5 times, most recently from 5f5c74e to 04a8193 Compare October 26, 2021 17:31
Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
@adlarkin adlarkin force-pushed the adlarkin/process_gui_ecm_changes branch from 04a8193 to 6cab7b4 Compare October 26, 2021 17:37
src/gui/GuiRunner.cc Outdated Show resolved Hide resolved
Nate Koenig and others added 5 commits November 2, 2021 08:22
Signed-off-by: Nate Koenig <nate@openrobotics.org>
Signed-off-by: Nate Koenig <nate@openrobotics.org>
Signed-off-by: Nate Koenig <nate@openrobotics.org>
Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
@adlarkin
Copy link
Contributor Author

adlarkin commented Nov 3, 2021

Following the conversation in gazebosim/gz-gui#306 (comment), I have removed deprecation warnings/notes in e61eef9 since we will be supporting both the event and service options moving forward.

@adlarkin
Copy link
Contributor Author

adlarkin commented Nov 4, 2021

@nkoenig since the changes made in gazebosim/gz-gui#306 make the GUI listen to the world stats topic to get pause/play updates from the server, we might want to increase how frequently world stats messages are published to prevent slow response times when users press the play/pause GUI button. We could change the messages published per second to something like 10 or 15 (it's currently only 5): https://github.com/ignitionrobotics/ign-gazebo/blob/e61eef90a392896b85253ee971098f1eafe56162/src/SimulationRunner.cc#L672

Let me know what you think about this.

@nkoenig
Copy link
Contributor

nkoenig commented Nov 4, 2021

@nkoenig since the changes made in ignitionrobotics/ign-gui#306 make the GUI listen to the world stats topic to get pause/play updates from the server, we might want to increase how frequently world stats messages are published to prevent slow response times when users press the play/pause GUI button. We could change the messages published per second to something like 10 or 15 (it's currently only 5):

https://github.com/ignitionrobotics/ign-gazebo/blob/e61eef90a392896b85253ee971098f1eafe56162/src/SimulationRunner.cc#L672

Let me know what you think about this.

Can you open a separate PR with this change?

@adlarkin
Copy link
Contributor Author

adlarkin commented Nov 4, 2021

Can you open a separate PR with this change?

👍 see #1163

Nate Koenig added 2 commits November 5, 2021 12:42
@nkoenig
Copy link
Contributor

nkoenig commented Nov 5, 2021

@scpeters , it looks like there are problems with homebrew that you're aware of. Is it okay to merge this, or should I wait for a homebrew fix?

@scpeters
Copy link
Member

scpeters commented Nov 6, 2021

@scpeters , it looks like there are problems with homebrew that you're aware of. Is it okay to merge this, or should I wait for a homebrew fix?

I just merged the bottle for now; we can fix it later

I'll restart the homebrew job, which should have a better chance now

@nkoenig
Copy link
Contributor

nkoenig commented Nov 6, 2021

I just merged the bottle for now; we can fix it later

I'll restart the homebrew job, which should have a better chance now

Thanks.

@nkoenig nkoenig merged commit 827fc3f into ign-gazebo6 Nov 6, 2021
@nkoenig nkoenig deleted the adlarkin/process_gui_ecm_changes branch November 6, 2021 04:20
{
std::lock_guard<std::mutex> lock(this->msgBufferMutex);

// update the server ECM if the request contains SerializedState information
if (_req.has_state())
this->entityCompMgr.SetState(_req.state());
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry to join the party late, but is this thread-safe? It's dangerous to update the ECM in a transport callback. I think we should define a clear moment in the update loop when the ECM is updated with the new state; maybe before PreUpdate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing thread safety out - I totally missed this the first time around 🤦 @nkoenig is going to open another PR with a fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏯 fortress Ignition Fortress 🌱 garden Ignition Garden needs upstream release Blocked by a release of an upstream library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants