Fix crash on GUI entity removal with levels

[adlarkin]

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

🦟 Bug fix

Fixes #856 (review)

Summary

After debugging the segfault pointed out in the link above, it seems like the levels system still tries to add components to entities even after they're removed (I'm not 100% certain if levels is to blame for this, but when I removed an entity in the shapes.sdf world, I did not see this behavior). This caused a segfault in #856 because components are not created if an entity doesn't exist, but the non existing entity (and component) were still marked as modified. This "mismatch" in data is problematic when EntityComponentManagerPrivate::CalculateStateThreadLoad is called in EntityComponentmanager::State, so, this PR adds a check when calling EntityComponentManager::CreateComponentImplementation to make sure that the entity exists.

While the bug fix here ended up being a simple one, I found something interesting when comparing #856 to the main branch. In the main branch, components are always created, regardless of whether an entity exists or not (I was also able to confirm this by testing the levels world that caused a segfault in #856 - as you can see, there's no checking for if the entity exists): https://github.com/ignitionrobotics/ign-gazebo/blob/c07b49c51caae6da1566c383d2b6ff91f4cb8097/src/EntityComponentManager.cc#L530-L566

So, on the main branch, we don't see this segfault since both 1) the component is created, and 2) the (non existent) entity and the component are marked as modified. This actually seems like incorrect behavior to me, and I believe that we should fix main (and other released versions as well) to prevent creating components for entities that do not exist. @iche033, what do you think?

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

[iche033]

I think the diff_drive plugin is creating various vel cmd components on every PreUpdate for entities that no longer exist hence causing the crash. We should probably fix the diff drive plugin too.

Maybe print an error in CreateComponentImplementation if users are trying to create components for entities that do not exist?

I believe that we should fix main (and other released versions as well) to prevent creating components for entities that do not exist. @iche033, what do you think?

yeah that sounds good to me.

[adlarkin]

Maybe print an error in CreateComponentImplementation if users are trying to create components for entities that do not exist?

Done in fd46a05

---

I have also updated the diff_drive plugin to skip removed entities in #927. Once that change gets forward-ported to main, we should be able to merge this PR (I have started the forward-port process in #933).

[adlarkin]

We should probably fix the diff drive plugin too.

@iche033, the diff drive plugin should now be fixed in all released versions (#927 has been forward-ported from Citadel to Fortress). Would you mind testing the bug you found in #856 (review) one last time before I merge it?

[iche033]

CI caught a compilation error and I can reproduce it locally. Looks like it could be introduced due to recent changes in the target branch

EntityComponentManager_TEST.cc:233:3:   required from here
/home/osrf/code/ign_f_ws2/src/ign-gazebo/test/gtest/include/gtest/gtest.h:1527:11: error: no match for ‘operator==’ (operand types are ‘const std::pair<long unsigned int, long unsigned int>’ and ‘ignition::gazebo::v6::components::Component<int, ignition::gazebo::v6::components::IntComponentTag>* const’)
   if (lhs == rhs) {

[adlarkin]

CI caught a compilation error and I can reproduce it locally

Oops, it looks like that came from 158a1af 😅 it should be fixed in 713c848. I just tested the levels world, and removing entities seemed to work fine for me. Just as a note, I typically just build gazebo from source and then use the binary dependencies for PR testing, but I found that I also had to build ign-launch from source as well. I believe it's because of the changes that were made in include/ignition/gazebo/components/Factory.hh in #856.

[iche033]

works for me.

I can't reproduce the Server test failures on ubuntu and does not look like it's introduced by changes in this PR. We can get this in first and check CI results again in #856