Introducing model descriptions and instances

[xiyuoh]

This is a continuation of Reuben's PR #230, opening a new one to accommodate push restrictions.

This PR:

• Introduces the concept of model descriptions and instances, where a model description provides the information about a model (asset source, scale, etc.), and a model being spawned on site is an instance of a particular model description. This allows users to make changes to a group of model instances with the same model description at one go. This is helpful for sites with multiple identical models.
• Allows users to create different scenarios with a scenario tree, where they can edit and switch between variants of the same environment with the ability to undo or redo changes without affecting the root scenario.
• Provides a configurable property plugin that enables users to add new properties and configuration widgets, such as mobile robot tasks for instances and differential drive properties for descriptions.

[xiyuoh]

Changes from Reuben's PR:

Major changes

• Model instance deletion: The logic for permanently deleting/temporarily removing a Model Instance in the original PR goes through the Delete Event twice - once to check whether the Model Instance should be temporarily removed only or permanently deleted (depends on the current scenario where this Model Instance is displayed), then a second time for actual deletion. The effort to streamline this process is in Filter entities sent for deletion #243, and is implemented here with filter_instance_deletion (057e186).
• Refactor Tasks implementation - it started by wanting to implement the Display trait for Tasks so that we don't have to rely on a vector of Strings, but it led to quite a huge change in the way the Tasks enum is being written (acb39b9, 94b54a9)

Minor changes

• Merge conflicts, a large portion of them in creation.rs (42f25c2)
• Model instance creation - modified further after clearing merge conflicts. Needs to be reviewed again after workcell editor PR is merged (bb13ee0, e2b365c, d450cd7)
• If a description is no longer selected or hovered over, remove the description entity (if any) from the relevant model instance support_selected/support_hovered instead of clearing all entities that the model instance supports (afddc82)
• Simplify Model Instance creation - there are already a few ways to create a new Model Instance, limiting users to either spawn them from existing Model Descriptions under Groups or spawning one when loading a new Model Description (8ffcfe6)
• Fixed bug where scenarios from a previously loaded site still remains visible in the Scenarios widget (5786bae)
• Align delete button for scenarios so that it's visible within the widget (ad5ba6e)
• Send a ChangeCurrentScenario event when adding new levels, despite the scenario not changing, to trigger visibility updates for all model instances. Otherwise when creating a new level, the model instances from before still remains visible in the editor unless the user toggles between scenarios (6ad0ea7)
• Minor cleanup (4729bef, 1d8aa7d, af5a2d5, c6eebf4, f0fa12c, d331a79, 7b01c91, 83f3d85, de9b60e, 90a60ba, 8c19eed)

Notes:

• Known bug: Right now with the demo map, when expanding the Added dropdown in the Scenarios widget, there will be constant error messages being printed as the editor is unable to locate assets for the TeleportIngestor and TeleportDispenser. The editor will also crash if we try to delete the Default scenario, since these entities are invalid. This is only specific to the demo map.

[xiyuoh]

I've found that when calculating grid occupancy, the physical entities used for calculation are mostly meshes instead of model instance entities. This meant if a user wishes to ignore a set of model instances, their meshes would still cause the space to be occupied. 5aed94f and 3a6ad1d tackle that by ensuring that meshes that are children of an ignored entity would also be ignored.

[xiyuoh]

Latest changes:

• Enabled saving to file for model properties by introducing the OptionalModelProperties component. It currently supports all available optional properties: DifferentialDrive, MobileRobotMarker for model descriptions, and Tasks created for model instances. (4252c38, bbc373c)
• Resolve merge conflicts from recent workflows PR (766fd62)
• Other minor cleanup (cc29375, 01fca32, d97232d)

TODO fix CI, though I foresee some refactoring through the review process

[luca-della-vedova]

I think this is making CI fail, the moment we compile rmf_site_format without the bevy feature Entity will not be defined, you can try this locally by just running cargo build in the rmf_site_format folder.
I don't know the details of the Tasks implementation but usually if we need to reference an entity in a "non bevy specific way" in rmf_site_format we just use RefTrait:

rmf_site/rmf_site_format/src/site.rs

Line 224 in 7d21b48

pub trait RefTrait: Ord + Eq + Copy + Send + Sync + Hash + 'static {}

Which is a trait for references and is implemented for Entity (for usage in site editor) and u32 (for usage in serialization / deserialization, as well as not having a bevy dependency).

Now the problem with this approach is that it will make OptionalModelProperty generic and that will propagate all the way up to the ModelDescriptionBundle, which might not be what you want

[luca-della-vedova]

But again, maybe having to do ModelDescriptionBundle<u32> instead of ModelDescriptionBundle wouldn't be too bad

[xiyuoh]

Ah thank you for pointing this out! I couldn't figure out the underlying issue behind using Entity. I can see another (and maybe better) way to store location information in tasks without using Entity, especially since the current method to save tasks involve exporting the entity ID that could change between runs. Let me make that change.

Edit: the change will still involve adding RefTrait to all the relevant structs

[luca-della-vedova]

Yap that's the classic use for RefTrait, to use both Entity and u32, usually the u32 that is saved / loaded is the value that is contained in the SiteID component

[xiyuoh]

Fixed in 347d546

[luca-della-vedova]

Also the elephant in the room while I was giving this a spin, where are the robots?

My expectation

What I would have expected from this PR is, when a world is loaded, models are converted into instances (and I see that) and robots are also correctly imported and converted into instances and that doesn't seem to be the case.
Specifically, there is currently a LocationTag for robot spawning which, similar to what used to be in the legacy traffic editor, contains information on what robot is spawned in the location.

My expectation (not sure that is the intention of this PR) would have been to see that either:

• The tag is removed, since it's not relevant anymore.
• The robot is imported and spawned as an instance in the position of the anchor as a child of the level.

OR

• The tag is kept and used, maybe changed to point to a group rather than a Model.
• The model is spawned as an instance with some sort of affiliation to the location, so that when the location is moved / deleted the robot is also affected.

The first implementation followed the second approach, but only for historical reasons and I don't have strong feelings on what is right (it seems the current implementation would do the first instead).
One thing I personally like about connecting the robot to the location is that if the location was moved around the robot would also be moved around, which can be quite convenient when the location is a charger (so we can always start the simulation with a robot in its charger) but again I'm fairly neutral.

What I actually see

What I see happening instead is that the location tag still exists and contains information about what robot is spawned in that location. The information is however ignored and the demo map by default contains no robot. Robots that are imported will use purely the model instance components creating a bit of a double source of truth conflict with the location tag

[xiyuoh]

@luca-della-vedova Thanks for highlighting the issues! I've taken your suggestions and removed the redundant SpawnRobot enum from the code, and enabled loading robot from legacy files in 73076af. The robots should now appear when we open up the demo map by default.

Regarding setting up the option to tag robots to location entities (such that when we move location anchors around, the robot models would follow) - I'm still experimenting to figure out what's the best way to preserve non-location pose updates while also supporting robot-to-location affiliation, though I understand from our chat this is not too high up on the priority list and can potentially be tackled in another (and smaller) follow-up PR.

[mxgrey]

In the current setup, we iterate over &mut model_instances which does not have this filter, meaning it will include all model instances in the world every update cycle. Moreover, this query will only include model descriptions whose Hovered or Selected status has changed during the latest cycle, so this line will not fetch anything if neither of these components changed for the model description.

I have a feeling this filter was meant to go on model_instances, not on model_descriptions.