Add in support for a version tag. by clalancette · Pull Request #133 · ros/urdfdom

clalancette · 2019-11-15T13:22:23Z

This is already specified by the xsd, so just make sure we
implement it and enforce it in the code.

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

This is already specified by the xsd, so just make sure we implement it and enforce it in the code. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

scpeters · 2019-11-21T00:46:19Z

looks good, just one minor suggestion about the error message

traversaro · 2019-11-21T07:55:16Z

urdf_parser/src/model.cpp

  model->name_ = std::string(name);

+  float version = -1.0;
+  int version_ret = robot_xml->QueryFloatAttribute("version", &version);


QueryFloatAttribute uses sscanf for parsing the string to a float, so I am afraid it is not locale-safe and it will not behave correctly on system with a locale that does not use . as decimal separator, see ros/urdfdom_headers#41 and #98 .

Arg, you are right, of course. Thanks for pointing this out. But I'm going to address the bigger issue of how to store the version number from @scpeters before doing anything here.

scpeters · 2019-11-21T21:15:12Z

I thought a little more about this, and do we want to parse to a float to store a version number? It won't be able to distinguish between 1.1 and 1.10

clalancette · 2019-11-22T13:59:50Z

I thought a little more about this, and do we want to parse to a float to store a version number? It won't be able to distinguish between 1.1 and 1.10

Hm, good point. It also has the system locale problems pointed out.

I chose a float to store it since it would make comparisons (==, <, <=, >, >=) easy. Storing it as a string won't give us that property. I guess we could do a custom class where we parse the string into the part before and after the dot, then implement comparison operators on that class. What do you think of that proposal?

traversaro · 2019-11-22T14:40:19Z

I thought a little more about this, and do we want to parse to a float to store a version number? It won't be able to distinguish between 1.1 and 1.10

Indeed, in the .xsd contained in the repo the type of the version attribute was already xs:string :

urdfdom/xsd/urdf.xsd

Line 321 in 06f5f9b

<xs:attribute name="version" type="xs:string" default="1.0" />

.

scpeters · 2019-11-22T17:39:04Z

it would be great if c++ had a primitive type for version numbers, but without that a custom version number class sounds good to me. we already have a split_string function in urdfdom_headers:

https://github.com/ros/urdfdom_headers/blob/master/urdf_model/include/urdf_model/utils.h#L48-L66

scpeters · 2019-11-22T17:39:33Z

should we add that class to urdfdom_headers or here?

The previous attempt used a float to store the version number. That has two problems: 1. It runs into locale issues, and 2. The XSD specifies it as a string This alternative introduces a class to do parsing of the version string. Besides conforming to the XSD, and getting us away from the locale issues, it also allows us to provide additional constraints on the version (like not allowing negative numbers). The UDFVersion class ends up living in the header file so we can write tests against it. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette · 2019-12-05T21:09:25Z

So I just pushed commit 088bf6c, which implements a class for URDF version parsing. A few things to note:

I left it in this repository (instead of urdfdom_headers as @scpeters proposed) because we currently don't need it outside of here. I guess we could eventually move it out of here, but I don't see the need right now.
I didn't implement overloaded operators on the class. The only meaningful thing we could do with that is compare one class instance to another, and I thought it was easier to just implement an equals() method on the class where you could pass integers.
I didn't implement anything but equals since we don't currently need anything else. We can add additional "operators" as needed.
I ended up making the allowed version numbers more strict.
I believe the hand parsing we are doing here should resolve the locale issues pointed out earlier, but verification of this would be great.

@scpeters @sloretz @traversaro Please take another look. Thanks!

urdf_parser/include/urdf_parser/urdf_parser.h

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

scpeters · 2019-12-05T21:51:49Z

the equal() operator is fine while we only have one version, but I could imagine having some use for at_least and at_most functions (inspired by macros in dartsim config.hpp) once there are more versions to check against

clalancette · 2019-12-05T22:04:22Z

the equal() operator is fine while we only have one version, but I could imagine having some use for at_least and at_most functions (inspired by macros in dartsim config.hpp) once there are more versions to check against

Right. I didn't implement them since there are no callers to them, and I don't like adding code that isn't currently used. I just figured we would implement them as we needed them. Alternatively, I guess I could add them in and just write tests for them. What do you think?

scpeters · 2019-12-05T22:20:34Z

if the plan is to add them in the future when we have multiple versions, I guess that's fine

sloretz · 2020-01-13T21:02:12Z

urdf_parser/include/urdf_parser/urdf_parser.h

+    }
+
+    // We accept any of the following strings:
+    // 1


Is this comment correct? It looks like the code below will throw runtime_error if the version is 1.

Oops, that's an outdated comment. I can open a PR to fix that.

ros-discourse · 2023-03-24T16:48:52Z

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/urdf-improvements/30520/6

ros-discourse · 2025-03-27T09:31:35Z

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/proposal-to-extend-jointlimits-in-urdf/42831/2

Add in support for a version tag.

aaa51ea

This is already specified by the xsd, so just make sure we implement it and enforce it in the code. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette mentioned this pull request Nov 15, 2019

Add in support for the version tag. ros/urdf_parser_py#52

Merged

traversaro reviewed Nov 21, 2019

View reviewed changes

scpeters reviewed Dec 5, 2019

View reviewed changes

urdf_parser/include/urdf_parser/urdf_parser.h Outdated Show resolved Hide resolved

Remove bogus comment.

5ee1650

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

scpeters approved these changes Dec 6, 2019

View reviewed changes

scpeters merged commit 1aef805 into master Jan 13, 2020

scpeters deleted the version-tag branch January 13, 2020 19:03

sloretz reviewed Jan 13, 2020

View reviewed changes

clalancette mentioned this pull request Feb 18, 2020

Forking URDFDom? ros/urdfdom_headers#59

Open

traversaro mentioned this pull request May 1, 2020

Move URDF specification to a Github repo? ros/urdfdom_headers#62

Open

traversaro mentioned this pull request Jan 20, 2024

Add support for quaternion in URDF #195

Open

scpeters mentioned this pull request Dec 5, 2025

Schema versioning strategy #230

Open

Conversation

clalancette commented Nov 15, 2019

Uh oh!

scpeters commented Nov 21, 2019

Uh oh!

traversaro Nov 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clalancette Nov 22, 2019

Choose a reason for hiding this comment

Uh oh!

scpeters commented Nov 21, 2019

Uh oh!

clalancette commented Nov 22, 2019

Uh oh!

traversaro commented Nov 22, 2019

Uh oh!

scpeters commented Nov 22, 2019

Uh oh!

scpeters commented Nov 22, 2019

Uh oh!

clalancette commented Dec 5, 2019

Uh oh!

Uh oh!

scpeters commented Dec 5, 2019

Uh oh!

clalancette commented Dec 5, 2019

Uh oh!

scpeters commented Dec 5, 2019

Uh oh!

sloretz Jan 13, 2020

Choose a reason for hiding this comment

Uh oh!

clalancette Jan 13, 2020

Choose a reason for hiding this comment

Uh oh!

clalancette Jan 13, 2020

Choose a reason for hiding this comment

Uh oh!

ros-discourse commented Mar 24, 2023

Uh oh!

ros-discourse commented Mar 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

traversaro Nov 21, 2019 •

edited

Loading