SRDF Python parser about srdfdom HOT 24 CLOSED

Why not just use python bindings around the C++ code, to prevent discrepancies in the parser, increase speed, reduce maintenance, and code duplication?

from srdfdom.

Thanks for your comment.

I agree with the remarks, however, I thought I would mimic what was done for the urdfdom upstream which contains a python parser in parallel of the c++ parser (and hence inherently probably has the same discrepancies you mention), even in the vivid branch (http://bazaar.launchpad.net/~ubuntu-branches/ubuntu/vivid/urdfdom/vivid/files/head:/urdf_parser_py/src/urdf_parser_py/)
Do you know the reason the python parser is kept in urdfdom then ?

I could give a try to use your idea, but I first need to learn how to use python c++ binding.

from srdfdom.

I try to avoid python at all costs (why make extra work off adding a python layer of complexity?) but some good examples are in this repo: https://github.com/ros-planning/moveit_ros/tree/indigo-devel/planning_interface

from srdfdom.

I don't have strong reasons for which I chose python. I thought this was the most readable I could do but this gets far beyond what I expected when creating this parser.

Anyway, I like to learn things, so I started to create a wrap_python_model.cpp cpp-python binding to expose the public functions of the srdf parser to python. Getter functions are quite easy to expose, as they only rely on structures defined in srdfdom, but I obviously also need to expose the init functions in order to init the parser with data, and this is problematic.

Indeed every single init function of the srdf Model requires a urdf::ModelInterface which would need to be filled in python to be passed to the exposed function. However, to do that, the urdf::Model Interface class should be exposed to python too. There is no such thing in urdf, the urdf_parser_py is only an augmented xml object with parser consistency checks. Such an exposed ModelInterface class should live in the urdfdom_headers and I am not sure they want to expose this interface to python since they already have the urdf_parser_py.

The srdf cpp parser requires urdf to check urdf-srdf coherence which my srdf_parser_py did not do. I only had the model consistency checks.

by the way, from the time I spend on creating the python binding, I realized the effort maintaining the wrapper might be as much as maintaining the python parser. The binding would need maintenance on api changes on the cpp srdf and on new functions (due to srdf added tags), but the python parser would require maintenance only on the srdf structure change (added tags).
Not really sure which is best.

In my application that requires srdf parsing, I might completely drop the srdf_parser_py and do a manual xml parsing, which would not rely on any srdf rules but parse raw xml.
It is a pity but that could be a solution.

What do you think ?

from srdfdom.

Indeed every single init function of the srdf Model requires a urdf::ModelInterface which would need to be filled in python to be passed to the exposed function.

What if you just loaded the URDF within the C++, and instead offered init functions that could provide a robot_description parameter name for the rosparam server, a file path to the URDF, or an XML string? This seems like the best option to me.

I am not sure they want to expose this interface to python since they already have the urdf_parser_py.

I don't know for sure, but I'd say having a python wrapped URDF parser would be appreciated too. btw, there isn't much of a "they" anymore since the WG days.

I realized the effort maintaining the wrapper might be as much as maintaining the python parser. The binding would need maintenance on api changes on the cpp srdf and on new functions (due to srdf added tags), but the python parser would require maintenance only on the srdf structure change (added tags).

SRDF hasn't changed sine June 2013 and even if it did, its not clear to me why making the needed changes to the python wrapper would be very difficult? It would certainly provide consistency in implementation.

Again, I'm no python expert but making a proper wrapper seems like a worthwhile cause.

I would really appreciate @isucan's opinions on this.

from srdfdom.

Thanks for your feedback.

You suggest to add some init functions in srdfdom that would not require the urdf to be pre-processed but process it internally. That could indeed simplify the wrapping.

I will wait for the opinion you also requested, before going in this direction.

Meanwhile I already modified my application to parse the srdf without the suggested parser, but integrating helper functions (which are then duplicate of what would be in a proper parser) to access the correct srdf elements I need.

from srdfdom.

URDF has its own python parser too, so to me either variant seems fine. I kind of prefer to have separate parsers (per language). The format is what matters, not the API itself. Of course, bindings are fine if it is easy to write them. If not, it makes sense to have a separate parser.
https://github.com/ros/urdfdom/tree/master/urdf_parser_py

from srdfdom.

@isucan thanks for your feedback.

I found python binding more complex than the python parser because of all the structures to be wrapped and passed, and the dependency to urdf structures (ModelInterface) that do not exist.

I derived the srdf_parser_py from the separate urdf parser you mention, even depending on its xml reflection python lib to reduce code duplication, and this issue I opened was to ask for a integration of this parser in the srdfdom project. You could integrate the code from the https://github.com/ubi-agni/srdf_parser_py in the srdfdom or take it as a separate repository if you like.

Having code upstream is always safer.

from srdfdom.

I think using the same repo (srdfdom) is better. Could you prepare a pull request to it for review?

from srdfdom.

I will work on that very soon.

from srdfdom.

Not sure if this is me or not yet, but seeing ros/rosdistro#4633 thought it might fit here.

Debian jessie, no urdfdom ros package installed in src dir but system dep liburdfdom-dev instead.

catkin build

Could not find a package configuration file provided by "urdfdom_py" with
  any of the following names:

    urdfdom_pyConfig.cmake
    urdfdom_py-config.cmake

Is it that the "Use the system deps instead" approach hasn't flowed on to urdfdom_py? Not getting a hit with pkg-config on this name.

from srdfdom.

Indeed, the latest PR #9 adds a new package dependency to srdfdom.
However urdfdom and urdfdom_py are different packages. The first one is indeed now a system package (referered as liburdfdom-dev), the second one is still released through ros via ros-indigo-urdfdom-py
I tested on a trusty install without ros-indigo-urdfdom-py and the command
rosdep check srdfdom
indeed indicates
System dependencies have not been satisified: apt ros-indigo-urdfdom-py
So a command like rosdep install srdfdom will install the missing dependency and findpackage will then find the catkin package.
Hope this helps.

from srdfdom.

hmmm jessie doesn't have many packages pre-compiled.

How would one go about packaging up the urdf_parser_py code as a source install of ros-indigo-urdfdom-py with the final result being urdfdom_py exposed as a package? Wouldn't that also include urdfdom which should be distributed via the system dep? Do these packages need splitting?

The changelog at https://github.com/ros-gbp/urdfdom_py-release shows The packages in the urdfdom_py repository were released into, though I see no repo by that name.

from srdfdom.

Sorted...

https://github.com/ros-gbp/urdfdom_py-release installed from source, although srdfdom/package.xml was missing a depend for urdfdom_py.

from srdfdom.

the URDFDom issue you are referring to is from last year, and this PR/issue hasn't been released yet, so I think your issue is unrelated.

from srdfdom.

@mryellow could you specify which depend to urdfdom_py is missing in srdfdom/package.xml ? I don't see the missing one (both build and run depend are there).

from srdfdom.

You know what, looking through the commit history on it now and I couldn't say but it appears I've corrupted it at some stage. I was working on it a bit drained there, could have mistakenly made an earlier edit looking for urdfdom_py solutions.

Sorry for any noise, hopefully keywords help the next noob with an oddball mixed install looking for the correct packages on google.

from srdfdom.

@davetcoleman or @isucan is there a possibility to release this change in indigo ? This adds a new feature but does not break existing code.

from srdfdom.

I think it should be released, yes. I can do it if @isucan is ok with it

from srdfdom.

+1 if we don't break API, we're good. Thanks!
On Apr 16, 2015 19:58, "Dave Coleman" [email protected] wrote:

I think it should be released, yes. I can do it if @isucan
https://github.com/isucan is ok with it

—
Reply to this email directly or view it on GitHub
#8 (comment).

from srdfdom.

@davetcoleman how do I know when the release has been done (except from looking at the package content )? Should it be seen in the release tab of github with a 0.2.8 ?

from srdfdom.

Dear @davetcoleman , @isucan is there a chance to get the python parser released for indigo- and jade- ?

from srdfdom.

i will after i get back from icra this week, unless @isucan wants to first

from srdfdom.

I'm really sorry for the long delay
ros/rosdistro#8681

from srdfdom.