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

//joint/axis/use_parent_model_frame does not fail fast or issue a warning in SDFormat 1.7? #327

Closed
EricCousineau-TRI opened this issue Aug 3, 2020 · 6 comments · Fixed by #490

Comments

@EricCousineau-TRI
Copy link
Collaborator

EricCousineau-TRI commented Aug 3, 2020

Ran across this here:
RobotLocomotion/drake#13758 (review)

Specifically, here:
https://github.com/RobotLocomotion/drake/blob/ab20bf25d9e9c05f861d7b6905cd6ef4dd9950c7/examples/multibody/four_bar/four_bar.sdf#L249
(note that //sdf/@version="1.7")

Perhaps it did print out a console warning... will need to confirm.
Ideally, though, this would eventually cause things to fail fast. Perhaps in SDFormat 1.8, we can make custom elements much more strict? (e.g. it must be namespaced?)

FYI @scpeters @azeey

@scpeters
Copy link
Member

scpeters commented Aug 4, 2020

some history on this:

As of libsdformat 2.3.0, the parser would fail hard and print an sdferr console message if any unrecognized elements were parsed. This was relaxed to ignoring the unrecognized elements and printing an sdfwarn message in 7999192 (bitbucket pull request 148). The changed behavior was released in libsdformat 2.3.1.

The behavior was changed further in 1f9156f (bitbucket pull request 449) to stop ignoring unrecognized elements and instead copy them and print an sdfdbg message (which doesn't go to the console by default). This change was released in libsdformat 8.0.0.

We could consider moving back to either of these behaviors (ignore and warn, or fail and error) in libsdformat10. Edit: We could also consider implementing "ignore and warn" in libsdformat9

@azeey
Copy link
Collaborator

azeey commented Aug 5, 2020

We'll probably have to check OriginalVersion to avoid failing on upconverted files that have unrecognized elements.

@chapulina
Copy link
Contributor

We could consider moving back to either of these behaviors

Not to feature creep, but another option would be to make the behavior configurable 😇 I can imagine even the same application using different behaviors at different times. For example, you may want to be more forgiving when loading an entire world file, but more strict when inserting a single model.

@brawner
Copy link
Collaborator

brawner commented Jan 30, 2021

Opened #481 to add a configurable way to fail with unrecognized elements. It's more general than this specific use case, but thanks to @azeey ParserConfig PR (#439) more customization is always possible.

@EricCousineau-TRI
Copy link
Collaborator Author

Will do a quick PR to see if #481 addressed this issue.

@chapulina Does Steven's work address your request for configuration? (I assume so, but just wanna check!)

@EricCousineau-TRI
Copy link
Collaborator Author

Confirmed and encoded in #490

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants