Skip to content

Allow name attribute in visual tag - #31

Merged
sloretz merged 4 commits into
ros:melodic-develfrom
gerkey:allow_name_in_visual
Mar 9, 2020
Merged

Allow name attribute in visual tag#31
sloretz merged 4 commits into
ros:melodic-develfrom
gerkey:allow_name_in_visual

Conversation

@gerkey

@gerkey gerkey commented May 18, 2018

Copy link
Copy Markdown

With some URDF like this:

    <visual name="tire">
      <origin xyz="0 0.06 0" rpy="${M_PI/2} 0 0" />
      <geometry>
        <mesh filename="${mesh_dir}rp1a_wheel.dae" />
      </geometry>
      <material name="MidGrey"/>
    </visual>

I get a warning like this:

Unknown attribute "name" in /robot[@name='resource_prospector']/link[@name='front_right_wheel_link']/visual[1]

According to the docs, it's valid to have a name attribute in a visual tag:
http://wiki.ros.org/urdf/XML/link#Elements

In this PR I made changes that I think are in the right direction to add support for that attribute. At least, with these changes, the warning disappears. But I must admit that I'm not certain that I've done the right thing here.

It might also be a good idea to update the schema: https://github.com/ros/urdfdom/blob/master/xsd/urdf.xsd#L123-L133, but I'm not sure whether that's necessary.

@ri-ceres

Copy link
Copy Markdown

Is there any reason why this shouldn't be merged? The missing name attribute breaks using the parser to find elements by name in Melodic, which previously worked in Kinetic.

@clalancette

Copy link
Copy Markdown
Contributor

Is there any reason why this shouldn't be merged? The missing name attribute breaks using the parser to find elements by name in Melodic, which previously worked in Kinetic.

There are a couple of things here:

  1. As-is, this patch breaks the API by adding an attribute in the middle of the argument list of the Visual constructor. This can be easily rectified by moving that to the end, however.
  2. I don't see how this worked in Kinetic. The code around here is very similar, and we definitely didn't remove the 'name' attribute between Kinetic and Melodic, so I don't see how this could be a regression.

@ri-ceres Any thoughts on how this might have worked in Kinetic?

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz

sloretz commented Mar 9, 2020

Copy link
Copy Markdown
Contributor

As-is, this patch breaks the API by adding an attribute in the middle of the argument list of the Visual constructor. This can be easily rectified by moving that to the end, however.

Moved to end in 624bc73

sloretz added 2 commits March 9, 2020 14:59
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
sloretz merged commit 3db6eab into ros:melodic-devel Mar 9, 2020
traversaro pushed a commit to traversaro/urdf_parser_py that referenced this pull request Nov 8, 2024
* Allow name attribute in visual tag

* Add name kwarg at end

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Add test for <visual name="...">

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Add version attribute to test

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Co-authored-by: Brian Gerkey <gerkey@openrobotics.org>
Co-authored-by: Shane Loretz <sloretz@osrfoundation.org>
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.

4 participants