Resolve Fuel resources when included in models and worlds #309
Conversation
added reference to Support Policy
…docs improve insertWorldFromSDF docs
There was a "h" missing in the link to codacy. I've added it
fix broken link typo
|
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. |
|
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 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>
|
There was a problem hiding this comment.
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.
diegoferigo
changed the title
|
Renamed the PR since basic fuel support was introduced in #158. |
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).
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.
Both issues are likely related. It also seems to be the source of |
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 In a nutshell, it also ensures that later calls to, say, Edit: This may actually cause problems when the simulator instance is destroyed, the sdf binary remains loaded, and another call to |
Looks good to me.
Regarding to that, I also found gazebosim/gz-sim#286.
To recap, if I understood correctly, sdformat <= 10 requires instantiating
I missed this angle while reviewing this PR, thanks for providing more details. Renamed to Resolve Fuel resources when included in models and worlds.
This is true. However, if the simulator instance is destroyed, none of the resources will work properly (I mean, |
diegoferigo
changed the title
As per robotology-legacy#309 (comment) Co-authored-by: diegoferigo <diego.ferigo@iit.it>
Co-authored-by: Diego Ferigo <dgferigo@gmail.com>
… into fuel-support
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 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 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 :)
The problem is that the sdf config is global and doesn't provide a 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. |
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.
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 |
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. |
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
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.
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 |
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.
I was testing the implementation where the FuelClient lives at Let's go with FuelClient as a local variable and see if it causes undesirable amounts of slowdown. |
|
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 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?) |
|
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. |
|
Merged, thanks for the contribution @FirefoxMetzger! |
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 aFuelClientwhich became an attribute ofGazeboSimulatorto 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.