fix updating a component's data via SerializedState msg

[adlarkin]

Signed-off-by: Ashton Larkin ashton@openrobotics.org

🦟 Bug fix

Summary

While working on #1109, I found that there's a bug in ign-gazebo where an existing component's data isn't modified correctly through a msgs::SerializedState. There's also a TODO note about this in the code, which makes me think that this issue has existed for a while: https://github.com/ignitionrobotics/ign-gazebo/blob/447e5c231ec312bd0ce4e4d349222145e0e135d5/src/EntityComponentManager.cc#L1692-L1697

The issue is that if the default assignment operator is used between base classes, derived class data is lost in the assignment (we are using BaseComponent in src/EntityComponentManager.cc since that file doesn't have access to template types). The solution is to use BaseComponent::Deserialize, since this method is implemented by derived classes: https://github.com/ignitionrobotics/ign-gazebo/blob/ignition-gazebo6_6.0.0/include/ignition/gazebo/components/Component.hh#L358-L359

For a simple example of how derived class data is lost when using the default assignment operator between base classes, see https://godbolt.org/z/hezKTbeEj

I should also mention that this fix will need to be backported. I didn't target an older branch in this PR because this fix is needed by #1109, which is urgent and doesn't have time to wait for forward-ports/releases.

Test it

I have added unit tests for the EntityComponentManager class in this PR. Without the changes in this PR, the test that updates an ECM's component via a msgs::SerializedState fails.

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

[adlarkin]

@nkoenig @chapulina is there an existing issue open that would be fixed by this PR? If so, I will link it so that this PR closes the issue. I couldn't find any related issues when searching through the issue tracker in ign-gazebo. This scenario was never tested before, so my guess is that this wasn't a known issue until now.

[codecov]

Codecov Report

Merging #1131 (37e5af6) into ign-gazebo6 (1b4c9a0) will increase coverage by 0.11%.
The diff coverage is 69.23%.

@@               Coverage Diff               @@
##           ign-gazebo6    #1131      +/-   ##
===============================================
+ Coverage        63.72%   63.84%   +0.11%     
===============================================
  Files              256      256              
  Lines            20075    20076       +1     
===============================================
+ Hits             12793    12817      +24     
+ Misses            7282     7259      -23     

Impacted Files	Coverage Δ
src/EntityComponentManager.cc	87.03% <69.23%> (+2.94%)	⬆️

---

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 1cf1ca4...37e5af6. Read the comment docs.

[nkoenig]

@nkoenig @chapulina is there an existing issue open that would be fixed by this PR? If so, I will link it so that this PR closes the issue. I couldn't find any related issues when searching through the issue tracker in ign-gazebo. This scenario was never tested before, so my guess is that this wasn't a known issue until now.

I don't think there is an issue for this. Thanks for fixing it.