Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolve Fuel resources when included in models and worlds #309

Merged
merged 18 commits into from
Mar 31, 2021

Conversation

FirefoxMetzger
Copy link
Contributor

@FirefoxMetzger FirefoxMetzger commented Mar 28, 2021

Closes: #296
Closes: #174
Closes: #168

After digging into it, all that was needed is to specify a callback. I chose to implement it slightly differently than #174

Instead of adding fuel support to the GazeboWrapper, I added it directly to the (lower level?) GazeboSimulator. Loading fuel models requires a FuelClient which became an attribute of GazeboSimulator to avoid constantly re-instantiating it (I mimic this behavior from ign-gazebo).

I didn't find any C++ unit tests to add a test for downloading fuel models, so I didn't add one. I found some python tests, so I can add a test there if desired.

I experimented with it locally. It expands the include tags correctly, but I am getting other resource not found errors, so I am now investigating where they come from and if they are related to the download or to the world being misspecified.

@FirefoxMetzger
Copy link
Contributor Author

The error that I am getting for loading fuel models with this PR comes from models that specify relative paths in their sdf.

Example Model: https://app.ignitionrobotics.org/GoogleResearch/fuel/models/Avengers_Thor_PLlrpYniaeB

it specifies a relative path to the mesh

<mesh>
    <uri>meshes/model.obj</uri>
</mesh>

which the current setup translates to

<mesh>
  <uri>file:///meshes/model.obj</uri>
</mesh>

Files that specify meshes via their fuel URI (e.g. panda)

<mesh>
    <uri>https://fuel.ignitionrobotics.org/1.0/openrobotics/models/panda with ignition position controller model/1/files/meshes/visual/link0.dae</uri>
</mesh>

work fine. If anyone can piece together which part of the pipeline is still missing; I'll happily update the PR.

@FirefoxMetzger
Copy link
Contributor Author

FirefoxMetzger commented Mar 29, 2021

Here's a small example to reproduce the issue:

from scenario import gazebo as scenario_gazebo
import time

scenario_gazebo.set_verbosity(scenario_gazebo.Verbosity_error)
gazebo = scenario_gazebo.GazeboSimulator(step_size=0.001, rtf=1.0, steps_per_run=1)
assert gazebo.insert_world_from_sdf("./test_fuel_world.sdf")
gazebo.initialize()
gazebo.gui()
gazebo.run(paused=True)
time.sleep(3)

It loads two models. A table (loads fine) and a box/container (currently breaks). The errors produced are

[Err] [SystemPaths.cc:444] Could not resolve file [meshes/model.obj]
[Err] [MeshManager.cc:172] Unable to find file[meshes/model.obj]
Setting callback for signal SIGINT
Setting callback for signal SIGTERM
Setting callback for signal SIGABRT
QStandardPaths: XDG_RUNTIME_DIR not set, defaulting to '/tmp/runtime-sebastian'
[GUI] [Err] [SystemPaths.cc:444] Could not resolve file [meshes/model.obj]
[GUI] [Err] [MeshManager.cc:172] Unable to find file[meshes/model.obj]
[GUI] [Err] [MeshDescriptor.cc:56] Mesh manager can't find mesh named [meshes/model.obj]
[GUI] [Err] [Ogre2MeshFactory.cc:496] Cannot load null mesh [meshes/model.obj]
[GUI] [Err] [Ogre2MeshFactory.cc:84] Failed to get Ogre item for [meshes/model.obj]
[GUI] [Err] [SceneManager.cc:360] Failed to load geometry for visual: visual
test_fuel_world.sdf
<sdf version='1.7'>
  <world name='panda_world'>
    <physics name='1ms' type='ignored'>
      <max_step_size>0.001</max_step_size>
      <real_time_factor>1</real_time_factor>
      <real_time_update_rate>1000</real_time_update_rate>
    </physics>
    <plugin name='ignition::gazebo::systems::Physics' filename='ignition-gazebo-physics-system'/>
    <plugin name='ignition::gazebo::systems::UserCommands' filename='ignition-gazebo-user-commands-system'/>
    <plugin name='ignition::gazebo::systems::SceneBroadcaster' filename='ignition-gazebo-scene-broadcaster-system'/>

    <light name='sun' type='directional'>
      <cast_shadows>1</cast_shadows>
      <pose>0 0 10 0 -0 0</pose>
      <diffuse>0.8 0.8 0.8 1</diffuse>
      <specular>0.2 0.2 0.2 1</specular>
      <attenuation>
        <range>1000</range>
        <constant>0.9</constant>
        <linear>0.01</linear>
        <quadratic>0.001</quadratic>
      </attenuation>
      <direction>-0.5 0.1 -0.9</direction>
      <spot>
        <inner_angle>0</inner_angle>
        <outer_angle>0</outer_angle>
        <falloff>0</falloff>
      </spot>
    </light>
    <gravity>0 0 -9.8</gravity>
    <magnetic_field>6e-06 2.3e-05 -4.2e-05</magnetic_field>
    <atmosphere type='adiabatic'/>
    <scene>
      <ambient>0.4 0.4 0.4 1</ambient>
      <background>0.7 0.7 0.7 1</background>
      <shadows>1</shadows>
    </scene>
    <model name='ground_plane'>
      <static>1</static>
      <link name='link'>
        <collision name='collision'>
          <geometry>
            <plane>
              <normal>0 0 1</normal>
              <size>100 100</size>
            </plane>
          </geometry>
          <surface>
            <friction>
              <ode/>
            </friction>
            <contact/>
          </surface>
        </collision>
        <visual name='visual'>
          <geometry>
            <plane>
              <normal>0 0 1</normal>
              <size>100 100</size>
            </plane>
          </geometry>
          <material>
            <ambient>0.8 0.8 0.8 1</ambient>
            <diffuse>0.8 0.8 0.8 1</diffuse>
            <specular>0.8 0.8 0.8 1</specular>
          </material>
          <plugin name='__default__' filename='__default__'/>
        </visual>
      </link>
      <plugin name='__default__' filename='__default__'/>
      <pose>0 0 0 0 -0 0</pose>
    </model>
    <include>
      <uri>https://fuel.ignitionrobotics.org/1.0/OpenRobotics/models/Table</uri>
      <name>table2</name>
      <pose>0.794 0 0 0 -0 1.5708</pose>
    </include>
    <include>
      <uri>https://fuel.ignitionrobotics.org/1.0/GoogleResearch/models/Avengers_Thor_PLlrpYniaeB</uri>
      <name>Avengers_Thor_PLlrpYniaeB</name>
      <pose>1 0.5 1.025 0 -0 0</pose>
    </include>
  </world>
</sdf>

Copy link
Member

@diegoferigo diegoferigo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @FirefoxMetzger for the contribution! I went through the code for a first pass, this PR looks good, I just have few comments and subtle changes that you could never have known :)

Instead of adding fuel support to the GazeboWrapper, I added it directly to the (lower level?) GazeboSimulator.

GazeboWrapper is the old name of GazeboSimulator. #174 was targeting a quite old version of gym-ignition.

I found some python tests, so I can add a test there if desired.

If you can add a new test in test_ignition_fuel.py would be great. A possibility is adding a new function in utils.py with the SDF string (as simple as possible, no plugins, no GUI, like the other functions there) that could be a distilled version of that you posted in this PR. Then, in the test you can load the SDF and check if the models are part of the world.

The error that I am getting for loading fuel models with this PR comes from models that specify relative paths in their sdf.

This behavior reminds me gazebosim/sdformat#227, but in that case only URDF files were affected. Could it be related to it?


// Attributes related to Fuel Support
// Note to devs: sdformat10 uses a global parser config. Starting with
// stformat11, this practice is depreciated in favour of creating a local
// parser config that is fed into all functions reading sdf
// (sdf::readFile(...), etc.) with Ignition Edifice and beyond, this will
// need an to be amended with the local sdf config.

Regarding this comment, do you plan to update to Edifice as soon as we merge #307 and release a new version? This PR could be a bug fix (and targeting master / Dome) or a new feature (and targeting devel / Edifice). Therefore, if you plan to update to Edifice, maybe this PR can use devel as target branch and directly include the sdformat11 logic.

cpp/scenario/gazebo/src/GazeboSimulator.cpp Show resolved Hide resolved
cpp/scenario/gazebo/src/GazeboSimulator.cpp Outdated Show resolved Hide resolved
cpp/scenario/gazebo/src/GazeboSimulator.cpp Outdated Show resolved Hide resolved
cpp/scenario/gazebo/src/GazeboSimulator.cpp Outdated Show resolved Hide resolved
cpp/scenario/gazebo/src/GazeboSimulator.cpp Outdated Show resolved Hide resolved
cpp/scenario/gazebo/src/GazeboSimulator.cpp Outdated Show resolved Hide resolved
@diegoferigo diegoferigo changed the title New Feature: Add Basic Fuel support Support inserting world files with models included from Fuel Mar 29, 2021
@diegoferigo
Copy link
Member

Renamed the PR since basic fuel support was introduced in #158.

@FirefoxMetzger
Copy link
Contributor Author

Regarding this comment, do you plan to update to Edifice as soon as we merge #307 and release a new version?

I will spin up a new VM and check if my current setup breaks if I install Edifice instead of Dome. If it works without too much pain, I'll migrate (as per the support policy).

Therefore, if you plan to update to Edifice, maybe this PR can use devel as target branch and directly include the sdformat11 logic.

I would do this as a bugfix for Dome. I put Edifice as the time to update, because we should try to be current, but there is ofc. a grace period.

sdformat11 still allows/uses the global config so this will work until Ignition Fortress at least. We should wait and see how Gazebo behaves regarding this. Skimming the sdformat11 source, there is the option to pass the config to "loading" SDF from a pointer [source]. This could mean that gazebo may be able to pick up any unparsed fuel links once it takes over the sdf from gym-ignition. In this case, the better change might be to remove our callback entirely.

This behavior reminds me gazebosim/sdformat#227, but in that case only URDF files were affected. Could it be related to it?

Both issues are likely related.

It also seems to be the source of [Err] [SystemPaths.cc:444] Could not resolve file [texture.png] in #301 . In this case, the texture is part of the Meshes .mtl file and specified as a relative path that doesn't seem to be resolved correctly.

@FirefoxMetzger
Copy link
Contributor Author

FirefoxMetzger commented Mar 29, 2021

Support inserting world files with models included from Fuel

I'm happy with the new name; however, it is a little missleading. Just to avoid confusion, the PR registers a global callback that becomes active as soon as a gazebo instance is created. This will not just affect insertWorldFromSDF, but every call to sdf::Root::Load and derivatives.

In a nutshell, it also ensures that later calls to, say, insertModel will correctly resolve fuel-based includes, and should work fine for nested fuel links, too. (Edit: See #311 )

Edit: This may actually cause problems when the simulator instance is destroyed, the sdf binary remains loaded, and another call to sdf::Load happens, as the FuelClient will have stopped existing at this point. Unfortunately, sdformat doesn't seem to implement the ability to unregister callbacks...

@diegoferigo
Copy link
Member

Regarding this comment, do you plan to update to Edifice as soon as we merge #307 and release a new version?

I will spin up a new VM and check if my current setup breaks if I install Edifice instead of Dome. If it works without too much pain, I'll migrate (as per the support policy).

Therefore, if you plan to update to Edifice, maybe this PR can use devel as target branch and directly include the sdformat11 logic.

I would do this as a bugfix for Dome. I put Edifice as the time to update, because we should try to be current, but there is ofc. a grace period.

Looks good to me.

This behavior reminds me osrf/sdformat#227, but in that case only URDF files were affected. Could it be related to it?

Both issues are likely related.

It also seems to be the source of [Err] [SystemPaths.cc:444] Could not resolve file [texture.png] in #301 . In this case, the texture is part of the Meshes .mtl file and specified as a relative path that doesn't seem to be resolved correctly.

Regarding to that, I also found gazebosim/gz-sim#286.

sdformat11 still allows/uses the global config so this will work until Ignition Fortress at least. We should wait and see how Gazebo behaves regarding this. Skimming the sdformat11 source, there is the option to pass the config to "loading" SDF from a pointer [source]. This could mean that gazebo may be able to pick up any unparsed fuel links once it takes over the sdf from gym-ignition. In this case, the better change might be to remove our callback entirely.

To recap, if I understood correctly, sdformat <= 10 requires instantiating FuelClient and registering the global callback (as done in this PR), instead sdformat >= 11 allows passing to Root::Load a local ParserConfig object with additional callbacks so that it does not rely on the global resources.

Support inserting world files with models included from Fuel

I'm happy with the new name; however, it is a little misleading. Just to avoid confusion, the PR registers a global callback that becomes active as soon as a gazebo instance is created. This will not just affect insertWorldFromSDF, but every call to sdf::Root::Load and derivatives.

In a nutshell, it also ensures that later calls to, say, insertModel will correctly resolve fuel-based includes, and should work fine for nested fuel links, too. (Edit: See #311 )

I missed this angle while reviewing this PR, thanks for providing more details. Renamed to Resolve Fuel resources when included in models and worlds.

Edit: This may actually cause problems when the simulator instance is destroyed, the sdf binary remains loaded, and another call to sdf::Load happens, as the FuelClient will have stopped existing at this point. Unfortunately, sdformat doesn't seem to implement the ability to unregister callbacks...

This is true. However, if the simulator instance is destroyed, none of the resources will work properly (I mean, World Model Link ... objects). In this state, the only way to call World::insertModel again would be instantiating a new GazeboSimulator object, that i believe will override the callback proving a fresh new FuelClient object, and get the new World. Do you have any other edge case in mind?

@diegoferigo diegoferigo changed the title Support inserting world files with models included from Fuel Resolve Fuel resources when included in models and worlds Mar 29, 2021
@FirefoxMetzger
Copy link
Contributor Author

FirefoxMetzger commented Mar 29, 2021

To recap, if I understood correctly, sdformat <= 10 requires instantiating FuelClient and registering the global callback (as done in this PR), instead sdformat >= 11 allows passing to Root::Load a local ParserConfig object with additional callbacks so that it does not rely on the global resources.

Very close, yes. Iirc, the local config replaces the global one if present (they are exclusive).

Whenever sdf looks for a resource it first searches paths internally (afaik environment variables are handled here). If its internal method doesn't work, it will start going through all registered FindCallbacks (in the order they were created) until a callback responds with a non-empty string, and then it will use that string as location of the resource.

Gazebo creates it's own callback to load fuel models, but it only gets added once the server instance is created (for us that should be at "GazeboServer::Initialize"?). We need this earlier to load worlds from fuel, hence we need to create our own callback for sdf and manage a FuelClient (to avoid instantiating it at every call).

Once Gazebo switches its behavior to using a local config for SDF its config (and its callbacks) will be used whenever sdf gets handed over to Gazebo. Assuming a sensible implementation, this would mean re-iterating over the sdf with the local config and resolving/finding resources using Gazebo's FuelClient. Theoretically, this means lazy parsing and might mean that we don't need a FuelClient anymore, as the fuel reference could be resolved later by Gazebo. We need to wait and see if they will actually do this though :)

In this state, the only way to call World::insertModel again would be instantiating a new GazeboSimulator object, that i believe will override the callback proving a fresh new FuelClient object, and get the new World. Do you have any other edge case in mind?

The problem is that the sdf config is global and doesn't provide a unregister function. This means that the old callback will linger as long as the sdf library isn't reloaded (globals -.-), even though the old FuelClient may have been destroyed. I'm not sure what exactly will happen actually. Either there will be a segfault because the reference in the lambda function is null, or there will be leaking memory, because the entire simulator instance (this) will be kept around due to the callback still being registered. Undesirable either way. Ignition gazebo, too, suffers from the same problem, btw. It will register a callback and if it gets destroyed, without the thread exiting, nobody knows how SDF will behave.

You are correct that a new gazebo object will register a new callback with a fresh FuelClient, but unless the above can be resolved, the only way to have multiple instances in the same process is to make sure that they all remain open until the application terminates.

@diegoferigo
Copy link
Member

To recap, if I understood correctly, sdformat <= 10 requires instantiating FuelClient and registering the global callback (as done in this PR), instead sdformat >= 11 allows passing to Root::Load a local ParserConfig object with additional callbacks so that it does not rely on the global resources.

Very close, yes. Iirc, the local config replaces the global one if present (they are exclusive).

Whenever sdf looks for a resource it first searches paths internally (afaik environment variables are handled here). If its internal method doesn't work, it will start going through all registered FindCallbacks (in the order they were created) until a callback responds with a non-empty string, and then it will use that string as location of the resource.

Gazebo creates it's own callback to load fuel models, but it only gets added once the server instance is created (for us that should be at "GazeboServer::Initialize"?). We need this earlier to load worlds from fuel, hence we need to create our own callback for sdf and manage a FuelClient (to avoid instantiating it at every call).

Once Gazebo switches its behavior to using a local config for SDF its config (and its callbacks) will be used whenever sdf gets handed over to Gazebo. Assuming a sensible implementation, this would mean re-iterating over the sdf with the local config and resolving/finding resources using Gazebo's FuelClient. Theoretically, this means lazy parsing and might mean that we don't need a FuelClient anymore, as the fuel reference could be resolved later by Gazebo. We need to wait and see if they will actually do this though :)

Makes sense to me, thanks for expanding. I agree with doing this by steps, let's rely on global for now, and switch to local with either Edifice or Fortress. Once we switch to Edifice, I fear that the alignment of the vendored Physics system will break the (unofficial) backward compatibility with Dome and Citadel, but it's fine. This is to say that if we want, we can directly use sdformat11 features directly from the next release that will be compatible with Edifice.

In this state, the only way to call World::insertModel again would be instantiating a new GazeboSimulator object, that i believe will override the callback proving a fresh new FuelClient object, and get the new World. Do you have any other edge case in mind?

The problem is that the sdf config is global and doesn't provide a unregister function. This means that the old callback will linger as long as the sdf library isn't reloaded (globals -.-), even though the old FuelClient may have been destroyed. I'm not sure what exactly will happen actually. Either there will be a segfault because the reference in the lambda function is null, or there will be leaking memory, because the entire simulator instance (this) will be kept around due to the callback still being registered. Undesirable either way. Ignition gazebo, too, suffers from the same problem, btw. It will register a callback and if it gets destroyed, without the thread exiting, nobody knows how SDF will behave.

You are correct that a new gazebo object will register a new callback with a fresh FuelClient, but unless the above can be resolved, the only way to have multiple instances in the same process is to make sure that they all remain open until the application terminates.

Since this is not code that is supposed to run in a time-sensitive loop, what about doing the following:

    // Configure Fuel Callback
    sdf::setFindCallback([](const std::string& uri) -> std::string {
        auto fuelClient = ignition::fuel_tools::FuelClient();
        const auto path =
            ignition::fuel_tools::fetchResourceWithClient(uri, fuelClient);
        return path;
    });

We will in any case optimize it in devel by using the local sdf config as soon as we update to Edifice.

@FirefoxMetzger
Copy link
Contributor Author

Since this is not code that is supposed to run in a time-sensitive loop

I don't know if this is always true actually. Since this is a global callback inside sdf it may get called at any time. For example, if I run a simulation that inserts objects dynamically as the simulation progresses (also levels) and they include fuel URIs then the callback will get called at the speed at which objects are inserted. Hopefully, this will happen rarely but we can't know that for certain.

We could make the fuel client a global singleton that is shared. I'm also not sure if I am overly scared on this point. I just tried opening and closing multiple instances with the current implementation

>>> for _ in range(100):
...     gazebo = scenario_gazebo.GazeboSimulator(step_size=0.001, rtf=1.0, steps_per_run=1)
...     assert gazebo.insert_world_from_sdf("./sdf/environment.sdf")
...     gazebo.initialize()
...     gazebo.close()
... 

and I saw neither increasing memory usage nor a segfault.

@diegoferigo
Copy link
Member

Since this is not code that is supposed to run in a time-sensitive loop

I don't know if this is always true actually. Since this is a global callback inside sdf it may get called at any time. For example, if I run a simulation that inserts objects dynamically as the simulation progresses (also levels) and they include fuel URIs then the callback will get called at the speed at which objects are inserted. Hopefully, this will happen rarely but we can't know that for certain.

If I understood the process, this callback is called only when no local resource is found. This means that even in the case you described, any slow down related to fuel would affect only the first run. Once the models, meshes, ... will be be already locally available, the callback should not get called. In any case, I suspect that the time to download a resource is much longer than the time to allocate the FuelClient.

We could make the fuel client a global singleton that is shared.

I was thinking the same:

    // Configure Fuel Callback
    sdf::setFindCallback([](const std::string& uri) -> std::string {
        static auto fuelClient = ignition::fuel_tools::FuelClient();
        const auto path =
            ignition::fuel_tools::fetchResourceWithClient(uri, fuelClient);
        return path;
    });

It should work, even though globals are globals, never nice to have.

I'm also not sure if I am overly scared on this point. I just tried opening and closing multiple instances with the current implementation

>>> for _ in range(100):
...     gazebo = scenario_gazebo.GazeboSimulator(step_size=0.001, rtf=1.0, steps_per_run=1)
...     assert gazebo.insert_world_from_sdf("./sdf/environment.sdf")
...     gazebo.initialize()
...     gazebo.close()
... 

and I saw neither increasing memory usage nor a segfault.

Why would you expect either increase in memory usage or segfaults? With the current implementation I would not expect any leak due to the callback since the FuelClient is destroyed in gazebo.close(), and in the next loop cycle the callback is updated with the new pointer, preventing segfaults (maybe I misunderstood what you meant).

@FirefoxMetzger
Copy link
Contributor Author

this callback is called only when no local resource is found.

Almost. sdformat will call the callback every time it encounters an unknown resource in the hopes of finding it. You are right that models will only be downloaded once, but the callback itself will be executed each time the URL is encountered causing a new FuelClient to be created, which converts the URL into a path on the local filesystem (without downloading anything).

We are discussing a hypothetical scenario and are probably doing premature optimization. Maybe it is easier to just create a new instance and see if somebody struggles with a slow simulation because of it. We can always fall back to the global singleton in that case, which might get refactored once Edifice/Fortress becomes a thing.

Why would you expect either increase in memory usage or segfaults?

I was testing the implementation where the FuelClient lives at GazeboSimulator::Impl:.FuelClient and I was worried that, once the fuel client is destroyed, the old callback should still be there (registered globally) but won't have a client to use when it gets executed. I thought this might segfault if FuelClient is destroyed or prevent the FuelClient from being destructed. I am probably just confusing myself 🤷

Let's go with FuelClient as a local variable and see if it causes undesirable amounts of slowdown.

Copy link
Member

@diegoferigo diegoferigo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Do you prefer to clean the commit history or should I proceed with squash-and-merge?

@FirefoxMetzger
Copy link
Contributor Author

You can do a squash-merge :) This way you can have the commit message style that makes the most sense to you.

You could add me as a co-author if you like by adding

Co-authored-by: FirefoxMetzger <[email protected]>

at the end of the squashed commit message. This way there is a log about who might know about this behavior. (Maybe git does this automatically?)

@diegoferigo diegoferigo merged commit f375db8 into robotology:devel Mar 31, 2021
@diegoferigo
Copy link
Member

For future reference, squash and merge already sets the author of the commit to the user that opened the PR. Adding the co-author (as I did) is not necessary.

@diegoferigo
Copy link
Member

Merged, thanks for the contribution @FirefoxMetzger!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants