Allow SDF model to be constructed in a single shot

[mxgrey]

🎉 New feature

Summary

This PR tweaks the behavior of the physics system so that when constructing a model it will first check if a ModelSdf component is available for the entity. If so, it uses that component to fill in all the model information, overriding the fields that are given by the other entity components (e.g. name and pose).

When creating a link, the physics engine will first check if a link of the same name for that model was already constructed. If so, the construction step is skipped, the link is queried from the model, and it gets added to the entity map. Same for joints and collision objects.

The net result of these behavioral changes is that physics engines which can only construct models all at once are able to be used for the typical use cases, while physics engines that can construct models incrementally can still do so when the user wants them to.

Test it

Try running with the dartsim plugin. Even though the behavior changed, there is no noticeable difference in the outcome.

The ability to test these changes with a bullet-featherstone plugin is forthcoming. The implementation is finished, but I am debugging some segfaults from inside of bullet at the moment.

Checklist

• Signed all commits for DCO
• Added tests
• Added example and/or tutorial
• 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.

[scpeters]

the reason for this is that the mesh and heightmap SDFormat elements contain URI's that in many cases point to fuel resources, but libsdformat doesn't depend on fuel. The Attach*ShapeFeatures accept gz::common types that contain the mesh and heightmap info, and gz-sim is responsible for fetching the data and populating those types.

Properly implementing the ConstructSdfMesh and ConstructSdfHeightmap would involve resolving those URIs, which seems beyond the responsibility of a physics plugin, so perhaps we should just deprecate those features from gz-physics

[mxgrey]

Yeah, I think the motivation totally makes sense.

I think what I'd propose is to move the implementations of the uri resolving functionality into a reusable utility function in the SDFormat library. Maybe even have them as methods in MeshShape and HeightmapShape classes for maximum visibility. The utility functions could just output the same data structures that are being passed into AttachMeshShape and AttachHeightmapShape so the physics plugin developer can easily recycle the implementations of those functions for ConstructSdfCollision.

[chapulina]

I've been confused by this in the past (gazebosim/gz-physics#9):

"For example, what does ConstructCollision do? Does it create and attach a collision to a link? How is that different from AttachMeshShape? If they do similar things, why are their names so different?"

mesh and heightmap SDFormat elements contain URI's that in many cases point to fuel resources, but libsdformat doesn't depend on fuel

SDF has the ability of resolving Fuel URIs for <include>s, we pass it a Fuel-Tools callback so it knows how to resolve those:

gz-sim/src/Server.cc

Lines 66 to 68 in 417609c

	// Configure SDF to fetch assets from ignition fuel.
	sdf::setFindCallback(std::bind(&ServerPrivate::FetchResource,
	this->dataPtr.get(), std::placeholders::_1));

So it could also resolve the URIs for other assets.

I think the main problem is that SDFormat doesn't depend on Gazebo Common, which is a huge dependency and brings external graphics libraries, so it can't return gz::common types.

I guess one option is passing those resolved URIs to gz-rendering and gz-physics, and each implementation decides how to load the assets. They can use gz::common if they want. It may cause a lot of duplication across various plugins though, or inconsistencies on how the meshes are loaded if they use different loaders.

[scpeters]

I think the main problem is that SDFormat doesn't depend on Gazebo Common, which is a huge dependency and brings external graphics libraries, so it can't return gz::common types.

yeah, I think this is the real crux of the problem. Perhaps we could add basic Mesh and HeightmapData classes to gz-math, while keeping the parsers in gz-common? Then libsdformat could return the gz-math types, and the find-file callback function could use fuel and gz-common's parsers to resolve the URI into the math object

[azeey]

I think @mjcarroll has a solution with gazebosim/sdformat#1147 so that the SDF DOM object passed into gz-physics has all the resource paths resolved.

[scpeters]

I think @mjcarroll has a solution with gazebosim/sdformat#1147 so that the SDF DOM object passed into gz-physics has all the resource paths resolved.

that means we don't need to use fuel, but it's still just a file path provided by libsdformat, but something will still need to parse that file, and in general we can't assume that the physics implementation will be able to parse it, so I think that wouldn't resolve the matter of properly implementing ConstructSdfMesh or ConstructSdfHeightmap in the general case so that AttachMeshShape and AttacheHeightmapShape aren't needed

[ahcorde]

There is a segfault when I try to load vanilla bullet.

In particular code is failing here:

gz-sim:
https://github.com/gazebosim/gz-sim/blob/merge-full-model-construction/src/systems/physics/Physics.cc#L2850-L2852

gz-physics:
https://github.com/gazebosim/gz-physics/blob/bullet-featherstone-collisions/bullet/src/KinematicsFeatures.cc#L25-L38

Another issue

This is not working with TPE, because this engine doesn't support the physics feature GetJointFromModel

[Wrn] [SystemLoader.cc:56] Trying to load deprecated plugin [ignition-gazebo-physics-system]. Using [gz-sim-physics-system] instead.
[Wrn] [Physics.cc:884] Plugin [gz::physics::tpeplugin::Plugin] misses required features:
- N2gz7physics17GetJointFromModelE

[ahcorde]

Joint is not placed in the right pose (bullet-featherstone), tested with log_record_dbl_pendulum.sdf

[mxgrey]

The seg fault issue with the classic bullet plugin is fixed by gazebosim/gz-physics#393 and the TPE failing to load is fixed by 4b01010.

I think the joint offset issue for bullet-featherstone shouldn't be a blocker for this PR since that will need to be fixed in gazebosim/gz-physics#373 and wouldn't involve any changes to this PR.

[mxgrey]

@osrf-jenkins retest this please

[mjcarroll]

@mxgrey we have a whole slew of test failures here that aren't in main. I spent a little bit of time, but didn't make much headway.

[chapulina]

Removed the Garden label because we're past feature freeze ❄️ .

This PR can either be merged into main and backported to gz-sim7 after the stable release, or retargeted to gz-sim7 and merged after the stable release (i.e. October).

[livanov93]

@mxgrey @ahcorde What needs to be done to merge this one?

[mjcarroll]

@osrf-jenkins retest this please

[mjcarroll]

@mxgrey @ahcorde What needs to be done to merge this one?

I believe that I got all the tests passing, but CI isn't reflecting that yet, giving it another go.

[mjcarroll]

@osrf-jenkins retest this please

[codecov]

Codecov Report

Merging #1560 (d411d1b) into gz-sim7 (6a8594b) will increase coverage by 0.09%.
The diff coverage is 69.80%.

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

@@             Coverage Diff             @@
##           gz-sim7    #1560      +/-   ##
===========================================
+ Coverage    64.16%   64.25%   +0.09%     
===========================================
  Files          335      336       +1     
  Lines        26508    26889     +381     
===========================================
+ Hits         17008    17277     +269     
- Misses        9500     9612     +112     

Impacted Files	Coverage Δ
include/gz/sim/SystemLoader.hh	100.00% <ø> (ø)
include/gz/sim/detail/EntityComponentManager.hh	93.86% <ø> (ø)
include/gz/sim/rendering/RenderUtil.hh	100.00% <ø> (ø)
.../plugins/component_inspector/ComponentInspector.hh	28.57% <ø> (ø)
src/gui/plugins/scene_manager/GzSceneManager.cc	17.58% <0.00%> (-0.60%)	⬇️
...rc/systems/ackermann_steering/AckermannSteering.hh	100.00% <ø> (ø)
src/systems/track_controller/TrackController.cc	46.80% <ø> (+0.19%)	⬆️
.../systems/triggered_publisher/TriggeredPublisher.hh	100.00% <ø> (ø)
.../plugins/component_inspector/ComponentInspector.cc	5.46% <2.77%> (-0.33%)	⬇️
...s/multicopter_motor_model/MulticopterMotorModel.cc	75.91% <33.33%> (-0.22%)	⬇️
... and 19 more

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