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

Properly add new models into the scenegraph #212

Merged
merged 3 commits into from
Jun 18, 2020

Conversation

nkoenig
Copy link
Contributor

@nkoenig nkoenig commented Jun 18, 2020

The edge check logic was incorrect because it was checking to see if an edge ID already existed in the sceneGraph when it should be checking if a connected between the two vertices already existed.

Without this fix, when a model is spawned it won't be added into the graph properly.

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

@nkoenig nkoenig requested a review from chapulina as a code owner June 18, 2020 12:56
@github-actions github-actions bot added the 📜 blueprint Ignition Blueprint label Jun 18, 2020
@codecov
Copy link

codecov bot commented Jun 18, 2020

Codecov Report

Merging #212 into ign-gazebo2 will increase coverage by 0.08%.
The diff coverage is 0.00%.

Impacted file tree graph

@@               Coverage Diff               @@
##           ign-gazebo2     #212      +/-   ##
===============================================
+ Coverage        62.25%   62.34%   +0.08%     
===============================================
  Files              123      123              
  Lines             6101     6102       +1     
===============================================
+ Hits              3798     3804       +6     
+ Misses            2303     2298       -5     
Impacted Files Coverage Δ
src/systems/scene_broadcaster/SceneBroadcaster.cc 36.24% <0.00%> (-0.12%) ⬇️
src/SimulationRunner.cc 88.77% <0.00%> (+1.20%) ⬆️

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 e42ad43...0aa28ae. Read the comment docs.

@nkoenig nkoenig requested a review from caguero June 18, 2020 15:45
@chapulina
Copy link
Contributor

I saw we have a test for when entities are deleted (even though the comment says otherwise):

https://github.com/ignitionrobotics/ign-gazebo/blob/e42ad43901b57a359ea16f030a88d4a7cdb32ce7/test/integration/scene_broadcaster_system.cc#L221-L222

It would be good to also test when entities are added, so we're sure this continues working.

Copy link
Contributor

@caguero caguero left a comment

Choose a reason for hiding this comment

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

Makes sense to me.

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

nkoenig commented Jun 18, 2020

I saw we have a test for when entities are deleted (even though the comment says otherwise):

https://github.com/ignitionrobotics/ign-gazebo/blob/e42ad43901b57a359ea16f030a88d4a7cdb32ce7/test/integration/scene_broadcaster_system.cc#L221-L222

It would be good to also test when entities are added, so we're sure this continues working.

Test added: 0aa28ae

@nkoenig nkoenig merged commit 77406ae into ign-gazebo2 Jun 18, 2020
@nkoenig nkoenig deleted the fix_scenebroadcaster_spawn branch June 18, 2020 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📜 blueprint Ignition Blueprint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants