ros / xacro

Xacro is an XML macro language. With xacro, you can construct shorter and more readable XML files by using macros that expand to larger XML expressions.
http://www.ros.org/wiki/xacro
BSD 3-Clause "New" or "Revised" License
85 stars 99 forks source link

Handling arguments in included xacro files #204

Open jarvisschultz opened 5 years ago

jarvisschultz commented 5 years ago

Consider the following two xacro files:

parent.xacro

<?xml version="1.0" ?>
<robot name="example" xmlns:xacro="http://www.ros.org/wiki/xacro">
  <xacro:arg name="gazebo" default="false"/>

  <xacro:include filename="child.xacro">
    <xacro:arg name="gazebo" value="False" />
  </xacro:include>

  <xacro:if value="$(arg gazebo)">
    <link name="test" />
  </xacro:if>

</robot>

child.xacro

<?xml version="1.0" ?>
<robot xmlns:xacro="http://www.ros.org/wiki/xacro">

  <xacro:arg name="gazebo" default="false"/>

  <xacro:if value="$(arg gazebo)">
    <link name="test" />
  </xacro:if>

</robot>

Running xacro --inorder parent.xacro gazebo:=true results in

<?xml version="1.0" ?>
<!-- =================================================================================== -->
<!-- |    This document was autogenerated by xacro from parent.xacro                   | -->
<!-- |    EDITING THIS FILE BY HAND IS NOT RECOMMENDED                                 | -->
<!-- =================================================================================== -->
<robot name="example" xmlns:xacro="http://www.ros.org/wiki/xacro">
  <link name="test"/>
  <link name="test"/>
</robot>

I'd expect there to be only one <link>, but it seems that I cannot set the value of the argument in an included xacro file. I've seen a few xacro files out there trying to set arguments in includes so I thought this was possible (ex1, ex2), but I can't make it work. Is this a regression or a bug? Am I missing something?

rhaschke commented 5 years ago

I don't think that any content in tags is considered at all. It's just silently ignored. But it could be indeed interesting to set scoped args and properties for an include. However, I will not have time for this in the next few weeks.

jarvisschultz commented 5 years ago

Thanks for the quick response. I think perhaps users assume that xacro files will behave like launch files (a reasonable assumption), and I do agree that it would be a useful feature. I'll let you know if I get any time to look at this as well.

tandronescu commented 4 years ago

Did you ever figure this out @jarvisschultz ? Running into the same issue, seems like a bug since like you said I have seen this work in some scenarios. Looking at the same sawyer example and I don't see the magic they did to make it work

jarvisschultz commented 4 years ago

I have never spent any time into digging into this feature. In my particular example, it was pretty easy to work around by using macros with args instead of xacro files with args. I.e. parent.xacro would read an <arg> tag and pass the value into a macro defined in child.xacro.

rickstaa commented 3 years ago

I think this issue in 2020 still contains a relevant feature request. I think having the xacro:include take child tags would result in cleaner code. I, however, currently also don't have time to take it on. Nevertheless, I will consider it in the future.

rickstaa commented 3 years ago

I just noticed that apparently, in ROS, noetic cmd line arguments automatically get passed to any macros inside a macro:include file if the right syntax is used. With the right syntax, I mean the following:

For a working example, see this pull request I created for the franka_ros repository. For example, if I set the hardware_interface argument when calling the panda_arm_hand.urdf.xacro file, it gets passed all the way through to the panda.control.xacro file.

We can close this feature request if we add a small note about this behaviour in the documentation. @jarvisschultz, @rhaschke, what do you think?

rhaschke commented 3 years ago

Isn't that documented here already? Both args and properties are forwarded to included files.

rickstaa commented 3 years ago

@rhaschke Thanks for your comment are you referring to the following?

When including files within a macro, not the macro-defining but the macro-calling file is the one that processes the include!

This would indeed imply the behavoir.

rhaschke commented 3 years ago

The key point is that args is a global dictionary, which is accessible from within any file or macro. If you feel that the docs should be improved, go ahead and do so ;-)

rickstaa commented 3 years ago

@rhaschke Thanks for your answer. I overlooked that sentence. I think the behaviour is clear in combination with the sentence I quoted above. Further, I did found out how the arguments were passed quickly when using the package. My main point was that we might be able to close this issue since something similar is already included in the package.

jarvisschultz commented 3 years ago

To me it seems like you worked around this in exactly same way I described in my comment above. Basically since you can't pass values for <arg> tags into included xacro files, you are using the rospack-like substitution argument, $(arg), to forward the value of an argument to a macro. While this is different than what I had originally envisioned in this issue, I do think it is how this need should be accomplished with the current capabilities of xacro.

Perhaps @rhaschke is correct that documentation improvements are really what is needed to illustrate to users the "proper" way of accomplishing this.

rhaschke commented 3 years ago

<xacro:arg> tags are considered everywhere - except within the scope of an <include> tag. Actually, all content of an include tag is neglected. Sorry that you ran into this trap. I still think it might be useful to define file-local properties there. However, this doesn't work for args, because those are global anyway.

rickstaa commented 3 years ago

@jarvisschultz That's correct. For me, I think this method is good enough. I, however understand that the proposal you created above would be more in line with what people expect when they are familiar with launch files. Maybe keep the issue open might somebody have time to implement the feature and add something like the following to section 8 of the documentation:

The file "other_file.xacro", will be included and expanded by xacro. Because of this, and the fact that the args dictionary is global, any arguments you pass to the file that contains the xacro:include will be available inside the expanded xacro.

For me, such a statement would be already enough to get the behavoir.

jarvisschultz commented 3 years ago

@rhaschke This is definitely correct and an important thing for people to understand. I agree that file-local properties could still be useful, and is very much along the lines of what I was thinking when I created this issue.

@rickstaa Looks like a good improvement to me. Maybe could also add a sentence explicitly helping users avoid this trap. Maybe something like:

If you want to pass custom <xacro:arg> values to the included other_file.xacro it is instead recommended to use a <xacro:macro>.

rhaschke commented 3 years ago

There is no need to pass arguments via the macro (of course that's possible too). But, you could declare your <xacro:args> just outside the <xacro:include> tag and it should work as expected.

rickstaa commented 3 years ago

@rhaschke Clear explanation, thanks! In that case, I think adding something like the statement below to section 6 would explicitly explain the current behaviour.

Using this you can run xacro like: 
     <param name="robot_description" command="$(find xacro)/xacro.py $(arg model) myvar:=true" />

Contrary to [launch](https://wiki.ros.org/roslaunch/XML/arg) files, where arguments are specific to a single launch file, the `args` dictionary in xacro files is global. As a result, arguments specified on the command line are also available in included xacro files (see section 8).

or simply:

Using this you can run xacro like: 
     <param name="robot_description" command="$(find xacro)/xacro.py $(arg model) myvar:=true" />

In xacro files, `args` is a global dictionary accessible from within any file or macro.

Maybe this is already explicitly mentioned in the docs but I failed to find it.

rickstaa commented 3 years ago

For me, the current documentation @rhaschke pointed me to already implicitly explains the behaviour, and the behaviour becomes clear quickly when experimenting with xacro files. In my case, I noticed the warning message Child elements of a <xacro:include> tag are ignored and quickly found out that the args were already available inside the included xacro. I was just thinking of a way to make it more clear for people that start using xacros. 🤔 But maybe this is not needed after all since this issue is the third search result when you google something like Child elements of a <xacro:include> tag are ignored or xacro:include arguments.

rhaschke commented 3 years ago

I like your second proposal and added it to the docs.

rickstaa commented 3 years ago

@rhaschke Amazing thanks a lot. I will hide my comments to clean up this feature request.

rcywongaa commented 2 years ago

Unfortunately, this means that the user of a child.xacro relies on the provider to properly wrap it in a macro. An alternative (though likely even more involved) would be to somehow allow wrapping a file in a macro on the user side. Would be cool to have something like the following

child.macro.xacro

<xacro:macro name="child" params="gazebo:=false">
  <xacro:include filename="child.xacro"/>
</xacro:macro>
rhaschke commented 2 years ago

@rcywongaa, including a file within a macro, should be possible already. Also using the property ${gazebo} shouldn't be an issue. Please open a new issue, if you actually experience problems with that.

rcywongaa commented 2 years ago

@rhaschke But included files inside the macro don't get the macro's parameters so it's still not possible to pass arguments to the included file from another xacro file. Using property also doesn't solve that.

Note this is all assuming that child.xacro is provided externally and that it's not possible / undesirable to modify it.

The following variants of parent.xacro all fail to overwrite the gazebo argument resulting in the same output as OP posted

parent1.xacro

<?xml version="1.0" ?>
<robot name="example" xmlns:xacro="http://www.ros.org/wiki/xacro">
  <xacro:macro name="child" params="gazebo:=false">
    <xacro:include filename="child.xacro"/>
  </xacro:macro>

  <xacro:child gazebo="false"/>

  <xacro:arg name="gazebo" default="false"/>
  <xacro:if value="$(arg gazebo)">
    <link name="test" />
  </xacro:if>
</robot>

parent2.xacro

<?xml version="1.0" ?>
<robot name="example" xmlns:xacro="http://www.ros.org/wiki/xacro">
  <xacro:property name="gazebo" value="false"/>
  <xacro:include filename="child.xacro"/>

  <xacro:arg name="gazebo" default="false"/>
  <xacro:if value="$(arg gazebo)">
    <link name="test" />
  </xacro:if>
</robot>

parent3.xacro

<?xml version="1.0" ?>
<robot name="example" xmlns:xacro="http://www.ros.org/wiki/xacro">
  <xacro:macro name="child">
    <xacro:property name="gazebo" value="false"/>
    <xacro:include filename="child.xacro"/>
  </xacro:macro>

  <xacro:child/>

  <xacro:arg name="gazebo" default="false"/>
  <xacro:if value="$(arg gazebo)">
    <link name="test" />
  </xacro:if>
</robot>

parent4.xacro

<?xml version="1.0" ?>
<robot name="example" xmlns:xacro="http://www.ros.org/wiki/xacro">
  <xacro:arg name="child.gazebo" default="false"/>
  <xacro:include filename="child.xacro" ns="child"/>

  <xacro:arg name="gazebo" default="false"/>
  <xacro:if value="$(arg gazebo)">
    <link name="test" />
  </xacro:if>
</robot>

Basically it's the same problem as the initial post. I'm merely suggesting how it would be useful and potential approaches to implementing it.

rhaschke commented 2 years ago

You need to understand that xacro distinguishes two types of parameters: properties and (cmdline) arguments. The former ones a much more flexible. If your included file just uses the latter ones, you are stuck to a global argument namespace.

rhaschke commented 2 years ago

I managed to achieve your task as follows: parent.xacro:

<?xml version="1.0"?>
<robot name="example" xmlns:xacro="http://www.ros.org/wiki/xacro">
    <xacro:macro name="child" params="gazebo:=false">
        <xacro:arg name="gazebo" default="${str(gazebo)}" />
        <xacro:include filename="child.xacro" />
    </xacro:macro>

    <xacro:child gazebo="true" />
</robot>

child.xacro:

<?xml version="1.0"?>
<robot xmlns:xacro="http://www.ros.org/wiki/xacro">
    <xacro:if value="${gazebo}">
        <link name="prop" />
    </xacro:if>
    <xacro:if value="$(arg gazebo)">
        <link name="arg" />
    </xacro:if>
</robot>

However, note that you can set the (default) argument only once. The best way to go is to use properties.