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

[parsers] URDF and YAML generation from ParserResult #73

Merged
merged 1 commit into from
May 12, 2020

Conversation

BenjaminNavarro
Copy link
Contributor

Unless I'm missing something, the generated URDF/YAML models must be later read with transformInertia = false because there is no way to recover the original rotation from the available data. I don't know if it is a big issue or if we can find a workaround for this

Copy link
Member

@gergondet gergondet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok (I haven't looked very carefully at the conversion code itself but I'm assuming the tests cover it well)

I'd like to see a few changes:

  1. Factor the testing code for comparing ParserResult instances and generating the examples since they are basically the same between each tests
  2. I don't like that you need to have transformInertia = false with the generated files, you can get an inertia that will transform correctly by calling sva::inertiaToOrigin(inertia, -mass, com, Eigen::Matrix3d::Identity()). The only information that is definitely lost is the inertial frame orientation that could be in the original URDF.

src/parsers/CMakeLists.txt Outdated Show resolved Hide resolved
src/parsers/CMakeLists.txt Outdated Show resolved Hide resolved
src/parsers/CMakeLists.txt Outdated Show resolved Hide resolved
src/parsers/urdf.cpp Outdated Show resolved Hide resolved
src/parsers/urdf_yaml_conv.cpp Show resolved Hide resolved
src/parsers/urdf_yaml_conv.cpp Show resolved Hide resolved
@BenjaminNavarro
Copy link
Contributor Author

1. Factor the testing code for comparing `ParserResult` instances and generating the examples since they are basically the same between each tests

Ok no problem

2. I don't like that you need to have `transformInertia = false` with the generated files, you can get an inertia that will transform correctly by calling `sva::inertiaToOrigin(inertia, -mass, com, Eigen::Matrix3d::Identity())`. The only information that is definitely lost is the inertial frame orientation that could be in the original URDF.

I don't like it either so I'll try what you suggest and see if it works properly

@BenjaminNavarro
Copy link
Contributor Author

Ok, so the parser tests refactoring is done and transformInertia=false is no longer necessary! Let's see if the CI is happy

The generated URDF/YAML models must be read with transformInertia = false becausethere is no way to recover the original rotation from the available data
@gergondet gergondet merged commit 5404765 into jrl-umi3218:master May 12, 2020
@gergondet
Copy link
Member

Thanks for your contribution @BenjaminNavarro

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 this pull request may close these issues.

2 participants