-
Notifications
You must be signed in to change notification settings - Fork 41
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
Support emitting an event on play/pause/step #306
Conversation
Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
This should now be ready for initial review. I have a question for whoever reviews this: #306 (comment) I also left some notes in the |
Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
0eb1905
to
915a1a6
Compare
@nkoenig I have updated this PR to reflect the new design that was discussed in gazebosim/gz-sim#1109 (comment). |
Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
@chapulina the tests for the |
…ior by default Signed-off-by: Nate Koenig <nate@openrobotics.org>
I think the |
Are you referring to what I'm seeing in gazebosim/gz-sim#1109 (comment)? |
Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
This should be fixed now - see gazebosim/gz-sim#1109 (comment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do tests need to be added for the event approach as well?
That would be ideal
If so, I'm not sure how this would be tested since the receiving end of the service is in gazebosim/gz-sim#1109.
How about listening to the event on the test and check that it is called? Maybe you can create a simple class that installs a filter 🤔
Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me. I'd like to switch out the header stepping flag in Garden. Here is the start: gazebosim/gz-msgs#199.
@adlarkin , @chapulina would it be possible to get this in today so that I can make a release and unblock gazebosim/gz-sim#1157? |
That's okay with me - once @chapulina takes a look and no more changes are requested, feel free to merge. I am seeing a new issue in Windows CI though that's related to the tests I added:
Does anyone know how to fix this? |
Signed-off-by: Nate Koenig <nate@openrobotics.org>
All of your tests were set to run only on linux, so I disable their compilation on windows in 80a64f4. |
Alright, CI is green-enough. There is a flaky test that has failed on Focal, but has passed in other iterations. |
@chapulina , I'm going to merge this by 10am today unless you stop me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just have minor comments, no blockers
include/ignition/gui/GuiEvents.hh
Outdated
@@ -27,6 +27,7 @@ | |||
#include <ignition/common/MouseEvent.hh> | |||
#include <ignition/math/Vector2.hh> | |||
#include <ignition/math/Vector3.hh> | |||
#include <ignition/msgs.hh> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's preferable to include just the message that's needed, this helps with compile times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 8e7cb8f
this->dataPtr->pause = false; | ||
this->dataPtr->node.Request(this->dataPtr->controlService, req, cb); | ||
if (this->dataPtr->useEvent) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block from 280 to 294 is repeated 3 times in different functions, consider moving it to a separate function to reduce duplication
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 8e7cb8f
/// \brief Helper class for testing listening to events emitted by the | ||
/// WorldControl plugin. This is used for testing the event behavior of | ||
/// the WorldControl plugin. | ||
class IGNITION_GUI_VISIBLE WorldControlEventListener : public QObject |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work putting the tests together! Ideally we wouldn't include this as part of the plugin itself, since it's only used for testing. But I think this can be addressed later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 8e7cb8f
WorldControl_TEST.cc | ||
) | ||
|
||
if (NOT MSVC) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, why did you need to separate Windows here? We're already disabling the test on Windows in the test's body.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because Windows would still compile the testing framework and produce compile time errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. It was compiling before. Not sure what causes the compiler error here, but I think it's ok to address this later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a follow-up, yes,the tests themselves ignore windows. For example: https://github.com/ignitionrobotics/ign-gui/blob/ignition-gui6_6.0.0/src/plugins/world_control/WorldControl_TEST.cc#L66
However, the new helper test class that was added for checking events (#306 (comment)) has to be added as a QT header, or else there will be linking errors. So, it seemed like this new QT header is what was causing issues on windows, and @nkoenig made a patch in #306 (comment) to prevent compiling the helper test class header on windows. I'm not sure if there's a better way to handle this or not, but hopefully that provides some more context/insights about this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried a few different options to make the header compile on windows, and gave up.
Signed-off-by: Nate Koenig <nate@openrobotics.org>
* share updated GUI ECM info with server on play Signed-off-by: Ashton Larkin <ashton@openrobotics.org> * clarify docs and fix service request type Signed-off-by: Ashton Larkin <ashton@openrobotics.org> * send event on play/pause/step Signed-off-by: Ashton Larkin <ashton@openrobotics.org> * support service and event Signed-off-by: Ashton Larkin <ashton@openrobotics.org> * fix tests Signed-off-by: Ashton Larkin <ashton@openrobotics.org> * Flip the default settng of usEvent in order to maintain current behavior by default Signed-off-by: Nate Koenig <nate@openrobotics.org> * ignore stats msgs triggered by a step Signed-off-by: Ashton Larkin <ashton@openrobotics.org> * explicit constructor for new gui event Signed-off-by: Ashton Larkin <ashton@openrobotics.org> * remove deprecation notes (support both event and service) Signed-off-by: Ashton Larkin <ashton@openrobotics.org> * test event behavior Signed-off-by: Ashton Larkin <ashton@openrobotics.org> * add missing helper test class Signed-off-by: Ashton Larkin <ashton@openrobotics.org> * Disable tests on windows Signed-off-by: Nate Koenig <nate@openrobotics.org> * Address PR feedback Signed-off-by: Nate Koenig <nate@openrobotics.org> Co-authored-by: Nate Koenig <nate@openrobotics.org> Co-authored-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Ashton Larkin ashton@openrobotics.org
🎉 New feature
Part of gazebosim/gz-sim#1106
Requires gazebosim/gz-msgs#190
Summary
If the GUI's ECM is modified directly while simulation is paused, the GUI needs to tell the server how the ECM changed. The approach taken is to have the
WorldControl
service called inign-gazebo
'sGuiRunner
instead ofign-gui
'sWorldControl
plugin. When play/pause/step is pressed in the GUI, an event is emitted that contains theWorldControl
information. This event is processed byign-gazebo
'sGuiRunner
. TheGuiRunner
will then take the information from this event, combine it with the GUI's ECM state, and call theWorldControl
service that was being called inign-gui
before this PR.ign-gui
'sWorldControl
will listen to the world'sstats
topic for confirmation that the emittedWorldControl
event was processed by the server.It's also worth noting that the client/server communication that exists before this PR (direct service call) is still supported - see the
Test It
section below.Test it
note: this PR needs to be tested with gazebosim/gz-sim#1109
Run a gazebo world and try to play/pause/step from the GUI. You should see proper update behavior. You can also try playing/pausing from the command line to verify that those updates work properly as well.
To play from the command line with the original service (make sure you update
/world/shapes/control
to the service name that matches your world):To pause from the command line with the original service (make sure you update
/world/shapes/control
to the service name that matches your world):To play from the command line with the new service that's called by the emitted event (make sure you update
/world/shapes/control/state
to the service name that matches your world):To pause from the command line with the new service that's called by the emitted event (make sure you update
/world/shapes/control/state
to the service name that matches your world):Testing server/client communication through either the new event approach or traditional service approach can be toggled by adding the following to the
WorldControl
plugin's XML (true
for event,false
for service - see an example here):Checklist
codecheck
passed (See contributing)Note to maintainers: Remember to use Squash-Merge