Conversation
|
Thank you for all of this work! Lots of great stuff in there. As you mentioned, some of these changes go beyond the "bug-fix" type of change that we can easily merge into ROS Indigo. I'll create a jade-devel branch right now where we can quickly merge these changes in, since these features look very useful. In terms of the expression parser, I wasn't directly involved in the original creation of xacro, so I can't speak with certainty, but my understanding is that xacro became complex over time. I think that originally it was intended for very simple XML operations, so it gradually became more and more complex. I agree that going to Python-based expressions will make the code cleaner. However, integer vs. floating-point division is an unfortunate historical artifact, and I think that there are enough "legacy" URDF files which expect this behavior that I'm afraid to change it. What if instead, the string delimiter is different, such as #{ } instead of ${ } ? Then, the legacy parser could be used still for dollar-sign strings, and the python parser would be used for pound-sign strings. Then I think we can merge this immediately into the jade-devel branch (which I'll create right now). |
|
Nice to hear that you like the suggestions. Regarding integer division, I would like to look into some python capabilities first. As the following links suggest, python itself is struggling with integer vs. float division since version 2.2 too and they suggested the operator // (double slash) for integer division. In python 3 single slash will default to float division. This new behaviour can be enforced in python >2.2 Introducing another variable indicator #{} is also an option, but it is not very appealing. |
|
Ohh, that's fantastic, I didn't realize that python 3 had already addressed this concern. If we do the "from future import divison" in python 2, does that get propagated into any "eval" statements? I don't know how much of the current interpreter is passed through to eval(). |
|
At least, in an ipython shell, the new division behavior is propagated to eval(). However, I'm not sure what side effects that might have in xacro itself. But probably xacro doesn't use calculations by itself. |
|
I added the Obviously, if there is bad code entered in evaluated expressions, hackers can do nasty things... |
|
Can you re-target this to the jade-devel branch? I think these features are great, and I think people would like them in Indigo since they fix a few things (I'm suspecting our hand-crafted parser still has some lurking bugs and/or unexpected behavior), and Indigo will be around for a long time. However, I think it would be good to give these improvements some "soak time" in Jade first, before we merge them into Indigo. The Jade branch should be ready to go; once this pull request is merged into Jade, I'll push out a release there. Thanks! |
properties and macros in (typically 3rd party) include files - enable the new behaviour by passing --inorder cmdline option - to improve code readibility and reusability, introduced functions process_include(node), grab_macro(elt, macros), grab_property(elt, symbols) containing 1:1 corresponding handling from process_includes, grab_macros, and grab_properties - added corresponding test case test_inorder_processing()
- replaced handle_expr with python-internal eval() call - care has been taken to resolve variables recursively on demand (in Table.__getitem__) - allows for evaluation of standard math functions - other desired functions could be added in eval_self_contained - Values in Table symbols are not stored as strings but as typed values. If text is required, a conversion with str() is performed. This is need to ensure proper evaluation of expressions. Otherwise 3*"1" would evaluate to "111". - using __future__.division we can handle int division evaluating to float as before
- for equality expressions in <xacro:if> - math function usage
- literal evaluation should only consider literals, but no expressions use ast.literal_eval() - removed eval() from xacro:if evaluation back to string comparison to handle (lowercase) true and false
|
Done. |
Dear maintainer,
this isn't a classical pull request, but rather a pull proposal. It comprises:
xml_matches(), handling of non-element nodes in<include>,<if>,<macro>as indicated in previous pull request #48<xacro:call macro="${variable}"/>eval()instead of the hand-coded parserAll changes build on top of each other to some extend. They have associated branches in our xacro fork. A more detailed motivation for the individual changes can found in the README
I'm aware of the fact, that these changes heavily modify the original code. To get an overview of all changes you should follow the individual commits, which I carefully packaged into meaningful and easily comprehensible steps. Of course, I'm open for questions and discussion as well ;-)
I'm also aware of the fact, that my changes will have some impact on existing xacro files out there. I see namely three changes xacro users need to take care of:
pr2 exampledoesn't rely on this fact, but successfully compiles in both modes.Because I'm aware of this fact, I decided to enable the ordered-processing optionally only, with the command line option --inorder
<include>,<if>,<macro>will also change the previous behaviour. This will be irrelevant however for urdf, because urdf only considers element nodes anyway.By the way, why did you opted for an own, hand-written expression parser instead of using python's capabilities? I don't see any benefit for this.
Having said all this, I would be happy if you are interested in some of these contributions and would like to consider them for the next or over-next ROS release.
Kind regards, Robert