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 asset gallery #149

Merged
merged 23 commits into from
Aug 24, 2023
Merged

Add fuel asset gallery #149

merged 23 commits into from
Aug 24, 2023

Conversation

luca-della-vedova
Copy link
Member

New feature implementation

Implemented feature

This PR adds an asset gallery that is allows spawning models from the Gazebo App database.
To access the asset gallery navigate to Create -> New Model -> Browse fuel, as shown below:

Screencast.from.2023-07-14.14-49-12.webm

Implementation description

The core implementation of the fuel client is in a separate gz-fuel crate I wrote for the purpose. I'm happy to either release it, keep a source dependency or just copy the code here to remove the dependency. More detailed list of changes:

  • Added Asset gallery, including model preview widget based on a camera + preview model with a dedicated RenderLayer to make sure they are not visible to other views.
  • Assets can be filtered by owner, tag or private / public property. Filters are chained to allow more fine grained control of the visualized models. The entire database is above 2500 models so the feature is quite necessary.
  • The gz-fuel crate saves the model cache to a json file when possible (every platform except wasm). Users can trigger an update of the cache through a button in the GUI. Update is async and goes to a background task in the IO thread pool to avoid having too much overhead.
  • Support for private models in fuel, users will need to set an API key to access them.
  • More helpful error messages, hinting to users whether the model fails to visualize because it's not supported (i.e. dae files that are very prevalent in Fuel) or because it was not found in the path / fuel.
  • Event infrastructure to trigger a re-download of failed models if the API key is updated.
  • Improve support for SDF, previously only AssetSource::Search sdfs would work, this won't be the case anymore and both AssetSource::Local and AssetSource::Remote SDFs should work. For example browsing through the Local asset source to a model.sdf file should successfully spawn the asset to the site.
  • Minor cleanups to model / asset source logic. Models had a system that had two queries and iterated on both of them, trivially split into two systems with one query each to improve parallelization.

Note that sadly because a lot, or even the majority of, the models in fuel are in .dae and there is no support for Collada, a lot of models will actually fail to spawn but that is out of the scope of this PR.

luca-della-vedova and others added 21 commits July 5, 2023 13:38
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
assets

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Copy link
Collaborator

@mxgrey mxgrey left a comment

Choose a reason for hiding this comment

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

This is very awesome. I've just left a few comments, mostly about places where .unwrap() is used where I think we can have safer alternatives.

rmf_site_editor/Cargo.toml Outdated Show resolved Hide resolved
let binding = p.clone();
*p = if let Some(stripped) = uri.strip_prefix("model://") {
// Get the org name from context, model name from this and combine
let org_name = binding.split("/").next().unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm concerned about using .unwrap() here. Couldn't a user crash this by inputting some bad text? What if we do something like .unwrap_or("") and allow it to just fail to find the asset later in the pipeline?

Copy link
Member Author

Choose a reason for hiding this comment

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

For both this and the similar issue, changed it to return empty string but logging an error to denote exactly what went wrong, instead of doing it silently. It should be a bit more helpful in diagnosing issues 91a8256

rmf_site_editor/src/site/sdf.rs Outdated Show resolved Hide resolved
rmf_site_editor/src/site/sdf.rs Outdated Show resolved Hide resolved
rmf_site_editor/src/site/sdf.rs Outdated Show resolved Hide resolved
rmf_site_editor/src/site/sdf.rs Outdated Show resolved Hide resolved
rmf_site_editor/src/site/sdf.rs Outdated Show resolved Hide resolved
rmf_site_editor/src/site_asset_io.rs Outdated Show resolved Hide resolved
luca-della-vedova and others added 2 commits August 24, 2023 09:09
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
@mxgrey
Copy link
Collaborator

mxgrey commented Aug 24, 2023

I was ready to approve this PR until I did a manual test and discovered that something broke for model scaling. Here's a screenshot from the hotel demo world:

Screenshot from 2023-08-24 12-16-55

I might have mangled something while resolving merge conflicts. I'll see if I can fix it.

@mxgrey
Copy link
Collaborator

mxgrey commented Aug 24, 2023

Never mind, the scaling issue was fixed after refreshing my model cache.

@mxgrey mxgrey merged commit d5a3f12 into main Aug 24, 2023
5 checks passed
@mxgrey mxgrey deleted the luca/fuel_asset_gallery branch August 24, 2023 05:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants