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

Fix ResourceSpawner #2490

Merged
merged 6 commits into from
Aug 8, 2024
Merged

Fix ResourceSpawner #2490

merged 6 commits into from
Aug 8, 2024

Conversation

mjcarroll
Copy link
Contributor

🦟 Bug fix

Fixes #2486

Summary

id.UniqueName() now returns without https:// prefixed, but we want to keep track of the URIs that the resources are downloaded from. These changes were added in gazebosim/gz-fuel-tools#376 and should have been accommodated here.

Checklist

  • Signed all commits for DCO
  • Added tests
  • 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

Signed-off-by: Michael Carroll <[email protected]>
@mjcarroll mjcarroll self-assigned this Jul 22, 2024
@github-actions github-actions bot added the 🎵 harmonic Gazebo Harmonic label Jul 22, 2024
Copy link
Contributor

@azeey azeey left a comment

Choose a reason for hiding this comment

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

Thanks for the fix. Just one issue

@@ -739,7 +739,7 @@ void ResourceSpawner::RunFetchResourceListThread(const std::string &_owner)
resource.isFuel = true;
resource.isDownloaded = false;
resource.owner = id.Owner();
resource.sdfPath = id.UniqueName();
resource.sdfPath = "https://" + id.UniqueName();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we avoid hard coding https://? Maybe get it from server somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually looking at this again, the UniqueName won't work on Windows (though gui plugins currently don't have any test coverage). Let me re-work it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I ended up adding a Url method to the Identifier objects in fuel_tools. This now depends on gazebosim/gz-fuel-tools#429

@logocar3
Copy link

Disclaimer - outsider needing this bug to be fixed.

There is Url property in ServerConfig object.

Change the line, where build fails:
742 | resource.sdfPath = id.Url().Str();

Into
742 | resource.sdfPath = id.Server().Url().Str();

Regards

@mjcarroll
Copy link
Contributor Author

There is Url property in ServerConfig object.

That URL only stores the base server URL. In this case, we need the full URL to the asset in question.

@azeey azeey added the beta Targeting beta release of upcoming collection label Jul 29, 2024
@azeey azeey added the needs upstream release Blocked by a release of an upstream library label Jul 30, 2024
@azeey
Copy link
Contributor

azeey commented Jul 30, 2024

@mjcarroll looks like this needs a gz-fuel-tool9 release.

@azeey azeey enabled auto-merge (squash) August 8, 2024 01:07
@azeey azeey merged commit 523b01b into gz-sim8 Aug 8, 2024
7 of 9 checks passed
@azeey azeey deleted the mjcarroll/fix_resource_spawner branch August 8, 2024 03:05
@usmanNoor5
Copy link

[ruby $(which gz) sim-2] Error [SDF.cc:165] Tried to use callback in sdf::findFile(), but the callback is empty. Did you call sdf::setFindCallback()?

how to handle this problem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta Targeting beta release of upcoming collection 🎵 harmonic Gazebo Harmonic needs upstream release Blocked by a release of an upstream library
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Regression in Resource Spawner in Harmonic
6 participants