Support nested model #258
Support nested model #258
Conversation
|
CC @Kevinskwk to test with our nested model simulation environment. |
|
I don't know if this is slightly outside the scope. From what I understand the support was only introduced for TPE (which is OK), however when running a world with nested models with DART the behavior changed from printing an error about nested models not being supported (before the PR), to a segmentation fault with the attached backtrace. |
|
I added a check in 7740d5c that to make sure the constructed model is not null and warned the user if it is. Note you'll also need to update the |
Tried it out and indeed the crash is fixed, thanks! I'll try to get a full tpe sim up and running but that is quite a daunting task so don't keep it as a blocker to get this merged 😅 |
There was a problem hiding this comment.
I'm a bit torn about this. We're adding Model::ConstructModel inside a the ConstructSdfModel feature, but then using the same Implementation function with an extra check. This creates a situation where a feature is partially implemented by a physics engine. In this case, the DART plugin doesn't have this capability so it returns an invalid ID. Instead it would be nice to have a separate feature, maybe called ConstructSdfNestedModel so that other physics engines that don't support nesting can still be used when nesting is not required.
On the flip side, since the SDFormat composition proposal is still in progress, the newly created ConstructSdfNestedModel would have the same issue of being a partial implementation once the SDFormat proposal lands in libsdformat11. Nevertheless, my preference would be to create the new feature and document that it is not complete. This would be a notice to whoever wants to implement this feature that its requirements may change soon.
If we create the new feature, we can formally limit the ConstructSdfModel feature to work with non-nested models.
ok I added ConstructSdfNestedModel feature in gazebosim/gz-physics#86 and updated the physics system to use this optional feature in 57c2d1d
I have not added this limitation yet. To confirm, this means we need to add |
Yes, this is what I was thinking. @scpeters, does this sound reasonable? |
src/systems/physics/Physics.cc
Outdated
| const auto *staticComp = | ||
| _ecm.Component<components::Static>(_parent->Data()); | ||
| _ecm.Component<components::Static>(topLevelModel); |
There was a problem hiding this comment.
I think it might be better to override the <static> element of child models during creation and checking the model's own <static> rather than going up the tree and fining the top level model. If, for example, a model is a grandchild whose parent is static=true, but whose grandparent (top level model) is static=false, this would not catch that.
More detail: gazebosim/sdf_tutorials#35
There was a problem hiding this comment.
ok I see, it seems like the proposal allows the child nested models to be static while the top level model can be non-static but not the other way around? So:
- if top level model is static, all nested models in the tree will be static.
- if top level model is non-static, nested models can be static or non-static
implemented in c070152
I added a new |
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
There was a problem hiding this comment.
I'm approving this PR, but I want to note that we need to address the issue of nested model poses at some point. This PR only updates the pose of the top level model and leaves the poses of nested models at their initial state. However, a number of plugins (eg. Breadcrumbs) rely on the model's pose in their computation, which means they will fail if the model they are in is nested inside another model. This is not an issue right now since TPE doesn't support joints and pose and velocity commands are disabled for nested models.
| auto ent = this->CreateEntities(_model, true, false); | ||
|
|
||
| // Load all model plugins afterwards, so we get scoped name for nested models. | ||
| for (const auto &[entity, element] : this->dataPtr->newModels) |
There was a problem hiding this comment.
Using a std::map for newModels will probably mean that the plugins of the parent model will be loaded before the plugins of the child model since the Entity ID of the parent will be a smaller number than the child's. Do you think that would be an issue?
There was a problem hiding this comment.
This was mainly done so the all the ancestor models are loaded first before loading the nested model's plugin so that it's able to get the scoped name. As for the order of loading the plugins, I think it maybe fine but not 100% certain. I just checked the behavior in gazebo classic and it's also loading the parent model's plugin first before the children's plugins. So we could give this a try and come back if we run into issues in the future?
src/systems/physics/Physics.cc
Outdated
| << " engine doesn't support feature " | ||
| << "[ConstructSdfNestedModelFeature]. " | ||
| << "Nested model will be ignored." | ||
| << std::endl; |
There was a problem hiding this comment.
I think a71a3d2 added the informed variable in the first entityCast above, but not here.
examples/worlds/nested_model.sdf
Outdated
| @@ -0,0 +1,110 @@ | |||
| <?xml version="1.0" ?> | |||
| <sdf version="1.6"> | |||
Signed-off-by: Ian Chen <[email protected]>
|
ok I think I addressed all comments and ticketed issue #392 about the nested model pose issue. |
|
@osrf-jenkins run tests please |
chapulina
added
needs upstream release
|
when finding the sdformat9 version in cmake, I think we should require version 9.3.0 |
Signed-off-by: Ian Chen <[email protected]>
|
bumped sdformat version requirement to 9.3.0 in 44cd092 |
chapulina
removed
the
needs upstream release
Signed-off-by: Ian Chen <[email protected]>
Codecov Report
@@ Coverage Diff @@
## ign-gazebo3 #258 +/- ##
===============================================
+ Coverage 77.19% 77.21% +0.02%
===============================================
Files 203 203
Lines 10864 10963 +99
===============================================
+ Hits 8386 8465 +79
- Misses 2478 2498 +20
Continue to review full report at Codecov.
|
|
Ouch, this should have been squash-merged. Now all of this PR's commits are scattered around the |
|
ohh oops, sorry. I thought it defaulted to squash and merge. I'll make sure to double check next time. |
Yeah it annoyingly defaults to the last used, which may be "merge commit" or "rebase and merge" in case of ports across branches 😕 |
depends on gazebosim/gz-physics#86 and gazebosim/sdformat#316
This PR updates the physics system to support parsing and constructing nested models from Model SDF DOM (see gazebosim/sdformat#316).
Since the physics system relies heavily on canonical links for setting and also getting pose information of models in the physics engine, some changes were made to support the case where the canonical link is inside a nested model, e.g.:
In the example above, the top level model has one nested model and no child links. In this case, the canonical link would be set to the link inside the nested model. A couple of helper functions (
TopLevelModelandRelativePose) were added to help compute the transform from the top level model to the nested link in order to correctly set and get model poses.Note that I considered using ign-physics's
ModelFrameSemanticsfeature to compute this transform data but the feature is currently not part of the minimum feature list, so it's probably not a good idea to depend on an optional physics feature when computing pose updates for a core part of the physics system.Here's an example nested model world: