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.

Sign up for GitHub

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

Jump to bottom

Automatically load a subset of world plugins #281

Merged
merged 32 commits into from
Jan 8, 2021
Merged

Automatically load a subset of world plugins #281

merged 32 commits into from
Jan 8, 2021

Conversation

mjcarroll
Copy link
Contributor

Addresses #57

@chapulina chapulina added 🔮 dome Ignition Dome migration Helps with migration from Gazebo classic to Ignition labels Aug 11, 2020
@chapulina chapulina mentioned this pull request Aug 13, 2020
Merged
@chapulina chapulina added the beta Targeting beta release of upcoming collection label Sep 8, 2020
@mjcarroll mjcarroll force-pushed the default_plugin_loading branch from 9acc99e to 00c5419 Compare September 9, 2020 18:51
@mjcarroll mjcarroll changed the base branch from master to ign-gazebo3 September 9, 2020 18:53
@mjcarroll mjcarroll force-pushed the default_plugin_loading branch from 00c5419 to ca51a90 Compare September 9, 2020 18:53
@mjcarroll mjcarroll marked this pull request as ready for review September 14, 2020 18:28
@chapulina chapulina added 🏰 citadel Ignition Citadel and removed 🔮 dome Ignition Dome labels Sep 15, 2020
chapulina
chapulina reviewed Sep 15, 2020
View reviewed changes
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Works for me!

Can you fix conflicts?

Also, since this changes a lot the way ign-gazebo is used and adds functionality that's difficult to discover, what do you think of writing a tutorial?

include/ignition/gazebo/ServerConfig.hh Show resolved Hide resolved
include/ignition/gazebo/config.hh.in Show resolved Hide resolved
include/ignition/gazebo/server.config Outdated Show resolved Hide resolved
src/ServerConfig.cc Show resolved Hide resolved
src/ServerConfig.cc Outdated Show resolved Hide resolved
src/SimulationRunner.cc Outdated Show resolved Hide resolved
src/SimulationRunner.cc Outdated Show resolved Hide resolved
src/SimulationRunner.cc Outdated Show resolved Hide resolved
src/SimulationRunner.cc Outdated Show resolved Hide resolved
src/SimulationRunner_TEST.cc Outdated Show resolved Hide resolved
@diegoferigo
Copy link
Contributor

Just checking @mjcarroll, I remember we already discussed the use case in #537. Would exporting an empty IGN_GAZEBO_SERVER_CONFIG be enough to prevent the load of these plugins? Do you think that adding a test for this use case could be worth?

chapulina
chapulina reviewed Sep 15, 2020
View reviewed changes
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Should the DefaultPlugins be removed from the DefaultWorld now that they will come from the config file?

https://github.com/ignitionrobotics/ign-gazebo/blob/375d626161ac4ca5b5ef0365f2f5357f498a924c/src/Server.cc#L62-L64

src/SimulationRunner.cc Outdated Show resolved Hide resolved
@chapulina chapulina linked an issue Sep 17, 2020 that may be closed by this pull request
Auto-load a subset of world plugins unless user opts-out #57
Closed
@mjcarroll mjcarroll force-pushed the default_plugin_loading branch from 6dd9275 to 77d102d Compare September 18, 2020 14:50
@mjcarroll
Copy link
Contributor Author

mjcarroll commented Sep 18, 2020 •
edited
Loading

Just checking @mjcarroll, I remember we already discussed the use case in #537. Would exporting an empty IGN_GAZEBO_SERVER_CONFIG be enough to prevent the load of these plugins? Do you think that adding a test for this use case could be worth?

@diegoferigo: Empty may currently be an issue, due to an upstream ign-common bug I found while implementing this: gazebosim/gz-common#96

To work around this in the short-term, exporting any content that doesn't point to a valid file should be sufficient to trigger this behavior. There are tests in ServerConfig_TEST.cc to cover this as well: https://github.com/ignitionrobotics/ign-gazebo/pull/281/files#diff-2c8a90ed6de1cbefd15d3af4914f0d2dR132-R142

@mjcarroll
Copy link
Contributor Author

Can you fix conflicts?

Done.

Also, since this changes a lot the way ign-gazebo is used and adds functionality that's difficult to discover, what do you think of writing a tutorial?

Will do.

@chapulina chapulina removed the beta Targeting beta release of upcoming collection label Sep 21, 2020
@chapulina
Copy link
Contributor

I'm removing this from the Dome beta release. We're already in feature freeze and code freeze is in 2 days. This is a backwards-compatible feature that can be merged into Citadel then merged forward to Dome after the release.

@chapulina chapulina mentioned this pull request Oct 9, 2020
Merged
chapulina
chapulina suggested changes Oct 13, 2020
View reviewed changes
Copy link
Contributor

@chapulina chapulina left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Compilation was failing, fixed in d2a18d1

How do you feel about removing the plugins from all example worlds that are using the default ones in this PR?

I'll work on a tutorial.

src/ServerConfig.cc Outdated Show resolved Hide resolved
src/ServerConfig.cc Outdated Show resolved Hide resolved
src/ServerConfig.cc Outdated Show resolved Hide resolved
src/ServerConfig.cc Outdated Show resolved Hide resolved
src/SimulationRunner.cc Outdated Show resolved Hide resolved
src/ServerConfig_TEST.cc Outdated Show resolved Hide resolved
src/ServerConfig_TEST.cc Outdated Show resolved Hide resolved
src/SimulationRunner.cc Outdated Show resolved Hide resolved
src/SimulationRunner_TEST.cc Outdated Show resolved Hide resolved
src/SimulationRunner_TEST.cc Outdated Show resolved Hide resolved
chapulina
chapulina suggested changes Oct 14, 2020
View reviewed changes
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Should the DefaultPlugins be removed from the DefaultWorld now that they will come from the config file?

Yeah that's necessary so that server.config is picked up when no SDF file is loaded. Addressed in #405 , which also has a tutorial.

include/ignition/gazebo/server_playback.config Outdated Show resolved Hide resolved
src/ServerConfig.cc Outdated Show resolved Hide resolved
@mjcarroll mjcarroll requested a review from maryaB-osr as a code owner October 14, 2020 13:43
@mjcarroll mjcarroll mentioned this pull request Oct 14, 2020
Merged
@mjcarroll mjcarroll force-pushed the default_plugin_loading branch from a4fa69b to 5727e16 Compare October 21, 2020 17:22
@mjcarroll mjcarroll requested a review from azeey as a code owner October 21, 2020 17:22
mjcarroll and others added 16 commits January 6, 2021 07:50
@mjcarroll
server_playback -> playback_server
d3fa176 
Signed-off-by: Michael Carroll <[email protected]>
@mjcarroll
Remove "resolved" variable
a0b1af4 
Signed-off-by: Michael Carroll <[email protected]>
@mjcarroll
Update error message
af0a710 
Signed-off-by: Michael Carroll <[email protected]>
@mjcarroll
Fixup things using Util include
0a4e2d8 
Signed-off-by: Michael Carroll <[email protected]>
@mjcarroll
Use common facilities
21b8245 
* joinPaths rather than string concatenation
* (env, setenv, unsetenv) where possible

Signed-off-by: Michael Carroll <[email protected]>
@mjcarroll
Reviewer feedback.
e16830d 
Signed-off-by: Michael Carroll <[email protected]>
@mjcarroll
Fix 2 tests
ef6a115 
Signed-off-by: Michael Carroll <[email protected]>
@mjcarroll
Fixup log recording/playback interactions
c974174 
Signed-off-by: Michael Carroll <[email protected]>
@mjcarroll
Linting, documentation, remove debug output
1af9321 
Signed-off-by: Michael Carroll <[email protected]>
@mjcarroll
Lint and remove dead code
f939b42 
Signed-off-by: Michael Carroll <[email protected]>
@chapulina @mjcarroll
Suggestions to #281 (#508)
79d3a2d 
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
@mjcarroll
Fix logging interaction with loading
6e76e7c 
Signed-off-by: Michael Carroll <[email protected]>
@mjcarroll
Revert "Fix logging interaction with loading"
b1597ac 
This reverts commit fe4eda9.

Signed-off-by: Michael Carroll <[email protected]>
@mjcarroll
Fix log interactions with default plugin loading
1961be0 
Signed-off-by: Michael Carroll <[email protected]>
@mjcarroll
Fix codecheck/lint
b36e403 
Signed-off-by: Michael Carroll <[email protected]>
@mjcarroll
Address reviewer feedback
8e40b07 
Signed-off-by: Michael Carroll <[email protected]>
@mjcarroll mjcarroll force-pushed the default_plugin_loading branch from 7d2d9da to 8e40b07 Compare January 6, 2021 13:51
@mjcarroll
Copy link
Contributor Author

I noticed that scenebroadcaster test seems to be failing over here as well: #531, so I don't think it's related to my changes?

@mjcarroll
Copy link
Contributor Author

Holding this until 3->4 forward ports are done

@mjcarroll mjcarroll merged commit 6b6f76f into ign-gazebo3 Jan 8, 2021
@mjcarroll mjcarroll deleted the default_plugin_loading branch January 8, 2021 14:28
@mjcarroll mjcarroll mentioned this pull request Jan 8, 2021
Closed
mjcarroll added a commit that referenced this pull request Jan 8, 2021
@mjcarroll @chapulina
Automatically load a subset of world plugins (#281)
1ac1b95 
Feature to allow loading of a default set of system plugins from a file. This behavior will trigger when a world sdf file is loaded with no plugins defined.  In this case, the simulator will load the plugins from a series of locations including environment variable, the users home folder, and finally in the installation directory.

This should allow users to not have to specify the same set of plugins in every world sdf file.

Signed-off-by: Michael Carroll <[email protected]>
Co-authored-by: Louise Poubel <[email protected]>
mjcarroll added a commit that referenced this pull request Jan 13, 2021
@mjcarroll @chapulina
Port: Automatically load a subset of world plugins (#281) (#537)
e9e9f19 
Feature to allow loading of a default set of system plugins from a file. This behavior will trigger when a world sdf file is loaded with no plugins defined.  In this case, the simulator will load the plugins from a series of locations including environment variable, the users home folder, and finally in the installation directory.

This should allow users to not have to specify the same set of plugins in every world sdf file.

Signed-off-by: Michael Carroll <[email protected]>
Co-authored-by: Louise Poubel <[email protected]>
nkoenig added a commit that referenced this pull request Jan 13, 2021
@nkoenig @mjcarroll
@chapulina @nlamprian
Nkoenig/3 to 4 20210113 (#554)
6fcbcea 
* Automatically load a subset of world plugins (#281)

Feature to allow loading of a default set of system plugins from a file. This behavior will trigger when a world sdf file is loaded with no plugins defined.  In this case, the simulator will load the plugins from a series of locations including environment variable, the users home folder, and finally in the installation directory.

This should allow users to not have to specify the same set of plugins in every world sdf file.

Signed-off-by: Michael Carroll <[email protected]>
Co-authored-by: Louise Poubel <[email protected]>

* Added missing version namespace (#541)

* Added missing version namespace

Signed-off-by: Nate Koenig <[email protected]>

* Fix codecheck

Signed-off-by: Nate Koenig <[email protected]>

Co-authored-by: Nate Koenig <[email protected]>

* Fix examples in migration plugins tutorial (#543)

Signed-off-by: Nick Lamprianidis <[email protected]>

* Prepare for 3.7.0 release (#552)

Signed-off-by: Nate Koenig <[email protected]>

Co-authored-by: Nate Koenig <[email protected]>

Co-authored-by: Michael Carroll <[email protected]>
Co-authored-by: Louise Poubel <[email protected]>
Co-authored-by: Nate Koenig <[email protected]>
Co-authored-by: Nick Lamprianidis <[email protected]>
@luca-della-vedova luca-della-vedova mentioned this pull request Feb 2, 2021
Merged
@diegoferigo diegoferigo mentioned this pull request Feb 3, 2021
Closed
@diegoferigo diegoferigo mentioned this pull request Aug 2, 2021
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chapulina chapulina chapulina approved these changes

@maryaB-osr maryaB-osr Awaiting requested review from maryaB-osr

Assignees

@mjcarroll mjcarroll

Labels
🏰 citadel Ignition Citadel migration Helps with migration from Gazebo classic to Ignition
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Auto-load a subset of world plugins unless user opts-out
3 participants
@mjcarroll @diegoferigo @chapulina