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

Add Fuel World Support #274

Merged
merged 14 commits into from
Aug 14, 2020
Merged

Add Fuel World Support #274

merged 14 commits into from
Aug 14, 2020

Conversation

JShep1
Copy link

@JShep1 JShep1 commented Aug 6, 2020

This PR adds functionality for the user to specify a fuel world URL after ign gazebo, ie, ign gazebo https://staging-fuel.ignitionrobotics.org/1.0/gmas/worlds/ShapesClone

It will first check for a cached copy of the world, and then download the world if no cached copy is found. After this, it will search for an sdf file on the locally downloaded/cached path and load the first one found.

See also gazebosim/gz-launch#43

Closes #89

John Shepherd added 2 commits July 30, 2020 17:55
Signed-off-by: John Shepherd <[email protected]>
@github-actions github-actions bot added the 📜 blueprint Ignition Blueprint label Aug 6, 2020
John Shepherd added 3 commits August 5, 2020 17:26
@JShep1 JShep1 marked this pull request as ready for review August 6, 2020 00:48
@JShep1 JShep1 requested a review from chapulina as a code owner August 6, 2020 00:48
@JShep1 JShep1 requested a review from nkoenig August 6, 2020 00:49
@JShep1 JShep1 mentioned this pull request Aug 6, 2020
@JShep1 JShep1 linked an issue Aug 6, 2020 that may be closed by this pull request
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.

The example given on the PR works for me.

Worlds with spaces

I added .sdf files to the test worlds on Fuel that only had .world until now:

But I'm not being able to load either of them. I tried:

ign gazebo -v 4 https://fuel.ignitionrobotics.org/1.0/OpenRobotics/fuel/worlds/Test%20world

and

ign gazebo -v 4 "https://fuel.ignitionrobotics.org/1.0/OpenRobotics/fuel/worlds/Test world"

I didn't dig in to find out what the problem is, I suspect it's about encoding the space

Tests

Do you mind adding a test to ign_TEST.cc?

In order to avoid relying on the live server for the test, you could just test the cached case, and point to a custom cache inside the test folder, using the IGN_FUEL_CACHE_PATH variable to point there.

Loading file programmatically

With the current implementation, it isn't possible to load a Fuel world if it's passed to ServerConfig::SetSdfFile. I see on the ign-launch PR that you're resolving the world file with Fuel before passing it to ServerConfig. I think it would be more general to just support that directly inside ServerConfig. Do you think you could move that logic here?

When you do, it would also be good to add a test to Server_TEST.cc

src/ign.cc Outdated Show resolved Hide resolved
src/ign.cc Outdated Show resolved Hide resolved
src/ign.cc Outdated Show resolved Hide resolved
@JShep1
Copy link
Author

JShep1 commented Aug 12, 2020

But I'm not being able to load either of them. I tried:

ign gazebo -v 4 https://fuel.ignitionrobotics.org/1.0/OpenRobotics/fuel/worlds/Test%20world

and

ign gazebo -v 4 "https://fuel.ignitionrobotics.org/1.0/OpenRobotics/fuel/worlds/Test world"

I didn't dig in to find out what the problem is, I suspect it's about encoding the space

I think the fuel part of those URLs isn't needed. It finds and downloads successfully with the URL https://fuel.ignitionrobotics.org/1.0/OpenRobotics/worlds/Test world

I'll make all remaining updates and changes now.

@chapulina
Copy link
Contributor

I think the fuel part of those URLs isn't needed

Ha, thanks! I copied that from the Bibtex on the world's page, which is wrong: https://app.ignitionrobotics.org/OpenRobotics/fuel/worlds/Test%20world

I opened a PR to fix it: https://gitlab.com/ignitionrobotics/web/app/-/merge_requests/62

John Shepherd added 4 commits August 12, 2020 14:01
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Copy link
Contributor

@nkoenig nkoenig left a comment

Choose a reason for hiding this comment

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

Works great.

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.

The functionality works. I just have some final comments about the tests and other tweaks.

src/ign.cc Outdated Show resolved Hide resolved
src/Server_TEST.cc Outdated Show resolved Hide resolved
src/Server_TEST.cc Outdated Show resolved Hide resolved
John Shepherd added 2 commits August 13, 2020 18:13
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
src/Server_TEST.cc Outdated Show resolved Hide resolved
Signed-off-by: John Shepherd <[email protected]>
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.

LGTM, feel free to merge with happy CI.

@JShep1 JShep1 merged commit 881aae7 into ign-gazebo2 Aug 14, 2020
@JShep1 JShep1 deleted the jshep1/add_fuel_world_support branch August 14, 2020 21:04
@chapulina chapulina mentioned this pull request Aug 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📜 blueprint Ignition Blueprint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Load worlds from Fuel
3 participants