assign correct MaterialPtr to all visuals #114

rhaschke · 2018-10-31T09:40:01Z

So far, only the first visual in visual_array got the correct MaterialPtr (from the model) assigned.
All other visual had a default-constructed Material with black color.
This required a work around in rviz, to figure out the correct material colors (#1079 or ros-visualization/rviz@ed3b0b3)

So far, only the first visual in visual_array got the correct MaterialPtr assigned.

clalancette · 2018-11-06T15:48:39Z

urdf_parser/src/model.cpp

+    else
+    {
+      CONSOLE_BRIDGE_logError("link '%s' material '%s' undefined.", link_name,visual->material_name.c_str());
+      model.reset();


CI is failing because you can't call reset on a const reference. This looks like it should probably be a non-const reference.

I'm sorry. Obviously I filed this PR in a rush. There was also a missing return value. Both issues should be fixed now.

There is one more open issue: The new parser is more picky about undefined materials than the old parser (which is good). However, I suggest to issue a warning only instead of an error.

Sure, that sounds good to me.

I ended up pushing 8175b16, which does some simple style cleanups. However, what I want to talk about here is the fact that the new code actually seems less picky about undefined materials. That is, it used to be that if the material was undefined, then the parser would return a nullptr from parseURDF. The new code, while it downgrades the print from an error to a warning, lets the code go on. In other words, the boolean return value from assignMaterial seems to be ignored. @rhaschke is this the intended behavior, and if so, could you say a bit about why this is safe/better to do here? Thanks.

eisoku9618 · 2018-12-09T11:52:16Z

+1

Could you merge and release this?

urdf_parser/src/link.cpp

urdf_parser/src/model.cpp

Signed-off-by: Chris Lalancette <[email protected]>

rhaschke

Sorry @clalancette, I should have explained the situation in more detail. The old parsing was less picky in the sense that it didn't recognized references to undefined materials (see inline github comments for details).
As an example consider this example urdf, which defines materials Black and Orange, but also uses Grey. The old parser doesn't complain about the missing Grey definition - which can be easily validated with check_urdf.
The new parser, however, complains about the issue. Because this more picky behaviour might break many existing urdf files (for example the Shadow Robot hand), I turned the error into a warning and don't bail out.

rhaschke · 2019-01-18T16:22:48Z

urdf_parser/src/link.cpp

@@ -381,7 +381,7 @@ bool parseVisual(Visual &vis, TiXmlElement *config)
    vis.material.reset(new Material());
    if (!parseMaterial(*vis.material, mat, true))
    {
-      CONSOLE_BRIDGE_logDebug("urdfdom: material has only name, actual material definition may be in the model");
+      vis.material.reset();


Before the visual's material pointer was always well defined.

rhaschke · 2019-01-18T16:23:52Z

urdf_parser/src/model.cpp

-              }
-              else
-              {
-                CONSOLE_BRIDGE_logError("link '%s' material '%s' undefined.", link->name.c_str(),link->visual->material_name.c_str());


Because the visual's material was always well-defined, this error was never printed.

clalancette · 2019-01-18T18:10:58Z

Sorry @clalancette, I should have explained the situation in more detail. The old parsing was less picky in the sense that it didn't recognized references to undefined materials (see inline github comments for details).
As an example consider this example urdf, which defines materials Black and Orange, but also uses Grey. The old parser doesn't complain about the missing Grey definition - which can be easily validated with check_urdf.
The new parser, however, complains about the issue. Because this more picky behaviour might break many existing urdf files (for example the Shadow Robot hand), I turned the error into a warning and don't bail out.

This makes sense to me. Thanks for the detailed explanation.

rhaschke · 2019-01-30T23:27:40Z

Ping @scpeters, @clalancette. Can we merge this bug fix and get that a new release anytime soon?
I think this is also related to Chris' cleanup in #104. It's so sad that the latter PR is already dangling there for more than a year.

rhaschke · 2019-01-30T23:31:26Z

Huh. That was a quick response! Thanks. What is your release schedule for this non-ROS lib? Is there a chance to see a new release in Bionic already?

scpeters · 2019-01-30T23:33:41Z

I made a 1.0.3 release yesterday which should be able to get into the new debian without trouble, but it's hard to get releases into bionic that aren't security fixes. cc @j-rivero

rhaschke · 2019-01-30T23:53:15Z

should be able to get into the new debian

What do you mean by this exactly? A .deb package or the Debian distribution?
How would the newly released lib enter my ROS ecosystem? Is it released via the ROS release / distribution mechanism, i.e. bloom too?

In urdf::Link, only the first visual's material comprises actual color values, all others only have the name. Thus to correctly show the color of all visuals, we need to search for the first visual with given material name, much like ros-visualization#1079 has done. If the urdfdom fix (ros/urdfdom#114) is released, this work-around can be reverted.

assign correct MaterialPtr to all visuals

1939751

So far, only the first visual in visual_array got the correct MaterialPtr assigned.

clalancette reviewed Nov 6, 2018

View reviewed changes

fix errors

b53fef5

rhaschke force-pushed the fix-material-assignment branch from 69b7c56 to b53fef5 Compare November 6, 2018 19:35

tolerate undefined materials with a warning

93a2bf4

rhaschke force-pushed the fix-material-assignment branch from be9e954 to 93a2bf4 Compare November 14, 2018 00:40

eisoku9618 reviewed Dec 9, 2018

View reviewed changes

urdf_parser/src/link.cpp Show resolved Hide resolved

eisoku9618 reviewed Dec 9, 2018

View reviewed changes

urdf_parser/src/model.cpp Show resolved Hide resolved

Minor style fixups.

8175b16

Signed-off-by: Chris Lalancette <[email protected]>

rhaschke commented Jan 18, 2019

View reviewed changes

clalancette approved these changes Jan 18, 2019

View reviewed changes

scpeters merged commit 3ffc4ee into ros:master Jan 30, 2019

scpeters mentioned this pull request Jan 30, 2019

Cleanup error checking in urdfdom #104

Open

rhaschke deleted the fix-material-assignment branch January 30, 2019 23:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

assign correct MaterialPtr to all visuals #114

assign correct MaterialPtr to all visuals #114

rhaschke commented Oct 31, 2018

clalancette Nov 6, 2018

rhaschke Nov 6, 2018

rhaschke Nov 12, 2018

clalancette Nov 13, 2018

rhaschke Nov 14, 2018

clalancette Jan 18, 2019

eisoku9618 commented Dec 9, 2018

rhaschke left a comment

rhaschke Jan 18, 2019

rhaschke Jan 18, 2019

clalancette commented Jan 18, 2019

rhaschke commented Jan 30, 2019

rhaschke commented Jan 30, 2019

scpeters commented Jan 30, 2019

rhaschke commented Jan 30, 2019

assign correct MaterialPtr to all visuals #114

assign correct MaterialPtr to all visuals #114

Conversation

rhaschke commented Oct 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eisoku9618 commented Dec 9, 2018

rhaschke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clalancette commented Jan 18, 2019

rhaschke commented Jan 30, 2019

rhaschke commented Jan 30, 2019

scpeters commented Jan 30, 2019

rhaschke commented Jan 30, 2019