Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarification on //link/must_be_base_link element #700

Open
diegoferigo opened this issue Sep 14, 2021 · 3 comments
Open

Clarification on //link/must_be_base_link element #700

diegoferigo opened this issue Sep 14, 2021 · 3 comments
Labels
bug Something isn't working

Comments

@diegoferigo
Copy link

While addressing a downstream issue robotology-legacy/gym-ignition#385, I discovered the existence of //link/must_be_base_link. After checking upstream, I only found references to the specification, but I didn't find any place where this element is actually read.

Is it used elsewhere? Should it be deprecated in favor of the new //model/@canonical_link attribute?

Environment

Issue related to SDF specification.

Description

  • Expected behavior: clarify if this element is used and read at least in Gazebo (Classic / Ignition)
  • Actual behavior: this element seems ignored
@diegoferigo diegoferigo added the bug Something isn't working label Sep 14, 2021
@FirefoxMetzger
Copy link
Contributor

FirefoxMetzger commented Sep 14, 2021

Judging by the specification both tags would have slightly different behavior since @canonical_link would also attach the implicit __model__ frame to the named link. Also, it would be impossible to attach multiple links of one model to the implicit world frame.

However, there is enough overlap to make this quite redundant indeed and having multiple //must_be_base_link elements inside the same model scope is probably just an edge case that I use for unit tests; I doubt it has much adoption "in the wild".

@azeey
Copy link
Collaborator

azeey commented Sep 14, 2021

The only place I've seen it used is in Gazebo classic in the Simbody support classes. https://github.com/osrf/gazebo/blob/f54f95e7cad959d22d5af1549149da6e81c12bed/gazebo/physics/simbody/SimbodyLink.cc#L61.

Conceptually, I think it's different from the canonical link, since the canonical link is simply the link to which the model frame (__model__) is attached. This link can be any link in the model. Whereas, the documentation for //link/must_be_base_link states that the link will have 6DOF. This is more like the concept of the root link in gazebosim/gz-physics#233.

Since it was only used for one physics engine, I'm in favor of deprecating it. If the need for a similar tag arises for a physics engine in the future, perhaps we can use custom elements instead.

@FirefoxMetzger
Copy link
Contributor

@azeey After reading through gazebosim/gz-physics#233 and considering your comment in #705 (comment) I'm actually wondering if it would be useful to not fully depreciate this element, but rather to replace it.

What I was thinking was that, indeed, //must_be_base_link doesn't quite fit the semantics of the current spec; however a //joint with type "free" does (which would be a 6DoF joint). In this case, //must_be_base_link could be replaced with

<link name="this_link" />
<joint name="fancy_joint" type="free">
<parent>world</parent>
<child>this_link</child>
</joint>

A nice implication of this would be that it now becomes easy to spot errors in the pose graph of an SDF. Currently a SDF of the form

<?xml version="1.0"?>
<sdf version="1.7">
  <world name="my_world">
    <model name="M1">
      <link name="L1"/>
      <link name="L2"/>
    </model>
  </world>
</sdf>

is a bit ambiguous in the sense that it is unclear if the author intended to create two independently moving (free) links, or if the author has forgotten to add a link. Adding the type="free" joint, would make this very easy to spot since now every //link will have to be connected to world via a chain of //joints. It could also be me not being aware of documentation on this, but I think that the behavior of the above model (multiple, independently floating links) is not explicitly defined by the spec. Perhaps this could be an opportunity to clear that up, too.

On a sidenote, it would also make my life easier. In scikit-bot I have a feature that builds the pose graph by connecting all frames declared in the SDF using //joints and //frame/attached_to (+ implicit attachment rules of //model, //sensor, etc.). My current problem with this is that I often don't end up with a single tree where world is the root, but rather with a set of trees, because something like a mobile robot / mobile base, won't define an explicit constraint w.r.t. world.

As a result, I have to walk over all declared frames after the graph(s) is/are connected and find connected components (afterwards, because I have to account for model nesting). Then, for each connected component, I have to insert a 6DoF connections (i.e. type="free" joints) to world so that all the nodes are connected and the behavior matches that of ignition gazebo. It would be a lot simpler if I could rely on SDFormat setting this joint explicitly, so that this can be solved naturally during graph construction instead of doing this complex clean up step.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants