Joint limits silently ignored if improperly nested

[avalenzu]

If a user specifies a joint as follows

<joint name="foo" type="prismatic">
    <parent>bar</parent>
    <child>baz</child>
    <axis>
      <xyz>1 0 0</xyz>
    </axis>
    <limit>
      <lower>0</lower>
      <upper>1</upper>
      <velocity>10</velocity>
      <effort>1000</effort>
    </limit>
  </joint>

the <limit> element will be silently ignored, since it is not inside the <axis> element. That silence can make it rather difficult to debug this as an end user.

[EricCousineau-TRI]

Cookie crumb: Andres ran into this while working with Drake's usage of libsdformat through the following API:
https://github.com/RobotLocomotion/drake/blob/v0.26.0/multibody/parsing/parser.h

[EricCousineau-TRI]

Putting this on the queue for libsdformat11!

[brawner]

It appears like the current behavior of the parser is to ignore any tags that aren't part of the spec. Is the suggested fix to start issuing warnings or errors when tags are included that don't match spec?

I was planning to implement this with just warnings for now, but that makes it impossible to test. Perhaps I should also add a config option to @azeey's ParserConfig for a pendantic flag?

[brawner]

After inspecting the source more closely it looks like these conditions are already checked. They just aren't presented to the user consistently.

Extra attributes in an element issue a warning:
https://github.com/osrf/sdformat/blob/b9fc65767f03c838c3583862ccb30a2c149ce230/src/parser.cc#L922-L924

Extra child elements currently only inform with a sdfdbg.
https://github.com/osrf/sdformat/blob/b9fc65767f03c838c3583862ccb30a2c149ce230/src/parser.cc#L1196-L1199

[scpeters]

see also #327 (comment)

[brawner]

Ooh, thanks for the bread crumb Steve. Perhaps adding some configurability in Addisu's parser config is the way to go

[jwnimmer-tri]

I'll let @EricCousineau-TRI chime in a with more thoughtful statement of the below once he's back online, but from Drake's perspective we have another consideration I'll attempt to describe.

We offer our own custom extensions to sdformat syntax (e.g., "<drake:rotor_inertia value='1.5' />") for things that are not yet expressible in the native sdformat schema.

I'd love to be able to have libsdformat produce errors for unknown elements that are written without a namespace, but then also silently defer to the calling application for any potential errors from the namespace-qualified elements. I want to let Drake do the error-checking and error-reporting for the drake: elements (and when the element is valid to us, that there is no error printed), but I always want to error out for any non-drake elements that libsdformat didn't understand.

Edit: Maybe Drake can already do this, by walking through the list of "unrecognized elements" given to us by libsdformat, and checking that all of them are in the drake namespace?

[brawner]

I think those elements containing a : are already ignored and copied over. Preserving that behavior makes sense

https://github.com/osrf/sdformat/blob/b9fc65767f03c838c3583862ccb30a2c149ce230/src/parser.cc#L1193-L1194

[brawner]

Opened #481

I think my new tests show that with the pedantic setting, unrecognized elements are flagged, unless they have a namespace character.

[EricCousineau-TRI]

Maybe Drake can already do this, by walking through the list of "unrecognized elements" given to us by libsdformat, and checking that all of them are in the drake namespace?

👍

There's also an issue to handle / provide best practices for custom namespace validation:
#286