Get updated GUI ECM info when a user presses 'play' #1109
Conversation
Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
adlarkin
added
the
needs upstream release
Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
adlarkin
force-pushed
the
adlarkin/process_gui_ecm_changes
branch
from
6c69236
to
051db64
Compare
Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
adlarkin
changed the title
adlarkin
force-pushed
the
adlarkin/process_gui_ecm_changes
branch
5 times, most recently
from
5f5c74e
to
04a8193
Compare
Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
adlarkin
force-pushed
the
adlarkin/process_gui_ecm_changes
branch
from
04a8193
to
6cab7b4
Compare
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>
|
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. |
|
@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. |
Can you open a separate PR with this change? |
👍 see #1163 |
Signed-off-by: Nate Koenig <nate@openrobotics.org>
|
@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 |
Thanks. |
| { | ||
| 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()); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Thanks for pointing thread safety out - I totally missed this the first time around 🤦 @nkoenig is going to open another PR with a fix.
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
codecheckpassed (See contributing)Note to maintainers: Remember to use Squash-Merge