ros / urdfdom

URDF parser
http://ros.org/wiki/urdf
Other
96 stars 132 forks source link

Color and texture in link material are optional #72

Closed otamachan closed 8 years ago

otamachan commented 8 years ago

According to http://wiki.ros.org/urdf/XML/link#line-79, a material of a link can be referenced only by name. But if neither a color tag nor a texture tag is present within a material tag, "Material has neither a color nor texture" error occurs.

This PR suppresses the above error.

scpeters commented 8 years ago

closing and reopening to trigger jenkins build

scpeters commented 8 years ago

The travis build fails because it can't find mock. Can you add google-mock to the .travis.yml file?

diff --git a/.travis.yml b/.travis.yml
index e0a4f2e..a1c91a6 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -8,7 +8,7 @@ compiler:
 script: "./.travis/build"
 before_install:
   - sudo apt-get update -qq
-  - sudo apt-get install -qq libboost-system-dev libboost-thread-dev libboost-test-dev libtinyxml-dev python-yaml
+  - sudo apt-get install -qq google-mock libboost-system-dev libboost-thread-dev libboost-test-dev libtinyxml-dev python-yaml
 matrix:
   allow_failures:
     - compiler: clang
otamachan commented 8 years ago

I rebased the branch and added python-mock to the .travis.yaml. It seems OK now.

scpeters commented 8 years ago

+1

jacquelinekay commented 8 years ago

Wouldn't it be even simpler to change the check_valid function of Material to pass? I believe Link is the only element of URDF that can have a material sub-element.

https://github.com/ros/urdfdom/pull/72/files#diff-034f34cdc2bfe8d50103d95f81bce8e0R163

otamachan commented 8 years ago

It's not documented (http://wiki.ros.org/urdf/XML/model), but a robot element also seems to be able to have a material element.

https://github.com/ros/urdfdom/blob/master/urdf_parser_py/src/urdf_parser_py/urdf.py#L476 https://github.com/ros/urdfdom/blob/master/urdf_parser/src/model.cpp#L96

jacquelinekay commented 8 years ago

That's a good point. That's actually referenced in #39, which builds on this feature... And the C++ urdf_parser parses Material as an element of a robot model:

https://github.com/ros/urdfdom/blob/master/urdf_parser/src/model.cpp#L96

Additionally, when the material element is parsed for the model, it uses the same parseMaterial function as the link:

https://github.com/ros/urdfdom/blob/master/urdf_parser/src/link.cpp#L51

So, to be consistent with the C++ implementation, I still think you don't need to create a new LinkMaterial class.

otamachan commented 8 years ago

Yes, I agree that to invalidate the check_valid method in the Material class is simpler and consistent. But in that case I could not find an implementation to pass this test case.

So I extended the Link class to invalidate check_valid only when it is inside the Link element.

Is it acceptable to remove that test case? I mean is it ok that even if neither a color nor texture in the material of the robot element, no error will be reported? If it's acceptable, I will revise my PR soon.

jacquelinekay commented 8 years ago

Actually, after looking more closely at the C++ implementation of parseMaterial, it looks like the case where the material is a sub-element of robot is consistent with your implementation:

https://github.com/ros/urdfdom/blob/master/urdf_parser/src/link.cpp#L51

So you were correct all along! Thanks for the contribution.

I'll look into the procedure for changing the ROS Wiki to document this, since the spec has nothing to say about it.

(Sorry about all the confusion... new maintainer >_>)

otamachan commented 8 years ago

No problem! Thank you for your review.