Skip to content

fix error handling in material parsing - #122

Open
rhaschke wants to merge 6 commits into
ros:rollingfrom
ubi-agni:tolerant-material-parsing
Open

fix error handling in material parsing#122
rhaschke wants to merge 6 commits into
ros:rollingfrom
ubi-agni:tolerant-material-parsing

Conversation

@rhaschke

@rhaschke rhaschke commented Feb 8, 2019

Copy link
Copy Markdown
Contributor

We should consider the return value of parseMaterial() to react to plain parsing errors.
Redefining the same material again should be accepted though. This often occurs when composing URDFs via xacro.

@rhaschke

rhaschke commented Feb 8, 2019

Copy link
Copy Markdown
Contributor Author

@clalancette I just noticed that you also worked on more restrictive material parsing in #104.
I will rebase my change onto your cleanup branch.

@rhaschke
rhaschke force-pushed the tolerant-material-parsing branch from beb1e4e to 9ebb53a Compare February 8, 2019 11:27
@rhaschke

rhaschke commented Feb 8, 2019

Copy link
Copy Markdown
Contributor Author

@scpeters Would be great to get #104 and this extension merged (and released).

@rhaschke

rhaschke commented Jun 20, 2019

Copy link
Copy Markdown
Contributor Author

Ping

@rhaschke
rhaschke force-pushed the tolerant-material-parsing branch from 9ebb53a to e787fc8 Compare September 21, 2022 11:54
@rhaschke

rhaschke commented Sep 21, 2022

Copy link
Copy Markdown
Contributor Author

@scpeters, @clalancette, @sloretz
I remember that we discussed at some ROSCon, years ago, whether to still improve urdfdom. While at that time urdfdom was claimed to be dead and should be replaced with some alternative, e.g. Gazebo's file format, it turns out that the package has seen lots of releases in past years (both into ROS1 and ROS2) and is still very alive.
Would be great to see the list of pending PRs like this getting merged (or at least discussed).

@rhaschke
rhaschke force-pushed the tolerant-material-parsing branch from e787fc8 to c5eaa16 Compare September 22, 2022 12:36
clalancette and others added 5 commits April 10, 2026 14:33
@clalancette @rhaschke
Cleanup error checking in model.cpp.
2e9aaca 
There are 2 main things done here:

1.  Check the return value from parseMaterial, parseLink, and
    parseJoint.  While these can throw a ParseError exception,
    most often they return false when they fail to parse.
2.  Revamp the error checking around those calls so that the
    code is indented a lot less.  It is much easier to read this way.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette @rhaschke
Slightly cleanup link.cpp.
0ca9f22 
Just make the code flow easier to understand.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette @rhaschke
Add in tests for new, more restrictive parsing.
f7b6caf 
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette @rhaschke
Quiet down the tests.
f8b1870 
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@rhaschke
Fix error handling in material parsing
44bfb21 
We should consider the return value of parseMaterial().
Redefining the same material again, should be accepted.
This often occurs when composing URDFs via xacro.

Signed-off-by: Robert Haschke <rhaschke@techfak.uni-bielefeld.de>
@rhaschke
rhaschke force-pushed the tolerant-material-parsing branch from c5eaa16 to 44bfb21 Compare April 10, 2026 12:33
@rhaschke
rhaschke changed the base branch from master to rolling April 10, 2026 12:36
@rhaschke

rhaschke commented Apr 10, 2026

Copy link
Copy Markdown
Contributor Author

I rebased this PR onto rolling. I hope it can be merged after 7 years ;-)

saikishor
saikishor suggested changes Apr 10, 2026
View reviewed changes

@saikishor saikishor left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some minor nitpicks

Comment thread urdf_parser/src/model.cpp
catch (ParseError &/*e*/) {
success = parseMaterial(*material, material_xml, false); // material needs to be fully defined here
} catch(ParseError & /*e*/) {
CONSOLE_BRIDGE_logError("material xml is not initialized correctly");

@saikishor saikishor Apr 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
CONSOLE_BRIDGE_logError("material xml is not initialized correctly");
CONSOLE_BRIDGE_logError("Exception: material xml is not initialized correctly");

@rhaschke rhaschke Apr 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I didn't change the original error message for consistency. If you want to change it anyway, I suggest a more useful message:

Suggested change
CONSOLE_BRIDGE_logError("material xml is not initialized correctly");
CONSOLE_BRIDGE_logError("Error parsing material specification.");

The information that the error is originating from an exception (rather than the return value) is not relevant to a user, I think.

Comment thread urdf_parser/test/urdf_unit_test.cpp
Comment on lines -30 to -40
std::ostream &operator<<(std::ostream &os, const urdf::Rotation& rot)
{
double roll, pitch, yaw;
double x, y, z, w;
rot.getRPY(roll, pitch, yaw);
rot.getQuaternion(x, y, z, w);
os << std::setprecision(9)
<< "x: " << x << " y: " << y << " z: " << z << " w: " << w
<< " roll: " << roll << " pitch: " << pitch << " yaw: "<< yaw;
return os;
}

@saikishor saikishor Apr 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Any reason to remove this?

@rhaschke rhaschke Apr 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This was part of @clalancette's cleanup branch, which I built upon. The commit message and motivation was: "quiet down the tests" 😉

Comment thread urdf_parser/test/urdf_unit_test.cpp
Comment on lines -69 to -74
if (!ok) {
std::cout << "initial rpy: " << r << " " << p << " " << y << "\n"
<< "before " << rot0 << "\n"
<< "after " << rot1 << "\n"
<< "ok " << ok << "\n";
}

@saikishor saikishor Apr 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same here

Comment thread urdf_parser/test/urdf_unit_test.cpp Outdated
Comment thread urdf_parser/test/urdf_unit_test.cpp
Comment thread urdf_parser/test/urdf_unit_test.cpp Outdated
Comment thread urdf_parser/src/model.cpp
catch (ParseError &/*e*/) {

if (!success) {
CONSOLE_BRIDGE_logError("link xml is not initialized correctly");

@saikishor saikishor Apr 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
CONSOLE_BRIDGE_logError("link xml is not initialized correctly");
CONSOLE_BRIDGE_logError("link : '%s' xml is not initialized correctly", link->name.c_str());

@rhaschke rhaschke Apr 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

link is probably not well-defined here. Thus accessing its name will cause a segfault.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@saikishor saikishor saikishor requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@rhaschke @saikishor @clalancette