-
Notifications
You must be signed in to change notification settings - Fork 13
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
Conversation
Signed-off-by: Luca Della Vedova <[email protected]>
assets Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
There was a problem hiding this 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/src/site/sdf.rs
Outdated
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(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Never mind, the scaling issue was fixed after refreshing my model cache. |
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:
RenderLayer
to make sure they are not visible to other views.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 isasync
and goes to a background task in the IO thread pool to avoid having too much overhead.dae
files that are very prevalent in Fuel) or because it was not found in the path / fuel.AssetSource::Search
sdfs would work, this won't be the case anymore and bothAssetSource::Local
andAssetSource::Remote
SDFs should work. For example browsing through theLocal
asset source to amodel.sdf
file should successfully spawn the asset to the site.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.