Make urdf plugable and revive urdf_parser_plugin #13

sloretz · 2020-07-15T18:27:16Z

This makes the urdf package load parser plugins, so it can use collada or sdformat parser plugins in the future.
It also makes the urdf xml parser a plugin instead of a special case.

A new api might_handle() returns a score for how likely it is a given string is meant for the plugin.
Right now the implementation is very simple, and a bit fragile.
The URDF plugin uses the position of the text <robot as the score.

This breaks ABI with urdf::Model because it now stores the class
loader instance, so everything downstream of this change must be rebuilt.

@scpeters @IanTheEngineer FYI

Requires ros/pluginlib#199

This makes the urdf package load parser plugins. It includes a new api might_handle() that returns a score for how likely the plugin is to be the one the given file format is meant for. This change also makes the urdf xml parser a plugin instead of a special case that is called directly. This breaks ABI with urdf::Model because the model now stores the class loader instance. Signed-off-by: Shane Loretz<[email protected]> Signed-off-by: Shane Loretz <[email protected]>

ros-discourse · 2020-07-18T01:14:12Z

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

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2020-07-16/15468/1

Signed-off-by: Shane Loretz <[email protected]>

clalancette

I did a first pass of review, please take a look.

This package looks like it needs a lot of cleanup work in addition to what you've done here. It's all orthogonal, but while we are looking, can you open up follow-up issues to deal with:

Merging https://github.com/ros2/urdf and https://github.com/ros/urdf
Replacing the use of fprintf here with something else (maybe console_bridge?)
Removing the deprecated APIs
Deleting/finishing commented-out code

Thanks.

urdf/include/urdf/model.h

urdf/package.xml

urdf/src/model.cpp

urdf/src/urdf_plugin.cpp

urdf_parser_plugin/CMakeLists.txt

urdf_parser_plugin/include/urdf_parser_plugin/parser.h

urdf_parser_plugin/package.xml

Signed-off-by: Shane Loretz <[email protected]>

Signed-off-by: Shane Loretz <[email protected]> Co-authored-by: Chris Lalancette <[email protected]>

Signed-off-by: Shane Loretz <[email protected]>

…os2-make_urdf_plugable

Signed-off-by: Shane Loretz <[email protected]>

sloretz · 2020-08-05T23:14:27Z

@clalancette I've addressed all comments :) . Mind having a second look?

urdf/package.xml

urdf/src/urdf_plugin.cpp

urdf_parser_plugin/include/urdf_parser_plugin/parser.h

urdf_parser_plugin/package.xml

Signed-off-by: Shane Loretz <[email protected]>

sloretz · 2020-08-19T15:36:38Z

CI one more time after (hopefully) fixing test issue on OSX (build: --packages-up-to urdf urdf_parser_plugin test: --packages-select urdf urdf_parser_plugin)

Linux
Linux-aarch64
macOS
Windows

Fixes OSX failing build ros/pluginlib#200 Signed-off-by: Shane Loretz<[email protected]> Signed-off-by: Shane Loretz <[email protected]>

sloretz · 2020-08-19T18:51:30Z

CI again, this time I'm sure I've fixed the OSX issue

Linux
Linux-aarch64
macOS
Windows

sloretz · 2020-08-19T19:00:11Z

@clalancette Mind having a look at the changes since your last review? 🙇

clalancette

One question here about the performance test.

clalancette · 2020-08-19T19:39:50Z

urdf/CMakeLists.txt

+  # Write files to enable pluginlib to find urdf_xml_parser in build folder
+  set(fake_urdf_prefix "${CMAKE_CURRENT_BINARY_DIR}/fake_urdf_install")
+  # Copy urdf_xml_parser to fake prefix lib/ and bin/
+  file(MAKE_DIRECTORY "${fake_urdf_prefix}/lib")
+  file(MAKE_DIRECTORY "${fake_urdf_prefix}/bin")
+  add_custom_command(TARGET urdf_xml_parser
+    POST_BUILD
+    COMMAND ${CMAKE_COMMAND} -E copy $<TARGET_FILE:urdf_xml_parser>
+    "${fake_urdf_prefix}/lib/"
+  )
+  add_custom_command(TARGET urdf_xml_parser
+    POST_BUILD
+    COMMAND ${CMAKE_COMMAND} -E copy $<TARGET_FILE:urdf_xml_parser>
+    "${fake_urdf_prefix}/bin/"
+  )
+  # Fake install plugin description file
+  file(COPY "urdf_parser_description.xml" DESTINATION "${fake_urdf_prefix}/share/urdf/")
+  # Fake install package.xml
+  configure_file("package.xml" "${fake_urdf_prefix}/share/urdf/package.xml" COPYONLY)
+  # Fake install entry in ament_index telling pluginlib where to find plugin description
+  file(WRITE "${fake_urdf_prefix}/share/ament_index/resource_index/urdf_parser_plugin__pluginlib__plugin/urdf"
+    "share/urdf/urdf_parser_description.xml\n")
+  # Fake install entry in ament_index declaring this a package
+  file(WRITE "${fake_urdf_prefix}/share/ament_index/resource_index/packages/urdf" "")


