Use deserialize to remove CreateComponentImplementation's template dependency

[adlarkin]

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

🦟 Bug fix

Summary

When I worked on #856, I came across an issue where I could not update derived component data when dealing with BaseComponent pointers in EntityComponentManager::CreateComponentImplementation: #856 (comment)

The workaround at the time was to have CreateComponentImplementation return a flag that indicated whether the derived component data needed to be updated or not. If the data needed to be updated, it was taken care of in the template method that called CreateComponentImplementation, since the template method knew what the derived component type is.

However, after working on #1131, I realized that we can use component's Serialize/Deserialize methods in EntityComponentManager::CreateComponentImplementation to properly update the derived component data when working with BaseComponent pointers. The reason why this works is because these serialization methods are virtual methods that are implemented by both the BaseComponent and derived component classes. A regular assignment operator wouldn't work because the component classes don't have this operator implemented as a virtual override (see https://godbolt.org/z/hezKTbeEj for an example of what happens when a default assignment operator is used with polymorphism - spoiler alert: the derived class data is lost in the assignment).

As a result of this change, CreateComponentImplementation can return a pointer to the component instead of a update flag, and the template method that calls CreateComponentImplementation can then use this component directly. I can't change the signature of this method in ign-gazebo6 since it will break ABI, but I left a TODO note about fixing it in ign-gazebo7. Since CreateComponentImplementation is a private method, we don't need to tick-tock it, right? We can change a private method without having to deprecate it, if I understand correctly.

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

[codecov]

Codecov Report

Merging #1148 (d212b22) into ign-gazebo6 (4d846d3) will decrease coverage by 0.13%.
The diff coverage is 15.25%.

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

@@               Coverage Diff               @@
##           ign-gazebo6    #1148      +/-   ##
===============================================
- Coverage        63.84%   63.71%   -0.14%     
===============================================
  Files              256      256              
  Lines            20077    20112      +35     
===============================================
- Hits             12819    12815       -4     
- Misses            7258     7297      +39     

Impacted Files	Coverage Δ
include/ignition/gazebo/EntityComponentManager.hh	100.00% <ø> (ø)
include/ignition/gazebo/gui/GuiEvents.hh	0.00% <0.00%> (ø)
src/gui/plugins/entity_tree/EntityTree.hh	100.00% <ø> (ø)
src/gui/plugins/select_entities/SelectEntities.cc	10.04% <0.00%> (ø)
src/rendering/SceneManager.cc	28.55% <ø> (ø)
src/gui/plugins/entity_tree/EntityTree.cc	9.00% <4.54%> (-0.62%)	⬇️
...e/ignition/gazebo/detail/EntityComponentManager.hh	95.83% <100.00%> (+2.41%)	⬆️
src/EntityComponentManager.cc	87.10% <100.00%> (-0.07%)	⬇️
src/SimulationRunner.cc	93.08% <0.00%> (-0.85%)	⬇️

---

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 9e00fc8...ece655e. Read the comment docs.

[adlarkin]

After some discussion that was had offline, a point was brought up that not all components have Deserialize implemented. So, we may need to close this PR and stick with the current approach (cc @chapulina).

[chapulina]

It was a good idea though. Maybe in the future we can make sure all components are serializable.

[adlarkin]

Closing this for now since it's not guaranteed that all components are deserializable.