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

A more minimal Ignition to Gazebo server rename effort #263

Merged
merged 2 commits into from
Aug 15, 2022

Conversation

nkoenig
Copy link
Contributor

@nkoenig nkoenig commented Jun 13, 2022

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

🎉 New feature

Summary

For Citadel and Fortress users I'd like to achieve the following goals:

  1. Assets that reference fuel.ignitionrobotics.org continue to work
  2. Assets the reference fuel.gazebosim.org work
  3. Assets installed in ~/.ignition/fuel

Test it

  1. Clear your ~/.ignition/fuel directory.
  2. Run this world file, which uses a fuel.gazebosim.org URI. The included model depends on another model whose URI uses fuel.ignitionrobotics.org.
<?xml version="1.0" ?>
<sdf version="1.6">
<world name="shapes">
  <plugin
    filename="ignition-gazebo-physics-system"
    name="ignition::gazebo::systems::Physics">
  </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>

  <scene>
  <ambient>1.0 1.0 1.0</ambient>
  <background>0.8 0.8 0.8</background>
  </scene>

  <include>
    <uri>https://fuel.gazebosim.org/1.0/OpenRobotics/models/Tunnel Tile 4</uri>
  </include>
</world>
</sdf>
  1. You should see Tunnel Tile 4 in ~/.ignition/fuel/fuel.gazebosim.org/openrobotics, and tunnel tile 1 in ~/.ignition/fuel/fuel.ignitionrobotics.org/openrobotics.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

@nkoenig nkoenig added the ign to gz Renaming Ignition to Gazebo. label Jun 13, 2022
@github-actions github-actions bot added Gazebo 1️1️ Dependency of Gazebo classic version 11 🏰 citadel Ignition Citadel labels Jun 13, 2022
@codecov
Copy link

codecov bot commented Jun 13, 2022

Codecov Report

Merging #263 (e14504c) into ign-fuel-tools4 (dd94c11) will increase coverage by 0.05%.
The diff coverage is 90.90%.

❗ Current head e14504c differs from pull request most recent head 352f42d. Consider uploading reports for the commit 352f42d to get more accurate results

@@                 Coverage Diff                 @@
##           ign-fuel-tools4     #263      +/-   ##
===================================================
+ Coverage            76.15%   76.21%   +0.05%     
===================================================
  Files                   19       19              
  Lines                 2718     2724       +6     
===================================================
+ Hits                  2070     2076       +6     
  Misses                 648      648              
Impacted Files Coverage Δ
src/LocalCache.cc 79.52% <ø> (-0.07%) ⬇️
src/FuelClient.cc 72.76% <80.00%> (+0.03%) ⬆️
src/ClientConfig.cc 85.02% <100.00%> (+0.26%) ⬆️
src/ModelIdentifier.cc 92.44% <100.00%> (+0.08%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@chapulina chapulina self-requested a review June 14, 2022 16:03
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 as advertised

$ tree ~/.ignition/fuel/ -L 4
/home/chapulina/.ignition/fuel/
├── fuel.gazebosim.org
│   └── openrobotics
│       └── models
│           └── tunnel tile 4
└── fuel.ignitionrobotics.org
    └── openrobotics
        └── models
            └── tunnel tile 1

I just have some minor comments below, but nothing blocking.

@@ -538,12 +538,14 @@ TEST_F(FuelClientTest, DownloadModel)
EXPECT_EQ(ResultType::FETCH_ALREADY_EXISTS, res5.Type());
}

// Download model with a dependency specified within its `model.config`
// Download model with a dependency specified within its `model.config`.
// The dependency points to fuel.gazebosim.org.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it valuable to also test the other way around, i.e. a ignitionrobotics URL that depends on a gazebosim URL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reverse also works. I've just added the ability to use both fuel.ignitionrobotics.org and 'fuel.gazebosim.org' at the same time. As long as both server configurations are present, then it'll work.

src/FuelClient.cc Show resolved Hide resolved
ServerConfig gzServerConfig;
gzServerConfig.SetUrl(common::URI("https://fuel.gazebosim.org"));
gzServerConfig.SetVersion("1.0");
this->servers.push_back(gzServerConfig);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for completeness, is there an advantage to hardcoding it here as opposed to there

servers:
-
name: osrf
url: https://fuel.ignitionrobotics.org

In theory, I think that hardcoding it here makes it difficult, if not impossible for users of other servers to disable gazebosim.org. Not sure if any users at all care about that, but it's a use case supported to an extent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are a couple problems associated with the yaml config file.

  1. I don't think it's possible to use the yaml config file when running ign gazebo.
  2. I believe the config file is only used with ign fuel if the --config option is specified.
  3. If either fuel.ignitionrobotics.org or fuel.gazebosim.org are missing in the ClientConfig, then worlds/models that use both URIs will not load.

I don't see an easy way around this.

src/ClientConfig_TEST.cc Show resolved Hide resolved
@chapulina chapulina added the enhancement New feature or request label Jul 23, 2022
@chapulina chapulina added bug Something isn't working and removed enhancement New feature or request labels Aug 1, 2022
@nkoenig nkoenig merged commit 716c966 into ign-fuel-tools4 Aug 15, 2022
@nkoenig nkoenig deleted the minimal_ign_gz_server_update branch August 15, 2022 21:07
@chapulina chapulina mentioned this pull request Aug 16, 2022
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 🏰 citadel Ignition Citadel Gazebo 1️1️ Dependency of Gazebo classic version 11 ign to gz Renaming Ignition to Gazebo.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants