Fix included nested model expansion in SDF generation

[jennuine]

🦟 Bug fix

Part of #479

Summary

This PR resolves the issue of included nested models (e.g., //model/include) being expanded even though the option "Expand include tags" is disabled.

Before PR

1. Load model_nested_include.sdf in Ignition
2. Click on the top left menu > Save world as...
3. Save the world (with "Expand include tags" disabled)
4. Open the saved sdf file and see the included models are expanded

After PR
Run the above steps again except the saved sdf file will have //include tags instead

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 #768 (73b7110) into ign-gazebo5 (d3ee64a) will increase coverage by 0.06%.
The diff coverage is 72.72%.

❗ Current head 73b7110 differs from pull request most recent head 32dd57a. Consider uploading reports for the commit 32dd57a to get more accurate results

@@               Coverage Diff               @@
##           ign-gazebo5     #768      +/-   ##
===============================================
+ Coverage        65.32%   65.39%   +0.06%     
===============================================
  Files              240      240              
  Lines            17624    17679      +55     
===============================================
+ Hits             11513    11561      +48     
- Misses            6111     6118       +7     

Impacted Files	Coverage Δ
.../plugins/component_inspector/ComponentInspector.cc	7.04% <0.00%> (-0.17%)	⬇️
src/rendering/RenderUtil.cc	42.10% <0.00%> (-0.05%)	⬇️
src/systems/physics/Physics.cc	69.44% <83.01%> (ø)
src/SdfGenerator.cc	94.77% <89.28%> (-1.23%)	⬇️
src/systems/physics/EntityFeatureMap.hh	93.65% <90.90%> (ø)
src/systems/diff_drive/DiffDrive.cc	86.91% <100.00%> (+0.94%)	⬆️
src/Server.cc	83.43% <0.00%> (+0.63%)	⬆️
src/SimulationRunner.cc	93.77% <0.00%> (+1.03%)	⬆️
... and 1 more

---

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 a9cfa0d...32dd57a. Read the comment docs.

[azeey]

Saving the Fuel version number doesn't seem to work for me. Can you add a test to verify?

[jennuine]

I modified the previous test, what do you think? 57d5833

[azeey]

nit: this is the same as modelDir.

[jennuine]

Thanks 73b7110

[azeey]

tip: This can be simplified as e->GetIncludeElement()->Get<std::string>("uri")

[jennuine]

73b7110

[azeey]

I noticed another thing. This changes the order of models. I'm testing with model_nested_include.sdf and the original has:

      <model name="M2">
        <include>
          <uri>sphere</uri>
          <pose>0 2 2 0 0 0</pose>
        </include>

        <model name="M3">
          <include>
            <name>coke</name>
            <uri>https://fuel.ignitionrobotics.org/1.0/OpenRobotics/models/Coke Can</uri>
            <pose>2 2 2 0 0 0</pose>
          </include>
        </model>
      </model>

The saved file has

      <model name='M2'>
        <model name='M3'>
          <include>
            <name>coke</name>
            <uri>https://fuel.ignitionrobotics.org/1.0/OpenRobotics/models/Coke Can</uri>
            <pose>2 2 2 0 -0 0</pose>
          </include>
        </model>
        <include>
          <uri>sphere</uri>
          <pose>0 2 2 0 -0 0</pose>
        </include>
      </model>

(with <plugin> removed). The order in the original is sphere, M3, whereas in the saved file M3,sphere. The two files will have different results when sdf::Model::ModelByIndex is called. I think can preserve the order if we do this:

Suggested change

	_elem->InsertElement(e->GetIncludeElement());
	e->RemoveFromParent();
	e ->Copy(e->GetIncludeElement());

This is the first time I've used sdf::Element::Copy and I now see its usefulness.

[jennuine]

Thanks for noticing that! 73b7110

[azeey]

I'm now getting the `//include/@name attribute set.

      <include name='include_nested_new_name'>
        <uri>include_nested</uri>
        <name>include_nested_new_name</name>
        <pose>1 2 3 0 -0 0</pose>
      </include>

I believe it's because e is a model and has a name attribute and sdf::Element::Copy doesn't seem to be clearing the attributes when copying. I think this is something we should fix in libsdformat. Since I don't know the reason for why attributes are not cleared in https://github.com/osrf/sdformat/blob/25004f0bad227cf11854cc208e74be08e8623359/src/Element.cc#L218-L220, I think it might be best to create another function to clear attributes instead of modifying sdf::Element::Copy.

[jennuine]

Should we create both a sdf::Element::RemoveAllAttributes() as well as sdf::Element::RemoveAttribute(string key)? Just to have both options for users

[azeey]

Yeah, that sounds good to me.

[jennuine]

Here's the PR that adds these methods: gazebosim/sdformat#555

This will be forward ported to dome & edifice once merged

[jennuine]

The new attribute removal methods have been ported to sdf11 and with b5a521c the attributes are cleared before copying the include element

[jennuine]

@osrf-jenkins run tests please

[jennuine]

Requires #758 to fix macOS build failure

[jennuine]

@osrf-jenkins run tests please

[azeey]

Looks great! Thanks for iterating.