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

Send state message when components are removed #1235

Merged
merged 5 commits into from
Dec 8, 2021

Conversation

nkoenig
Copy link
Contributor

@nkoenig nkoenig commented Dec 6, 2021

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

🦟 Bug fix

Summary

The WorldCmdPose was being transmitted to the GUI and back to the Server, resulting in bad behavior. See here.

This PR changes the SceneBroadcaster to send a new state message when components are removed.

Before I write tests, I'd like to get some feedback on this approach.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (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

@nkoenig nkoenig mentioned this pull request Dec 6, 2021
8 tasks
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Dec 6, 2021
@codecov
Copy link

codecov bot commented Dec 6, 2021

Codecov Report

Merging #1235 (bf374c9) into ign-gazebo6 (4152bdf) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@               Coverage Diff               @@
##           ign-gazebo6    #1235      +/-   ##
===============================================
+ Coverage        62.23%   62.26%   +0.03%     
===============================================
  Files              275      275              
  Lines            22710    22713       +3     
===============================================
+ Hits             14133    14142       +9     
+ Misses            8577     8571       -6     
Impacted Files Coverage Δ
include/ignition/gazebo/EntityComponentManager.hh 100.00% <ø> (ø)
src/EntityComponentManager.cc 87.07% <100.00%> (+0.04%) ⬆️
src/systems/scene_broadcaster/SceneBroadcaster.cc 93.75% <100.00%> (ø)
src/SimulationRunner.cc 93.62% <0.00%> (+0.98%) ⬆️

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 4152bdf...bf374c9. Read the comment docs.

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.

Nice! We just need a unit test.

@nkoenig
Copy link
Contributor Author

nkoenig commented Dec 6, 2021

Nice! We just need a unit test.

Just checking that you don't think there are unforeseen side effects of this?

@chapulina
Copy link
Contributor

Just checking that you don't think there are unforeseen side effects of this?

I can't think of any unwanted effects, the worst I can think of is that we're publishing more state messages, but that should be negligible if there aren't components being removed every iteration.

@azeey
Copy link
Contributor

azeey commented Dec 7, 2021

We have been preferring to zero out components instead of removing them for performance reasons, so I don't think we have any components that get removed every iteration. So I think this approach might work as a work around. My proposal for a more long term solution is to have a scheme where components are requested for removal, just like entities, and their removal happens after all the *Update phases have completed. I think this would remove the need to keep WorldPoseCmd around for an extra iteration.

Signed-off-by: Nate Koenig <[email protected]>
@nkoenig
Copy link
Contributor Author

nkoenig commented Dec 7, 2021

Tests have been added in b2ad027

test/integration/scene_broadcaster_system.cc Outdated Show resolved Hide resolved
test/integration/scene_broadcaster_system.cc Show resolved Hide resolved
@nkoenig nkoenig merged commit 17f63c4 into ign-gazebo6 Dec 8, 2021
@nkoenig nkoenig deleted the has_removed_components branch December 8, 2021 04:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏯 fortress Ignition Fortress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants