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

Config file loading options #796

Open
nkoenig opened this issue Apr 29, 2021 · 9 comments
Open

Config file loading options #796

nkoenig opened this issue Apr 29, 2021 · 9 comments
Assignees
Labels
bug Something isn't working help wanted We accept pull requests! OOBE 📦✨ Out-of-box experience

Comments

@nkoenig
Copy link
Contributor

nkoenig commented Apr 29, 2021

Environment

  • OS Version: Ubuntu 18.04
  • Source or binary build? Binary, Ignition Dome

Description

Running this world will cause Ignition Gazebo to hang at start because the included model has a plugin.

<?xml version="1.0" ?>
<sdf version="1.6">
  <world name="default">
    <include>
      <uri>https://fuel.ignitionrobotics.org/1.0/openrobotics/models/finals staging area</uri>
    </include>
  </world>
</sdf>

I believe the problem is that Ignition Gazebo sees the plugin, which in this case is a performer detector plugin, and doesn't load any additional plugins. Adding the following plugins to the world will make gazebo behave correctly.

    <plugin
      filename="ignition-gazebo-physics-system"
      name="ignition::gazebo::systems::Physics">
    </plugin>
    <plugin
      filename="ignition-gazebo-sensors-system"
      name="ignition::gazebo::systems::Sensors">
      <render_engine>ogre2</render_engine>
    </plugin>
    <plugin
      filename="ignition-gazebo-user-commands-system"
      name="ignition::gazebo::systems::UserCommands">
    </plugin>
    <plugin
      filename="ignition-gazebo-scene-broadcaster-system"
      name="ignition::gazebo::systems::SceneBroadcaster">
    </plugin>

I think we have an opt-in approach with plugins, which can cause strange and silent failures like this. An opt-out approach might be nicer, where an expert user can explicitly say that they don't want a plugin.

@nkoenig nkoenig added the bug Something isn't working label Apr 29, 2021
@chapulina
Copy link
Contributor

An option mentioned in #57 (comment) is to only consider world plugins when checking if plugins have already been loaded.

An opt-out approach might be nicer, where an expert user can explicitly say that they don't want a plugin.

An opt-out aproach requires the user to know all the possible plugins beforehand. So if we add a new plugin that's loaded by default, the user needs a way to know that they need to exclude it.

@nkoenig
Copy link
Contributor Author

nkoenig commented Apr 30, 2021

The opt-out idea is meant to make the default behavior do the right thing, and thereby improve the user experience. The trade off would be a more difficult time for expert users. There are probably numerous ways to implement this approach such as:

  1. A command line argument.
  2. A special plugin, whose existence means that Ignition Gazebo should only load the specified plugins.
  3. A gazebo specific extension to SDF.

@chapulina
Copy link
Contributor

I think we can split this into 2 separate use cases:

  1. User loads a world with only non-world plugins and expects the default world plugins to be loaded
  2. User loads a world with a few world plugins and expects other world plugins to be loaded by default alongside those

We can tackle the 1st use case with my suggestion, I don't think we have any reason not to load the default world plugins when there are model plugins loaded. This was overlooked in the initial implementation, I think we can consider it a bug and fix it in stable releases. @mjcarroll , can you think of a reason not to do this? I believe this addresses the specific use case on this issue.

The 2nd use case needs more thought. The current behaviour is not to load any world plugin by default if the user has specified at least one world plugin (except when recording or playing back a log, we always load the necessary plugins for you). This makes it complicated to append plugins to the default configuration. I think it would be useful to have a mechanism for appending plugins instead of substituting. I think we could offer both options 1. (command line) and 3. (SDF) to configure this. I'm afraid that appending by default would break current users though, so I'd recommend we offer the option to turn on appending on stable releases, and flip the default behaviour on Fortress to be more beginner-friendly.

I also think it would be handy to have an option to ignore plugins specified on SDF. For example, you're loading an SDF world that has buoyancy handcoded into it and you want to disable that without editing the file.

I also think we should make the behaviour consistent between GUI and server configs.

My suggestion:

Loading a world that has:

  • world plugin A on the SDF
  • world plugin B on the config

Up to Edifice:

  • ign gazebo world.sdf
    • Load only A (current behaviour)
  • ign gazebo --append-server-config
    • Load A and B
  • ign gazebo --force-server-config
    • Load only B

From Fortress:

  • ign gazebo world.sdf
    • Load A and B
  • ign gazebo --append-server-config
    • Load A and B, deprecation warning saying that the flag isn't needed anymore
  • ign gazebo --force-server-config
    • Load only B
  • ign gazebo --ignore-server-config
    • Load only A

@nkoenig
Copy link
Contributor Author

nkoenig commented Oct 5, 2021

Would it be possible to prioritize a resolution for this bug? There are many models on Fuel that cause Gazebo to hang if they are included.

@chapulina
Copy link
Contributor

I'm planning to make some time to wrap up #1012 this month 👍 Please comment there if you have a good idea on how to handle this situation: #1012 (comment)

@chapulina chapulina added the OOBE 📦✨ Out-of-box experience label Dec 20, 2021
@chapulina chapulina self-assigned this Mar 10, 2022
@chapulina
Copy link
Contributor

I think we can split this into 2 separate use cases:

  1. User loads a world with only non-world plugins and expects the default world plugins to be loaded
  2. User loads a world with a few world plugins and expects other world plugins to be loaded by default alongside those

@chapulina
Copy link
Contributor

I think that backporting #1383 to Citadel would be too much work, so I say we don't do it.

All that's left to close this issue is completing #1012

@chapulina chapulina changed the title Gazebo hangs when loading a model with a plugin Config file loading options Apr 6, 2022
@chapulina chapulina removed their assignment Apr 29, 2022
@chapulina chapulina self-assigned this May 4, 2022
@chapulina chapulina removed their assignment Jul 7, 2022
@chapulina chapulina added the help wanted We accept pull requests! label Jul 7, 2022
@azeey azeey self-assigned this Jun 27, 2023
@azeey azeey moved this from Inbox to In progress in Core development Jul 22, 2024
@azeey
Copy link
Contributor

azeey commented Jul 24, 2024

I'm going to attempt the gz specific SDF tag approach since that can:

  1. Allow more fine grained control
  2. Can be specified in the SDF world by the author, so other users don't have to remember to specify the appropriate command line options

My approach is to define //plugin/[@gz:config_action] ,i.e, <plugin gz:config_action='some_value'>, with the following values:

  • append_replace: append this plugin to the end of the plugin list (default value). if a plugin with the same name exists in the default list, it gets replaced
  • prepend_replace: prepend this plugin to the beggining of the plugin list. if a plugin with the same name exists in the default list, it gets replaced
  • append: append this plugin to the end of the plugin list.
  • prepend: prepend this plugin to the beggining of the plugin list.

I'll also define /world/gz:policies, i.e, <world><gz:policies><some_value></gz:policies></world>. This will be a general place to add policies that affect behavior similar to https://cmake.org/cmake/help/book/mastering-cmake/chapter/Policies.html

Inside <gz:policies> we will have elements:

  • <no_default_server_plugins>: Default server plugins will not be loaded. Only the plugins in the SDF file will be loaded instead. This will be defaulted to false in Ionic, but true in Harmonic
  • <no_default_gui_plugins>: Default gui plugins will not be loaded. Only the plugins in the SDF file will be loaded instead. This will be defaulted to false in Ionic, but true in Harmonic

The default behavior in Ionic will be that plugins in the SDF file are appended to the default list with replacement. I think this is a sane default as it covers most use cases.

@iche033
Copy link
Contributor

iche033 commented Aug 1, 2024

Some thoughts after going over the proposed approach in #796 (comment):

  • prepend / append makes sense for gui plugins. For server plugins, ideally the users should use the new <gz:system_priority> (Specify System::PreUpdate, Update execution order #2487) tag instead to specify order to execution. I think a tutorial helps here.
  • <no_default_server_plugins>false</no_default_server_plugins>: took me a sec to resolve the double negative the first time (probably just me). How about using <include_default_server_plugins> instead? No strong opinion on this one.
  • Optimization: exclude scene broadcaster from the default list of server plugins. Only load it when GUI or logging is enabled.
  • Future user experience consideration: Load plugins needed for sensors automatically, e.g. if <sensor type="camera" ..> exists in sdf, load the sensors system without needing users to specify it (closer to how classic works, smoother migration process). We then need to provide a way for advanced users to override and use their own sensors systems if they don't want to use the default one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted We accept pull requests! OOBE 📦✨ Out-of-box experience
Projects
Status: In progress
Development

Successfully merging a pull request may close this issue.

4 participants