Element Clone() now also clones the parent

[luca-della-vedova]

🦟 Bug fix

Fixes gz-sim issue gazebosim/gz-sim#1561

Summary

Cloning elements didn't copy the element's parent, hence this information was lost after a Clone() operation. This caused a few issues in downstream packages that relied on reading an element's parent, which would work with the original element but wouldn't as soon as the element was cloned.
Specifically this created some issues in Plugins at the gz-sim level.
I'm not sure whether it is intended that a cloned element has no parent, but if it's not this should fix it.

Note that Copy() has the same behavior of not copying the parent, but adding the same change there breaks a lot of downstream integration tests, so that's also something to keep in mind

Checklist

• Signed all commits for DCO
• Added tests
• Updated documentation (as needed)
• Updated migration guide (as needed)
• Consider updating Python bindings (if the library has them)
• 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 and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

[codecov]

Codecov Report

Merging #1116 (4fec0cd) into sdf12 (a84742c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##            sdf12    #1116   +/-   ##
=======================================
  Coverage   91.28%   91.29%           
=======================================
  Files          80       80           
  Lines       13086    13089    +3     
=======================================
+ Hits        11946    11949    +3     
  Misses       1140     1140           

Impacted Files	Coverage Δ
src/Element.cc	96.96% <100.00%> (+<0.01%)	⬆️
src/Plugin.cc	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[chapulina]

Seems reasonable to me. The behaviour change deserves at least a note in the migration guide. Not sure if this is something we want to do from Citadel and forward-port.

I'd like to confirm with @scpeters or @azeey if there are unintended consequences before merging though.

[azeey]

Generally, I would expect the code that called Element::Clone to be the one responsible for setting the parent (at least, that's what how the API is being used https://github.com/gazebosim/sdformat/blob/sdf12/src/Element.cc#L221). I'm concerned that changing it to automatically set the parent could cause unexpected, and perhaps, hard to debug issues. Before this PR, if you forgot to set the parent, the user that gets the cloned element will try to access the parent and fail since it would be a nullptr. Detecting that would be easier, I think. But with this PR, the user will access a parent element, which is most likely not the desired parent element. And finding this bug will be harder.

Also, it's not immediately clear to me how this fixes gazebosim/gz-sim#1561. Do you mind elaborating?

[luca-della-vedova]

Generally, I would expect the code that called Element::Clone to be the one responsible for setting the parent (at least, that's what how the API is being used https://github.com/gazebosim/sdformat/blob/sdf12/src/Element.cc#L221). I'm concerned that changing it to automatically set the parent could cause unexpected, and perhaps, hard to debug issues. Before this PR, if you forgot to set the parent, the user that gets the cloned element will try to access the parent and fail since it would be a nullptr. Detecting that would be easier, I think. But with this PR, the user will access a parent element, which is most likely not the desired parent element. And finding this bug will be harder.

Indeed, as I mentioned in the PR I am not quite aware if the current behavior is intended, I'm happy to close this PR if you think this change could cause issues in other areas since it's a pretty low level change.

Also, it's not immediately clear to me how this fixes gazebosim/gz-sim#1561. Do you mind elaborating?

It's been some time but if I remember correctly the issue was in two different areas (hence this PR):

• The ToElement API of Plugin didn't set the parent, even though technically this information is in its dataPtr->sdf property. This means that when offering the backward compatible API that needed an element, and performing the sdf::Plugin -> sdf::Element conversion through the ToElement API, the parent information was lost.
• Even with that specific issue fixed, (if I remember correctly) what is passed to systems is a clone of the original Element, since the Clone function doesn't copy the parent and again the parent information is lost.

Anyway I'll be closing this PR, at the end of the day the recommended approach in gazebosim/gz-sim#1561 (comment) works and sounds like it's the best way forward, from the silence in that issue it also sounds like we were the only ones using that specific workflow so it's probably fine not to go to great lengths and risk breaking other use cases for a quite obscure one.