-
Notifications
You must be signed in to change notification settings - Fork 147
Add support for capsule geometry type #238
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
Conversation
Signed-off-by: Sai Kishor Kothakota <sai.kishor@pal-robotics.com>
b151e11 to
4c8716d
Compare
Signed-off-by: Sai Kishor Kothakota <sai.kishor@pal-robotics.com>
scpeters
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you mind adding a schema test for the capsule feature similar to https://github.com/ros/urdfdom/blob/rolling/urdf_parser/test/urdf_schema_quaternion_test.cpp?
Signed-off-by: Sai Kishor Kothakota <sai.kishor@pal-robotics.com>
Thank you for pointing out. I completely missed it 🙈 |
|
While adding tests on ros/urdf_parser_py#96 |
urdf_parser/src/link.cpp
Outdated
| return false; | ||
| } | ||
|
|
||
| if (!std::isfinite(c.length) || !std::isfinite(c.radius) || c.length < 0 || c.radius < 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a good logic check, but none of the other geometry parsing functions have something like it
how do you feel about reverting this logic check from URDF 1.1 and open a separate pull request that will target rolling and add these logic checks to this and the other geometry parsing functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this is a new entry, I think it makes sense to have it integrated with the checks already. I personally would keep it.
For the rest of the geometry types, yes I already have a plan to do that for other types and rest of the components in the URDF
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's your opinion on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we shouldn't backport the additional logic checks for existing geometry types. So we can eventually merge them into rolling, but I wouldn't include those checks in URDF 1.1
A case could be made that for consistency, the Capsule code for URDF 1.1 that will be backported should also not include the checks. It's subjective, so I won't demand it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood. I can remove them. I'll open a different PR adding such logic to all the geometry cases targeting rolling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@scpeters Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've the other checks done here: https://github.com/saikishor/urdfdom/tree/extend/invalid_data/checks
Once this is merged, I can open the new one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Signed-off-by: Sai Kishor Kothakota <sai.kishor@pal-robotics.com>
|
urdfdom_headers 2.1.0 has been released (ros/rosdistro#49659), so I have updated the |
Perfect. Thanks |
|
Seems like CI is failing as it is taking binaries from stable branch |
|
Thanks for taking care of CI |
|
well I've tried, but I'm not sure what is going wrong. I'm going to close and reopen once in order to restart CI, but if that doesn't work I'll run tests locally |
|
I agree. it is weird. For me it works locally with recent changes. Details |
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
003b5b3 to
65924e6
Compare
|
the Rpr job is fixed now, and I wasn't able to fix the |
65924e6 to
6823576
Compare
|
Thank you so much |
Needs ros/urdfdom_headers#94