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.

Sign up for GitHub

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

Jump to bottom

URDF parser shouldn't treat multitree as an error #1267

Closed
jslee02 opened this issue Mar 29, 2019 · 4 comments
Closed

URDF parser shouldn't treat multitree as an error #1267

jslee02 opened this issue Mar 29, 2019 · 4 comments
Labels
type: bug Indicates an unexpected problem or unintended behavior

Comments

@jslee02
Copy link
Member

jslee02 commented Mar 29, 2019 •
edited
Loading

URDF parser errors when the root link is "world" (treated as a keyword) and the root link has multiple children. However, DART supports multitree in a skeleton. Moreover, this logic continues with this error and yet surprisingly it successfuly creates multitree anyway.

We should make it clear that it's not an error and update the logic accordingly.
@jslee02 jslee02 added the type: bug Indicates an unexpected problem or unintended behavior label Mar 29, 2019
@mxgrey
Copy link
Member

mxgrey commented Apr 1, 2019

I think URDF wasn't designed to support multi-tree robot descriptions, so the original author of this (which wasn't me; all I did was refactor this code) decided that we should consider instances of multi-tree URDFs to be an error.

There are a few things about our URDF parser that aren't really standard, though, so I'm completely in favor of supporting a superset of the URDF standard.

@jslee02
Copy link
Member Author

jslee02 commented Apr 2, 2019

I think URDF wasn't designed to support multi-tree robot descriptions

That is true. So I think we should either return nullptr with an error message for that case (which is not the current implementation) or allow multi-tree robot with a warning message saying it's not the standard.

I'm inclined to the second option.

@mxgrey
Copy link
Member

mxgrey commented Apr 2, 2019

The second option sounds good to me too 👍

@jslee02 jslee02 mentioned this issue Apr 3, 2019
Merged
5 tasks
jslee02 added a commit that referenced this issue Apr 6, 2019
@jslee02
Change urdf parser to warn if robot model has multi-tree (#1270)
0f4cf21 
This PR addresses #1267.

***

**Before creating a pull request**

- [x] Document new methods and classes
- [x] Format new code files using `clang-format`

**Before merging a pull request**

- [x] Set version target by selecting a milestone on the right side
- [x] Summarize this change in `CHANGELOG.md`
- [x] Add unit test(s) for this change
@jslee02
Copy link
Member Author

jslee02 commented Apr 6, 2019

Resolved by #1270

@jslee02 jslee02 closed this as completed Apr 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
type: bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@mxgrey @jslee02