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

[Garden] Download models in the background #1484

Closed
wants to merge 83 commits into from

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented May 13, 2022

🎉 New feature

Summary

it requires gazebosim/sdformat#1027

I was trying to add this functionality to Citadel, but we need to load a simple world when we are downloading the models, this simple world will add a name to the Scene which in most of the cases is not going to match with the desired world name.

This will generate that SceneBroadcaster will create some topics with undesired names. Using the reset API I can create the desired topics but, the old topics are still there.

Example:

/clock
/gazebo/resource_paths
/gui/camera/pose
/stats
/world/empty_world/clock
/world/empty_world/dynamic_pose/info
/world/empty_world/pose/info
/world/empty_world/scene/deletion
/world/empty_world/scene/info
/world/empty_world/state
/world/empty_world/stats
/world/shapes/dynamic_pose/info
/world/shapes/pose/info
/world/shapes/scene/deletion
/world/shapes/scene/info
/world/shapes/state

Another issue happens in the Physics thread, sometimes some links are not added. Myabe a race condition ?

It builds on top of #1327

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.

mjcarroll and others added 30 commits February 21, 2022 13:56
Reduces the size of simulationrunner header a bit

Signed-off-by: Michael Carroll <michael@openrobotics.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>

Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll
Copy link
Contributor

I think that the merge is okay. It may be cleaner to rebase on main at this point, though?

Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
src/SystemManager.cc Outdated Show resolved Hide resolved
ahcorde and others added 3 commits July 7, 2022 14:33
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
@mjcarroll
Copy link
Contributor

This seems odd, did it come in from the merge from main?

/github/workspace/src/Server.cc: In member function 'void gz::sim::v7::Server::Init()':
  /github/workspace/src/Server.cc:207:43: error: 'class sdf::v13::Root' has no member named 'WorldNameFromFile'
    207 |     auto errors2 = this->dataPtr->sdfRoot.WorldNameFromFile(

@ahcorde
Copy link
Contributor Author

ahcorde commented Jul 8, 2022

This seems odd, did it come in from the merge from main?

/github/workspace/src/Server.cc: In member function 'void gz::sim::v7::Server::Init()':
  /github/workspace/src/Server.cc:207:43: error: 'class sdf::v13::Root' has no member named 'WorldNameFromFile'
    207 |     auto errors2 = this->dataPtr->sdfRoot.WorldNameFromFile(

@mjcarroll This is not merged yet gazebosim/sdformat#1027

@mjcarroll
Copy link
Contributor

@mjcarroll This is not merged yet gazebosim/sdformat#1027

Ah, sorry didn't catch that. Looks like it has an approval, do you want to merge/release or wait for @chapulina ?

@chapulina
Copy link
Contributor

do you want to merge/release or wait for @chapulina ?

Don't wait for me on the SDF PR 🚢

Copy link
Contributor

@jennuine jennuine left a comment

Choose a reason for hiding this comment

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

Just an initial pass

examples/worlds/fuel.sdf Outdated Show resolved Hide resolved
examples/worlds/fuel.sdf Show resolved Hide resolved
src/SdfEntityCreator.cc Outdated Show resolved Hide resolved
Comment on lines +89 to +91
errors = root.LoadSdfString(
this->dataPtr->config.SdfString());
this->dataPtr->sdfRoot = root.Clone();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use this->dataPtr->sdfRoot directly?

Suggested change
errors = root.LoadSdfString(
this->dataPtr->config.SdfString());
this->dataPtr->sdfRoot = root.Clone();
errors = this->dataPtr->sdfRoot.LoadSdfString(
this->dataPtr->config.SdfString());

Below as well

errors = root.Load(filePath);
this->dataPtr->sdfRoot = root.Clone();

if (!this->dataPtr->config.DownloadInParallel())
Copy link
Contributor

Choose a reason for hiding this comment

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

When does DownloadInParallel get set to false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in src/gz.cc

Copy link
Contributor

Choose a reason for hiding this comment

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

I see it get set to true but I never see if it get set to false

src/Server.cc Outdated Show resolved Hide resolved
src/SimulationRunner.hh Outdated Show resolved Hide resolved
src/SystemManager.cc Outdated Show resolved Hide resolved
include/gz/sim/ServerConfig.hh Outdated Show resolved Hide resolved
src/SdfEntityCreator.cc Show resolved Hide resolved
@chapulina chapulina added bug Something isn't working and removed needs upstream release Blocked by a release of an upstream library labels Jul 25, 2022
@chapulina chapulina changed the base branch from main to gz-sim7 August 10, 2022 18:20
Signed-off-by: Nate Koenig <nate@openrobotics.org>
@nkoenig nkoenig mentioned this pull request Aug 26, 2022
9 tasks
@nkoenig
Copy link
Contributor

nkoenig commented Aug 26, 2022

@ahcorde see #1669 as an alternative that doesn't need the restart feature.

@azeey azeey added the beta Targeting beta release of upcoming collection label Jul 31, 2023
@azeey
Copy link
Contributor

azeey commented Aug 18, 2023

Closing in favor of #1669

@azeey azeey closed this Aug 18, 2023
@azeey azeey removed the beta Targeting beta release of upcoming collection label Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 🌱 garden Ignition Garden
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Download Fuel models on the background
7 participants