Well, yuck. If I'm understanding this properly, this is more-or-less recreating the install directory structure under build. I guess its OK, but it seems pretty fragile to me; if we change the ament structure for any reason, this is going to break.

How valuable do you think this performance test is? I'm wondering if it would be more straightforward to not include it at all.

Well, yuck. If I'm understanding this properly, this is more-or-less recreating the install directory structure under build. I guess its OK, but it seems pretty fragile to me; if we change the ament structure for any reason, this is going to break.

Yeah. It will break if how pluginlib uses the ament index is changed. I copied this logic from pluginlib's own tests. A cmake function provided by pluginlib that implemented this would eliminate the duplication if something does change

https://github.com/ros/pluginlib/blob/7c3e7d5b7af6d776079e2e9cba2ac9a5581777b0/pluginlib/CMakeLists.txt#L86-L94

How valuable do you think this performance test is? I'm wondering if it would be more straightforward to not include it at all.

It's straight forward to delete it, though it does give valuable info about the overhead of the plugin infrastructure, and it does prove the plugin infrastructure works. I would rather robustify it than delete it.

That's a valid point @clalancette, these performance tests we just a guide to make sure that the new plugin-based file type check weren't introducing large amounts of overhead. While no one should be parsing URDF/SDF inside their control loop, we don't want this plugin structure to have a substantial impact on existing code execution times.

I think it would be fine to refrain from including this install deployment. This code was more-or-less intended as a one-off verification that the plugin checking times were reasonable. @sloretz can you recap the results of the tests here? Would they be impacted at all by the size of the URDF/SDFormat file being parsed?

I think it would be fine to refrain from including this install deployment. This code was more-or-less intended as a one-off verification that the plugin checking times were reasonable.

I would recommend keeping it as it's the only test that makes sure the urdf plugin is loadable.

@sloretz can you recap the results of the tests here?

Linux amd64

direct - 0.015 ms

plugin - 0.743 ms

Linux aarch64

direct - 0.045 ms

plugin - 3.009 ms

OSX

direct - 0.012 ms

plugin - 2.371 ms

Windows

direct - 0.017 ms

plugin - 1.970 ms

Would they be impacted at all by the size of the URDF/SDFormat file being parsed?

Yes since cb348db asks tinyxml2 to parse the whole document to decide the confidence. See discussion on #13 (comment) . The size dependent overhead would apply times every plugin with a similar score plugin - ros/sdformat_urdf#1 (comment)

3341988 uses a new pluginlib CMake function that enables testing plugins in the same package that built them: ros/pluginlib#201

IanTheEngineer

I think this is a valuable addition for the community @sloretz. Thanks for the PR. I'm sure @gavanderhoorn would appreciate the fact that you implemented his confidence suggestion from the original SDFormat support proposal: ros#34 (comment)

IanTheEngineer · 2020-08-19T21:03:44Z

urdf/CMakeLists.txt

+  # Write files to enable pluginlib to find urdf_xml_parser in build folder
+  set(fake_urdf_prefix "${CMAKE_CURRENT_BINARY_DIR}/fake_urdf_install")
+  # Copy urdf_xml_parser to fake prefix lib/ and bin/
+  file(MAKE_DIRECTORY "${fake_urdf_prefix}/lib")
+  file(MAKE_DIRECTORY "${fake_urdf_prefix}/bin")
+  add_custom_command(TARGET urdf_xml_parser
+    POST_BUILD
+    COMMAND ${CMAKE_COMMAND} -E copy $<TARGET_FILE:urdf_xml_parser>
+    "${fake_urdf_prefix}/lib/"
+  )
+  add_custom_command(TARGET urdf_xml_parser
+    POST_BUILD
+    COMMAND ${CMAKE_COMMAND} -E copy $<TARGET_FILE:urdf_xml_parser>
+    "${fake_urdf_prefix}/bin/"
+  )
+  # Fake install plugin description file
+  file(COPY "urdf_parser_description.xml" DESTINATION "${fake_urdf_prefix}/share/urdf/")
+  # Fake install package.xml
+  configure_file("package.xml" "${fake_urdf_prefix}/share/urdf/package.xml" COPYONLY)
+  # Fake install entry in ament_index telling pluginlib where to find plugin description
+  file(WRITE "${fake_urdf_prefix}/share/ament_index/resource_index/urdf_parser_plugin__pluginlib__plugin/urdf"
+    "share/urdf/urdf_parser_description.xml\n")
+  # Fake install entry in ament_index declaring this a package
+  file(WRITE "${fake_urdf_prefix}/share/ament_index/resource_index/packages/urdf" "")


That's a valid point @clalancette, these performance tests we just a guide to make sure that the new plugin-based file type check weren't introducing large amounts of overhead. While no one should be parsing URDF/SDF inside their control loop, we don't want this plugin structure to have a substantial impact on existing code execution times.

I think it would be fine to refrain from including this install deployment. This code was more-or-less intended as a one-off verification that the plugin checking times were reasonable. @sloretz can you recap the results of the tests here? Would they be impacted at all by the size of the URDF/SDFormat file being parsed?

urdf/include/urdf/model.h

urdf/src/model.cpp

IanTheEngineer · 2020-08-19T21:20:16Z

urdf_parser_plugin/include/urdf_parser_plugin/parser.h

+  /// \brief Indicate if data is meant to be parsed by this parser
+  /// \return confidence that the data is meant for this parser, where smaller
+  ///         values mean more confidence, 0 is absolute certainty, and
+  ///         std::numeric_limits<size_t>::max() is no confidence.


My intuition would be that a score of 1.0 is absolute certainty and 0.0 is no confidence (treating this as a probability), but that would require floating point comparisons. Is there a reason to avoid floating point?

I would have trouble thinking what a score like 1000 would mean in this context. To back out a probability from this score would be something like 1-(x/std::numeric_limits<size_t>::max()) and plugging in our hypothetical 1000 in for x?

Is there a reason to avoid floating point?

No reason to avoid floating point.

(treating this as a probability)

I don't think the score can be treated as a probability. One could take a bunch of robot descriptions, run them through the might_handle() function, and count the number of URDF or non-URDF files in a range of scores. Then one could say "documents that scored between 29 and 42 were URDF files 50% of the time".

I would have trouble thinking what a score like 1000 would mean in this context.

I don't think 1000 would mean anything on its own; it only has meaning when compared against other scores. If someone scores 10 free throws in a minute, are they an NBA basketball player? I don't think that has an answer. Now say there are two people and one scores 10 free throws in a minute while the other scores 2 - which one is more likely to be an NBA player? The latter is more like what's happening here (with a caveat that the free throw test isn't the same for each plugin since each has it's own might_handle() implementation).

(with a caveat that the free throw test isn't the same for each plugin since each has it's own might_handle() implementation).

I think this is where the intuition for a probability comes in. What seems most likely is that plugins are either going to score a 0 for "yes" or std::numeric_limits<size_t>::max() for "no", at which point anything in between will be a best guess.

What seems most likely is that plugins are either going to score a 0 for "yes" or std::numeric_limits<size_t>::max() for "no", at which point anything in between will be a best guess.

Yeah, the scores less than std::numeric_limits<size_t>::max() are a bit arbitrary. Maybe it would be more intuitive if the documentation was "return the position in the string where you became sure this document was for you"?

I think this is where the intuition for a probability comes in. What seems most likely is that plugins are either going to score a 0 for "yes" or std::numeric_limits<size_t>::max() for "no", at which point anything in between will be a best guess.

Yeah, and not only that, the writer of a new plugin won't know whether they'll be scored "better" than the other plugins without reading all other available plugins. In which case, this regresses towards the binary 0/1 . I'm now rethinking the whole plan to return anything but a boolean :). That being said, it isn't something I would block on, though a more constrained range and/or better documentation on what exactly to return would be helpful.

69b3d49 defines the score range being the position in the document the parser became confident the format was for it, and 025f816 makes the urdf plugin return data.size() instead of the max size_t value.

From offline conversation on this topic, 31014c0 makes urdf::Model try the urdf plugin anyways if no plugin things the file is for it.

Signed-off-by: Shane Loretz <[email protected]>

Signed-off-by: Shane Loretz<[email protected]> Signed-off-by: Shane Loretz <[email protected]>

Signed-off-by: Shane Loretz <[email protected]>

clalancette

I've got a few more open comments.

clalancette · 2020-08-31T18:49:59Z

urdf/src/model.cpp


 namespace urdf
 {
+class ModelImplementation


Nit: this can be a final class, since nothing will be deriving from it.

Final in 06474e8

clalancette · 2020-08-31T18:54:39Z

urdf/src/model.cpp

-     ROS_ERROR_STREAM("Exception while creating planning plugin loader " << ex.what() << ". Will not parse Collada file.");
-   }
+  size_t best_score = std::numeric_limits<size_t>::max();
+  pluginlib::UniquePtr<urdf::URDFParser> best_plugin;


There is some possibility that no plugin matches, in which case we wouldn't assign anything to best_plugin until after the for loop. I think the "no confidence" logic below that currently prevents us from accessing anything bad, but I'd still feel better if we explicitly assigned this to nullptr here.

Explicit initialize to nullptr in f9fe6f9

urdf/src/model.cpp

Signed-off-by: Shane Loretz <[email protected]>

sloretz · 2020-09-18T18:29:35Z

@clalancette May I ask for another look? There are new changes since your last review 🙇

sloretz · 2020-09-18T18:51:12Z

CI (build: --packages-up-to urdf urdf_parser_plugin test: --packages-select urdf urdf_parser_plugin)

Linux
Linux-aarch64
macOS
Windows

sloretz self-assigned this Jul 15, 2020

This was referenced Jul 15, 2020

Rough ROS 2 port ros/collada_urdf#39

Open

Rough support for sdformat robot descriptions ros/joint_state_publisher#55

Merged

Initial urdf parser plugin that parses sdformat ros/sdformat_urdf#1

Merged

sloretz mentioned this pull request Jul 27, 2020

Proposal: SDFormat URDF Parser Plugin ros/urdf#34

Closed

sloretz added 5 commits July 27, 2020 15:21

Restore dependency on urdfdom

4608277

Signed-off-by: Shane Loretz <[email protected]>

Style

683c3ff

Signed-off-by: Shane Loretz <[email protected]>

Stops crash; not sure why TODO

2058537

Signed-off-by: Shane Loretz <[email protected]>

Add benchmark showing plugin overhead

535c275

Signed-off-by: Shane Loretz <[email protected]>

Whitespace

e310409

Signed-off-by: Shane Loretz <[email protected]>

clalancette reviewed Aug 5, 2020

View reviewed changes

sloretz and others added 13 commits August 5, 2020 13:36

CMake 3.5

7250733

Signed-off-by: Shane Loretz <[email protected]>

Include <string>

7efedb4

Signed-off-by: Shane Loretz <[email protected]>

Remove buildtool_export on ament_cmake

288413b

Signed-off-by: Shane Loretz <[email protected]>

exec_depend -> build_export_depend urdfdom headers

f9b2176

Signed-off-by: Shane Loretz <[email protected]>

Remove commented code

e09b0e1

Signed-off-by: Shane Loretz <[email protected]>

Document urdf_parser_plugin usage

3adaaba

Signed-off-by: Shane Loretz <[email protected]>

Document PIMPL forward declaration

d079336

Signed-off-by: Shane Loretz <[email protected]>

Alphabetize dependencies

41fa3f7

Signed-off-by: Shane Loretz <[email protected]>

Use tinyxml2 to reduce false positive might_handle()

cb348db

Signed-off-by: Shane Loretz <[email protected]>

Update urdf/src/urdf_plugin.cpp

4ba3e05

Signed-off-by: Shane Loretz <[email protected]> Co-authored-by: Chris Lalancette <[email protected]>

Handle pluginlib exceptions

33ad016

Signed-off-by: Shane Loretz <[email protected]>

Merge branch 'ros2-make_urdf_plugable' of github.com:ros2/urdf into r…

7055891

…os2-make_urdf_plugable

Return early on failure

8bb27d1

Signed-off-by: Shane Loretz <[email protected]>

sloretz requested a review from clalancette August 5, 2020 23:14

clalancette reviewed Aug 6, 2020

View reviewed changes

urdf/package.xml Outdated Show resolved Hide resolved

urdf/src/urdf_plugin.cpp Outdated Show resolved Hide resolved

urdf_parser_plugin/include/urdf_parser_plugin/parser.h Outdated Show resolved Hide resolved

urdf_parser_plugin/package.xml Outdated Show resolved Hide resolved

Document size_t max is no confidence score

c9f629d

Signed-off-by: Shane Loretz <[email protected]>

sloretz added 2 commits August 19, 2020 08:34

Skip if xml fails to parse

aac7fd3

Signed-off-by: Shane Loretz <[email protected]>

Make sure test can find pluginlib plugin

67df5dd

Signed-off-by: Shane Loretz <[email protected]>

sloretz mentioned this pull request Aug 19, 2020

[ROS 2] Support modules on OSX with .so or .bundle suffix ros/pluginlib#200

Open

Use SHARED instead of module

deabb1b

Fixes OSX failing build ros/pluginlib#200 Signed-off-by: Shane Loretz<[email protected]> Signed-off-by: Shane Loretz <[email protected]>

sloretz requested a review from clalancette August 19, 2020 18:59

clalancette reviewed Aug 19, 2020

View reviewed changes

IanTheEngineer reviewed Aug 19, 2020

View reviewed changes

sloretz mentioned this pull request Aug 19, 2020

Replace fprintf with ROS 2 logging #16

Open

sloretz added 2 commits August 19, 2020 15:54

picked -> chosen

70a81cc

Signed-off-by: Shane Loretz <[email protected]>

Use pluginlib_enable_plugin_testing()

3341988

Signed-off-by: Shane Loretz <[email protected]>

sloretz mentioned this pull request Aug 25, 2020

[ROS 2] Add function for same-package pluginlib tests ros/pluginlib#201

Merged

sloretz added 3 commits August 25, 2020 15:06

Define might_handle() return to length of data

69b3d49

Signed-off-by: Shane Loretz<[email protected]> Signed-off-by: Shane Loretz <[email protected]>

Return data.size() when not confident

025f816

Signed-off-by: Shane Loretz <[email protected]>

Try urdf when no plugin is confident

31014c0

Signed-off-by: Shane Loretz <[email protected]>

clalancette reviewed Aug 31, 2020

View reviewed changes

clalancette mentioned this pull request Sep 3, 2020

Remove TinyXML dependency from urdf. #17

Merged

sloretz added 2 commits September 14, 2020 12:18

ModelImplementation final

06474e8

Signed-off-by: Shane Loretz <[email protected]>

Initialize best_plugin to nullptr

f9fe6f9

Signed-off-by: Shane Loretz <[email protected]>

clalancette approved these changes Sep 18, 2020

View reviewed changes

sloretz merged commit 4b73ae2 into ros2 Sep 18, 2020

delete-merged-branch bot deleted the ros2-make_urdf_plugable branch September 18, 2020 20:06

sloretz mentioned this pull request Sep 18, 2020

Add dependency on TinyXML2 #19

Merged

clalancette mentioned this pull request Sep 24, 2020

Enable warnings #20

Merged

sloretz mentioned this pull request Sep 27, 2021

Foxy release? ros/sdformat_urdf#3

Closed

tobiasneumann mentioned this pull request Oct 26, 2022

[urdf] package.xml: add missing exec_depend to urdf_parser_plugin #34

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make urdf plugable and revive urdf_parser_plugin #13

Make urdf plugable and revive urdf_parser_plugin #13

sloretz commented Jul 15, 2020 •

edited

Loading

ros-discourse commented Jul 18, 2020

clalancette left a comment

sloretz commented Aug 5, 2020

sloretz commented Aug 19, 2020

sloretz commented Aug 19, 2020

sloretz commented Aug 19, 2020

clalancette left a comment

clalancette Aug 19, 2020

sloretz Aug 19, 2020

IanTheEngineer Aug 19, 2020

sloretz Aug 19, 2020

sloretz Aug 25, 2020

IanTheEngineer left a comment •

edited

Loading

IanTheEngineer Aug 19, 2020

IanTheEngineer Aug 19, 2020

sloretz Aug 19, 2020 •

edited

Loading

IanTheEngineer Aug 19, 2020

sloretz Aug 19, 2020

clalancette Aug 20, 2020

sloretz Aug 25, 2020

clalancette left a comment

clalancette Aug 31, 2020

sloretz Sep 14, 2020

clalancette Aug 31, 2020

sloretz Sep 14, 2020

sloretz commented Sep 18, 2020

sloretz commented Sep 18, 2020

Make urdf plugable and revive urdf_parser_plugin #13

Make urdf plugable and revive urdf_parser_plugin #13

Conversation

sloretz commented Jul 15, 2020 • edited Loading

ros-discourse commented Jul 18, 2020

clalancette left a comment

Choose a reason for hiding this comment

sloretz commented Aug 5, 2020

sloretz commented Aug 19, 2020

sloretz commented Aug 19, 2020

sloretz commented Aug 19, 2020

clalancette left a 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

Choose a reason for hiding this comment

IanTheEngineer left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sloretz Aug 19, 2020 • edited Loading

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

clalancette left a 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

sloretz commented Sep 18, 2020

sloretz commented Sep 18, 2020

sloretz commented Jul 15, 2020 •

edited

Loading

IanTheEngineer left a comment •

edited

Loading

sloretz Aug 19, 2020 •

edited

Loading