Use workflows for model spawning

[luca-della-vedova]

New feature implementation

Implemented feature

This PR brings model loading from a fairly complex state machine involving several marker components and implicit system ordering to workflow. This unlocks the following:

• Proper dependency tracking: Arbitrary hierarchies of nested models will now be loaded as part of the root model, if any of the dependencies fails the user will know which dependency fails and the whole model spawning will be aborted.
• Flexibility with callbacks: We can use a callback based system to perform actions on a model as soon as it is spawned, removing the need to use marker components and systems querying them for this purpose.
• "Quicker" model loading. Since we can do model loading in a series of blocking workflows they all can be executed in a sequence potentially in a single frame without having to wait multiple systems to go through command flushes and change detection (that a marker component based approach would need).
• Safer component adding / removing to models that are being spawned, which removes the need for the ModelTrashCan that we introduced before to avoid panics, we can now just do a normal despawn_recursive()

Implementation description

High level

A new workflow for model loading with a command implementation has been added, its inputs are the entity that the model should be spawned in, as well as its asset source.

// Before
let model = Model {
    [...]
};
commands.spawn(model);

// After, we still need to add all the required components, the service only reads an `AssetSource` and spawns a model accordingly
let source = AssetSource[...]
let model = Model {
    source: source.clone(),
    [...]
};
let id = commands.spawn(model).id();
commands.spawn_model((id, source).into());

Workflow details

spawn_model takes a ModelLoadingRequest as an input, which can be default initialized from a (Entity, AssetSource), but also uses a builder pattern so users can provide a Callback<Entity, ()> that will be called on the model if it spawned correctly. This is currently used for workcell editor mode, where we want to do a custom action on the model (for example post-process its hierarchy).

rmf_site/rmf_site_editor/src/interaction/select/place_object_3d.rs

Lines 477 to 478 in 9bde074

	let req = ModelLoadingRequest::new(model_id, source).then(flatten_models);
	commands.spawn_model(req);

The workflow itself propagates its request throughout and returns a ModelLoadingResult:

rmf_site/rmf_site_editor/src/site/model.rs

Line 63 in 9bde074

pub type ModelLoadingResult = Result<ModelLoadingRequest, Option<ModelLoadingError>>;

The result will contain :

