-
Notifications
You must be signed in to change notification settings - Fork 49
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 WorldControlState msg type #190
Conversation
Signed-off-by: Ashton Larkin <[email protected]>
cc1a19e
to
59c112b
Compare
Codecov Report
@@ Coverage Diff @@
## ign-msgs8 #190 +/- ##
==========================================
Coverage 84.56% 84.56%
==========================================
Files 7 7
Lines 855 855
==========================================
Hits 723 723
Misses 132 132 Continue to review full report at Codecov.
|
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 looks ok to me as is, but I'd like the full implementation to be complete and iterated on before merging. This is targeting a release branch, so once it's merged / released, we can't change it.
Leaving a "request changes" to signify this.
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.
Minor documentation comments.
Targeting |
Signed-off-by: Ashton Larkin <[email protected]>
All review feedback has been addressed. As mentioned previously, if this goes into Fortress, the message types can't be changed once they're merged. So, I will hold off on merging this until gazebosim/gz-gui#306 and gazebosim/gz-sim#1109 are finalized. |
Signed-off-by: Ashton Larkin <[email protected]>
Signed-off-by: Ashton Larkin <[email protected]>
Following the discussion I had with @nkoenig in gazebosim/gz-sim#1109 (comment), I've updated this PR to only create a |
I've tested this with gazebosim/gz-gui#306 and it seems to work well. I'll merge and get a release out the door so that we can move forward with downstream PRs. |
Signed-off-by: Ashton Larkin [email protected]
🎉 New feature
Part of gazebosim/gz-sim#1106
Summary
As discussed in gazebosim/design#8 (comment), if the GUI's ECM is updated while simulation is paused, the GUI should share any ECM updates that occurred with the server when simulation is resumed. This PR creates a
WorldControlState
message to accomplish this. This message type contains bothWorldControl
and aSerializedState
(theSerializedState
can be used to contain ECM information, andWorldControl
is used for the same purpose - play/pause/step).Test it
This message can be tested with gazebosim/gz-sim#1109.
Checklist
codecheck
passed (See contributing)Note to maintainers: Remember to use Squash-Merge