osrf / vrx

Virtual RobotX (VRX) resources.
Apache License 2.0
436 stars 198 forks source link

`generate_worlds.launch` is broken again #435

Open M1chaelM opened 2 years ago

M1chaelM commented 2 years ago

The vrx-docker README has instructions to generate practice worlds from yaml files: https://github.com/osrf/vrx-docker/blob/master/README.md#preparing-trials-for-a-task

These instructions current produce an error because the prepare_task_trials.bash script calls generate_worlds.launch which eventually generates a xacro file that references macros that xacro is unable to find. For example, the command

prepare_task_trials.bash stationkeeping

generates a file called stationkeeping0.world.xacro that contains the following lines:

<world name="robotx_example_course">
  <xacro:include filename="$(find vrx_gazebo)/worlds/xacros/include_all_xacros.xacro" />
  <xacro:include_all_xacros />
    <!-- === sydneyregatta_minus_scene === -->
      <xacro:sydneyregatta_minus_scene />

However, when xacro is called on this file it is unable to find the macro sydneyregatta_minus_scene, which should have been included by the include_all_xacros macro, which contains the following lines:

  <xacro:macro name="include_all_xacros">
    <xacro:include filename="$(find vrx_2019)/worlds/sandisland.xacro" />
    <xacro:include filename="$(find vrx_gazebo)/worlds/xacros/sandisland_minus_scene.xacro" />
    <xacro:include filename="$(find vrx_gazebo)/worlds/xacros/sydneyregatta_minus_scene.xacro" />

None of the xacro files included by this macro can be found anymore. If they are manually added to the stationkeeping0.world.xacro file, it works fine.

This was tested and working earlier this year, so I'm not sure what changed. I know we have repeatedly had problems with this particular design choice. At least once the reason was that the version of xacro updated and changed the way it scoped included files.

I'm still working on a fix, but it would be helpful if someone could verify that they can reproduce the error, just in case I made some mistake in my setup somewhere.

M1chaelM commented 2 years ago

@caguero or @j-herman would you mind running through the instructions at https://github.com/osrf/vrx-docker/blob/master/README.md#preparing-trials-for-a-task to verify this is broken. The error message you should see is:

unknown macro name: xacro:sydneyregatta_minus_scene
j-herman commented 2 years ago

@M1chaelM I was seeing this error on Tuesday. I tried everything I could think of to troubleshoot, with no success, and it suddenly started working again. I'll dig back through the history to see if there's anything that might have triggered the change, but I really think it was not something I did.
Try rebooting?

j-herman commented 2 years ago

The error is back on my system as well, but I can confirm one-time success since I have the three xacro and world files that were constructed from the new wildlife yaml. I will document my troubleshooting steps in more detail this time, and hopefully if the error goes away again we'll have a better idea of where to look.

M1chaelM commented 2 years ago

Thanks for confirming. I did try updating and rebooting just in case, but still no luck. I will investigate. I think this will require looking into xacro source code because the project documentation is poor.

j-herman commented 2 years ago

Yes, after spending a few hours looking through the xacro documentation and source code, I think it's more surprising that it worked once than that it's broken. Historical research shows that the fix which you thought had broken something last fall was somehow lost on a subsequent update, which may be why it all started working again that time. The fix was re-fixed last month.
It will be easier to change how we are including the files than to try to sort out what's going on with xacro. From the time I spent going through the source and the recent changes, it seems like the file includes live in the top-most level and we shouldn't be having this problem. There is a way to set the scope for properties, but not for includes.

M1chaelM commented 2 years ago

I think we should file an issue against the xacro repo to at least get clarity on how it's supposed to work. I will do this. I'm not really sure whether our nested include strategy is supported or not.