• Ok(the original request) if successful.
• Err(None) if the workflow was aborted but without an error. Currently this happens only if the model was not spawned because it already contains a scene with the requested AssetSource, this could happen if a user called spawn_model several times on the same entity with the same source and avoids unnecessary work.
• Err(Some(err))` if it fails for any other reason, asset not found, failed parsing. When this is triggered we add a marker component for model failure and cleanup the model scene.

Marker components for state

Marker components are not fully gone, when a model spawning is requested a component with its Promise will be added to the entity.

rmf_site/rmf_site_editor/src/site/model.rs

Lines 318 to 320 in 9bde074

	/// Component added to models that are being loaded
	#[derive(Component, Deref, DerefMut)]
	pub struct ModelLoadingState(Promise<ModelLoadingResult>);

This component can be used in queries to know whether any models are still pending spawning, it is currently used in the SDF exporter to trigger saving when all models either finished or failed loading. When the following query is empty we know that no models are currently being loaded. This could potentially be removed if we made site loading a workflow which itself will only complete when all its models finished loading, however I left this out of this PR to avoid blowing up the size of this refactor.

rmf_site/rmf_site_editor/src/site/sdf_exporter.rs

Line 49 in 9bde074

missing_models: Query<(), With<ModelLoadingState>>,

Furthermore, a marker component could be added (I just removed it, check #238 (comment)) when the workflow fails to load a model:

rmf_site/rmf_site_editor/src/site/model.rs

Lines 322 to 324 in 9bde074

	/// Marker component added to models that failed loading
	#[derive(Component)]
	pub struct ModelFailedLoading;

This component is currently only used to detect which models failed loading and should be retried when a new Gazebo app API key is set:

rmf_site/rmf_site_editor/src/site/fuel_cache.rs

Lines 127 to 131 in 9bde074

	pub fn reload_failed_models_with_new_api_key(
	mut commands: Commands,
	mut api_key_events: EventReader<SetFuelApiKey>,
	failed_models: Query<(Entity, &AssetSource), With<ModelFailedLoading>>,
	) {

But it could also be used, for example, to generate an export log when a site is exported to SDF (for example, reporting to the user that a certain set of models couldn't be loaded and hence might be missing from the world).

[luca-della-vedova]

ad502f0 contains an example of removing the marker component, it will mean that users have to rely on a less obvious component to keep track of which model failed loading but saves some code, I'm happy with both options.

[mxgrey]

This is just a preliminary review, but so far this migration is looking very good. I think we're going to massively benefit from workflows for model loading, so I'm looking forward to getting this merged.

I've left some comments that are mostly about tweaking the API and the inputs/outputs of the model loading workflow. Do let me know if you have any questions/concerns about the recommendations.

[mxgrey]

When the loading gets skipped because the entity has already loaded the same asset source, I'm not sure that returning an Err(None) conveys the best message.

I might suggest something like

pub struct ModelLoadingSuccess {
    pub request: ModelLoadingRequest,
    /// If true, nothing needed to happen because the requested model was already loaded
    pub unchanged: bool,
}

pub type ModelLoadingResult = Result<ModelLoadingSuccess, ModelLoadingError>;

[luca-della-vedova]

Using Err(None) for signaling "noop" allowed a very convenient one line early termination in the workflow, I am a bit rusty on workflows, how would that translate to a boolean? I suppose I would have to do a map_block then connect to scope.terminate if the boolean is true?

[luca-della-vedova]

60f9695

[mxgrey]

Thinking about terminology and ergonomics, I might suggest some tweaks to this API.

I think the functionality of this method would be better described as update_asset_source rather than spawn_model. I'd suggest the following API to streamline the API a little bit:

/// This is implemented for Commands
pub trait ModelSpawningExt<'w ,'s> {
    /// Spawn a new model and begin a workflow to load its asset source.
    /// This is only for brand new models does not support reacting to the load finishing.
    fn spawn_model(&mut self, parent: Entity, model: Model) -> EntityCommands<'_> {
        self.spawn_model_impulse(parent, model, |impulse| impulse.detach());
    }

    /// Spawn a new model and begin a workflow to load its asset source.
    /// Additionally build on the impulse chain of the asset source loading workflow.
    fn spawn_model_impulse(
        &mut self,
        parent: Entity,
        model: Model,
        impulse: impl FnOnce(Impulse<'_, '_, '_, ModelLoadingResult, ()>),
    );

    /// Run a basic workflow to update the asset source of an existing entity
    fn update_asset_source(
        &mut self, 
        entity: Entity,
        source: AssetSource,
    ) {
        self
        .update_asset_source_impulse(entity, source)
        .detach();
    }

    /// Update an asset source and then keep attaching impulses to its outcome.
    /// Remember to call `.detach()` when finished or else the whole chain will be
    /// dropped right away.
    fn update_asset_source_impulse(
        &mut self, 
        entity: Entity,
        source: AssetSource,
    ) -> Impulse<'w, 's, '_, ModelLoadingResult, ()>;
}

Then the very common multi-step spawning like

let e = commands.spawn((model, Pending)).set_parent(self.frame).id();
commands.spawn_model((e, source).into());

can be simplified to

let e = commands.spawn_model(self.frame, model).insert(Pending).id();

The spawn_model method would automatically use update_asset_source internally.

In cases where something special needs to happen after the asset source is done loading, users can instead do

let e = commands.spawn_model_impulse(
    self.frame,
    model,
    |impulse| {
        impulse
        .then( _ ) // Any kind of provider
        .then( _ ) // Any kind of provider
        .detach();
    },
).id();

This means the user won't be limited to only Callbacks and can also react to error results. If the user wants to update the asset source of an existing model instead of spawning a new one, then they would use update_asset_source or update_asset_source_impulse instead.

[luca-della-vedova]

5feaf8b

[mxgrey]

This is looking really great. I have a few more small pieces of feedback.

While testing out this PR, I realized that the preempting feature isn't working correctly for workflows. It doesn't seem to recognize when an earlier run of a label has finished. That's something that I'll need to test and fix upstream, so I'll dig into that and open a bevy_impulse PR for that.

[mxgrey]

Is there any risk that the scene at index 0 will be missing? I'd strongly prefer to not unwrap if there's even a very marginal possibility of panicking.

Another thing to note is that this current setup will always clone the scene handle at index 0, even if the default_scene was present. In this case it's pretty minor; it only costs incrementing the reference count of an Arc. But I think we should practice good chaining hygiene that captures our real intention.

I'd suggest changing this to:

        let scene = gltf
            .default_scene
            .as_ref()
            .cloned()
            .or_else(|| gltf.scenes.get(0).cloned())?;

That way we only look for gltf.scenes.get(0) if the default_scene isn't available, and we'll just quit the service early with None if neither is available.

[luca-della-vedova]

Done with a small change in 182fa8b (we can just do a single cloned() call)

[mxgrey]

I'm just leaving one last very minor suggestion.

Otherwise the only blocker is to merge open-rmf/bevy_impulse#40

We should also consider setting the bevy_impulse dependency to be 0.0.2 after the above PR is merged, although targeting main will continue to work for now. Actually that would need to wait until I've published a new release, which is something I should discuss with the PMC before doing.