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.

Sign up for GitHub

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

Jump to bottom

SceneBroadcaster: only send changed state information for change events #1392

Merged
merged 6 commits into from
Mar 28, 2022
Merged

SceneBroadcaster: only send changed state information for change events #1392

merged 6 commits into from
Mar 28, 2022

Conversation

adlarkin
Copy link
Contributor

@adlarkin adlarkin commented Mar 15, 2022 •
edited
Loading

Signed-off-by: Ashton Larkin [email protected]

🦟 Bug fix

Fixes #1358

Summary

The SceneBroadcaster is sending the full ECM state whenever there is a change event, resulting in a performance hit because of excessive serialization/de-serialization between the server and GUI (by sharing the full state, entities/components that have not changed are still serialized, which is unnecessary). This PR takes the approach proposed in #1358 (comment) to send only the changed state from server -> GUI whenever there is a change event.

I have added test coverage to the SceneBroadcaster integration test to ensure that the ECM that receives the changed state (which in theory would be the GUI ECM) is properly updated to reflect the changes without losing unchanged state information. I also took some time to look at how EntityComponentManager::SetState is implemented (this is how the GUI's ECM is updated whenever it receives state information from the server ECM), and it looks to me like there shouldn't be any issues with only sending the changed state instead of the full state for change events (EntityComponentManager::SetState doesn't reset the ECM's current state; instead, it "applies" the incoming state to the current ECM state). It would probably be worth testing some GUI plugins before merging this to make sure that this change doesn't produce unexpected behavior changes.

Checklist

  • Signed all commits for DCO
  • Added tests
  • 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 and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Mar 15, 2022
@adlarkin adlarkin mentioned this pull request Mar 15, 2022
Closed
@codecov
Copy link

codecov bot commented Mar 15, 2022 •
edited
Loading

Codecov Report

Merging #1392 (72abf5b) into ign-gazebo6 (9b07b24) will increase coverage by 0.03%.
The diff coverage is 100.00%.

❗ Current head 72abf5b differs from pull request most recent head da8a393. Consider uploading reports for the commit da8a393 to get more accurate results

@@               Coverage Diff               @@
##           ign-gazebo6    #1392      +/-   ##
===============================================
+ Coverage        62.99%   63.03%   +0.03%     
===============================================
  Files              301      301              
  Lines            24261    24262       +1     
===============================================
+ Hits             15283    15293      +10     
+ Misses            8978     8969       -9
Impacted Files Coverage Δ
src/EntityComponentManager.cc 89.36% <100.00%> (+1.08%) ⬆️
src/systems/scene_broadcaster/SceneBroadcaster.cc 92.85% <100.00%> (+0.03%) ⬆️

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 9b07b24...da8a393. Read the comment docs.

chapulina
chapulina reviewed Mar 17, 2022
View reviewed changes
Copy link
Contributor

@chapulina chapulina 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'd just like to see more testing with changed component values.

It's possible that this will break current users who aren't setting the changed state correctly.

test/integration/scene_broadcaster_system.cc Show resolved Hide resolved
@ahcorde ahcorde mentioned this pull request Mar 21, 2022
Closed
@srmainwaring
Copy link
Contributor

@adlarkin great fix thanks. I've tested this on macOS / Metal by cherry-picking into main and it completely solves an issue I had with running a set_pose user command at a high update rate (#1403).

@adlarkin adlarkin requested a review from chapulina March 22, 2022 19:25
@adlarkin
Copy link
Contributor Author

It looks like the last part of the new test I added is a little flaky on github actions. I believe it's due to slower machines on GitHub actions. I tried to increase the wait times for messages to be received via ign-transport in e3c38fe, and it seemed to fix actions on focal, but not on bionic. Jenkins CI is passing on Ubuntu though, and the tests pass locally on my machine.

chapulina
chapulina approved these changes Mar 25, 2022
View reviewed changes
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Thanks, the tests look good to me!

It looks like the last part of the new test I added is a little flaky on github actions.

I tried to make the scene broadcaster publish more often in e8dc431. Let's see how it goes, feel free to merge if Bionic passes.

@adlarkin
Copy link
Contributor Author

GitHub actions is green, and ubuntu CI is green. Thanks, @chapulina - merging! 🎉

@adlarkin adlarkin merged commit 0996fe0 into ign-gazebo6 Mar 28, 2022
@adlarkin adlarkin deleted the adlarkin/serialization_performance branch March 28, 2022 17:28
srmainwaring pushed a commit to srmainwaring/gz-sim that referenced this pull request Mar 28, 2022
@adlarkin @chapulina @srmainwaring
SceneBroadcaster: only send changed state information for change even…
ec5a17b 
…ts (gazebosim#1392)

Signed-off-by: Ashton Larkin <[email protected]>

Co-authored-by: Louise Poubel <[email protected]>
chapulina added a commit that referenced this pull request Apr 13, 2022
@chapulina @ahcorde
@j-rivero @adityapande-1995 @nkoenig @iche033 @adlarkin @azeey @ivanpauno @WilliamLewww @marcoag @mjcarroll
6 ➡️ 7 (main) (partial) (#1437)
c660c87 
* GzSceneManager: Prevent crash 💥 when inserted from menu (#1371)

Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: ahcorde <[email protected]>

Co-authored-by: ahcorde <[email protected]>

* Prepare version 6.7.0 (#1373)

* Prepare version 6.7.0

Signed-off-by: Jose Luis Rivero <[email protected]>

* Populate GUI plugins that are empty (#1375)

Signed-off-by: Louise Poubel <[email protected]>

* Fix visualization python tutorial (#1377)

Signed-off-by: ahcorde <[email protected]>

* Add xyz and rpy offset to published odometry pose (#1341)

* Added xyz and rpy offset to published pose

Signed-off-by: Aditya <[email protected]>

* Added headless rendering tutorial (#1386)

* Added headless rendering tutorial

Signed-off-by: Nate Koenig <[email protected]>

* Update tutorials/headless_rendering.md

Co-authored-by: Louise Poubel <[email protected]>
Signed-off-by: Nate Koenig <[email protected]>

* Update tutorials/headless_rendering.md

Co-authored-by: Louise Poubel <[email protected]>
Signed-off-by: Nate Koenig <[email protected]>

* Mention ogre2

Signed-off-by: Nate Koenig <[email protected]>

* Added note about software rendering

Signed-off-by: Nate Koenig <[email protected]>

* add 'on linux systems'

Signed-off-by: Nate Koenig <[email protected]>

* Add xyz and rpy offset to published odometry pose (#1341)

* Added xyz and rpy offset to published pose

Signed-off-by: Aditya <[email protected]>
Signed-off-by: Nate Koenig <[email protected]>

Co-authored-by: Nate Koenig <[email protected]>
Co-authored-by: Louise Poubel <[email protected]>
Co-authored-by: Aditya Pande <[email protected]>

* Allow to turn on/off lights (#1343)

Signed-off-by: ahcorde <[email protected]>
Co-authored-by: Louise Poubel <[email protected]>

* Add gazebo Entity id to rendering sensor's user data (#1381)

Adds the gazebo::Entity id to a rendering::Sensor's UserData object. The main use case is so that we can get back the corresponding gazebo Entity in the rendering thread when processing sensors.

Signed-off-by: Ian Chen <[email protected]>

Co-authored-by: Nate Koenig <[email protected]>
Co-authored-by: Alejandro Hernández Cordero <[email protected]>

* Don't mark entities with a ComponentState::NoChange component as modified (#1391)

Signed-off-by: Ashton Larkin <[email protected]>

* Disable ModelCommandAPI_TEST.RgbdCameraSensor on macOS (#1397)

Disabling test since it's very flaky.

Signed-off-by: Addisu Z. Taddese <[email protected]>

* Disable PeerTracker.PeerTrackerStale on macOS (#1398)

Signed-off-by: Addisu Z. Taddese <[email protected]>

* Toggle Light visuals (#1387)

Signed-off-by: ahcorde <[email protected]>

* Prevent hanging when world has only non-world plugins (#1383)

Signed-off-by: Louise Poubel <[email protected]>

Co-authored-by: Alejandro Hernández Cordero <[email protected]>

* Component inspector: refactor Pose3d C++ code into a separate class (#1400)

Signed-off-by: Louise Poubel <[email protected]>

* add initial_position param to joint controller system (#1406)

Similar to the <initial_position> parameter in the JointTrajectoryController system, this PR adds an <initial_position> parameter to the JointPositionController to let users specifies the initial target pos for a joint.

Signed-off-by: Ian Chen <[email protected]>

* Fix JointStatePublisher topic name for nested models (#1405)

The joint-state-publisher system currently assumes it's always attached to the top level model and hence generates an incorrect topic name for nested models. This PR updates it to generate the correct topic name.

Signed-off-by: Ian Chen <[email protected]>

* Added user command to set multiple entities (#1394)

Signed-off-by: ahcorde <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>

Co-authored-by: Louise Poubel <[email protected]>

* Disable tests that are expected to fail on Windows (#1408)

Signed-off-by: Louise Poubel <[email protected]>

* Fortress: Install Ogre 2.2, simplify docker (#1395)

Signed-off-by: Louise Poubel <[email protected]>

* SceneBroadcaster: only send changed state information for change events (#1392)

Signed-off-by: Ashton Larkin <[email protected]>

Co-authored-by: Louise Poubel <[email protected]>

* Add wheel slip user command (#1241)

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

* Distortion camera integration test (#1374)

Signed-off-by: William Lew <[email protected]>
Co-authored-by: Alejandro Hernández Cordero <[email protected]>
Co-authored-by: Louise Poubel <[email protected]>

* Add the Model Photo Shoot system, port of Modelpropshop plugin from Gazebo classic (#1331)

This system can be used to generate thumbnails of models. In comparison to the Modelpropshop plugin in Gazebo classic, this adds the ability to generate thumbnails after randomizing the joint positions of the model.

Signed-off-by: Marco A. Gutierrez <[email protected]>
Signed-off-by: Addisu Z. Taddese <[email protected]>

* Referring to Fuel assets within a heightmap (#1419)

Signed-off-by: Louise Poubel <[email protected]>

* Disable sensors in sensors system when battery is drained (#1385)

Added a new parameter <disable_on_drained_battery> to the sensors system. If set to true, sensors will be disabled if the model is out of battery.

It listens to the battery state of charge from the battery plugin, and if the charge reaches zero, all child sensors and nested sensors are set to be inactive.

Signed-off-by: Ian Chen <[email protected]>

* 🏁🎈 5.4.0 (#1420)

Signed-off-by: Louise Poubel <[email protected]>

* ServerConfig accepts an sdf::Root DOM object (#1333)

Signed-off-by: Nate Koenig <[email protected]>

Co-authored-by: Nate Koenig <[email protected]>
Co-authored-by: Michael Carroll <[email protected]>
Co-authored-by: Alejandro Hernández Cordero <[email protected]>

* Preparing 6.8.0 release (#1425)

* Prepareing 6.8.0 release

Signed-off-by: Jose Luis Rivero <[email protected]>

* Update changelog after merging forward port

Signed-off-by: Jose Luis Rivero <[email protected]>

* Add Gaussian noise to Odometry Publisher (#1393)

* Added gaussian noise and odometry with covariance publisher

Signed-off-by: Aditya <[email protected]>

Co-authored-by: Louise Poubel <[email protected]>

* Fix deprecation warnings for ModelPhotoShoot (#1437)

Signed-off-by: Louise Poubel <[email protected]>

Co-authored-by: ahcorde <[email protected]>
Co-authored-by: Jose Luis Rivero <[email protected]>
Co-authored-by: Aditya Pande <[email protected]>
Co-authored-by: Nate Koenig <[email protected]>
Co-authored-by: Nate Koenig <[email protected]>
Co-authored-by: Ian Chen <[email protected]>
Co-authored-by: Ashton Larkin <[email protected]>
Co-authored-by: Addisu Z. Taddese <[email protected]>
Co-authored-by: Ivan Santiago Paunovic <[email protected]>
Co-authored-by: William Lew <[email protected]>
Co-authored-by: Marco A. Gutiérrez <[email protected]>
Co-authored-by: Michael Carroll <[email protected]>
@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-04-13-fortress-edifice/1367/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chapulina chapulina chapulina approved these changes

@azeey azeey Awaiting requested review from azeey

@mjcarroll mjcarroll Awaiting requested review from mjcarroll mjcarroll is a code owner

@ahcorde ahcorde Awaiting requested review from ahcorde

Assignees
No one assigned
Labels
🏯 fortress Ignition Fortress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Delay and time jump in object position update when using the transform tool
4 participants
@adlarkin @srmainwaring @osrf-triage @chapulina