Refactor optional model properties #263

xiyuoh · 2025-01-14T06:23:32Z

This PR targets #254 to refactor the current way of storing optional model description/instance data to file. The following changes aim to address the issues brought up in the ticket:

A unified struct Mobility to represent mobile robots and accommodates mobility types other than the provided DifferentialDrive. Mobility is now an optional ModelProperty for model descriptions, and users can opt to select DifferentialDrive or add their own plugins to introduce other kinds of mobility to the editor. Mobility data (kind and config), along with any other optional ModelProperty we may introduce for model descriptions, are stored to file under OptionalModelData, which is a serde_json::Value.
Task is no longer an enum that only supports default tasks (GoToPlace and WaitFor) in the site editor. It is similar to Mobility where users can create their own task kinds and add them to the editor using plugins. When creating a new Task, any registered Task type can be selected from the editor, and their own configuration widget will appear accordingly. Tasks are also stored to file under OptionalModelData as a serde_json::Value. To enable adding tasks to a model instance, users will need to toggle the Robot property button for its model description.
Updated the way we store ModelPropertyData - it initially used TypeId to identify each ModelProperty, but it is not compatible with serde_json. I've since changed it to ComponentId instead.

Mobility configuration:

Task configuration:

Some considerations as I was working on the refactor:

I still chose to store both Mobility and Tasks within the renamed OptionalModelData component (hopefully the naming is more suitable than OptionalModelProperties) instead of creating separate components for model descriptions/instances. Making the switching from a HashMap to serde_json::Value should help with increasing configurability for either case. Mobility is added for model descriptions and Tasks for model instances, like before.
I intentionally didn't add Mobility or Tasks to the Model Description/Instance bundles respectively. To make sure that we don't have an unnecessary component required for non-mobile and non-robot models.
I updated MobileRobotMarker to RobotMarker and retained it as a separate ModelProperty from Mobility. This is for future use cases where we may support non-mobile robots that are also capable of performing tasks. The RobotMarker property is used to allow task creation for model instances affiliated to the selected description.
I didn't add in Task schedule / integration with scenarios as this PR is getting a little bloated already, happy to refine this refactor and bring in task schedules after.

Signed-off-by: Xiyu Oh <xiyu@openrobotics.org>

Insert RobotMarker if instance has Tasks Signed-off-by: Xiyu Oh <xiyu@openrobotics.org>

mxgrey

This PR is moving things in a really good direction with the extensibility it gives. But we need to make sure to find the right balance between flexibility and explicitness.

I've left some inline comments to recommend changes that I think will help lean the balance back towards more explicitness.

mxgrey · 2025-01-15T12:53:08Z

rmf_site_format/src/task.rs

+#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)]
+#[cfg_attr(feature = "bevy", derive(Component, Reflect))]
+pub struct GoToPlace {
+    pub location: NameInSite,


This can just be a String. NameInSite only exists to be a component that wraps a String.

mxgrey · 2025-01-15T13:12:20Z

rmf_site_format/src/model.rs

 /// Defines a property in a model description, that will be added to all instances
 #[derive(Serialize, Deserialize, Debug, Clone, PartialEq)]
 #[cfg_attr(feature = "bevy", derive(Component))]
-pub struct OptionalModelProperties<T: RefTrait>(pub Vec<OptionalModelProperty<T>>);
+pub struct OptionalModelData(pub serde_json::Value);


What if instead of the RobotMarker component we replace this OptionalModelData struct with

#[derive(Serialize, Deserialize, Component, ...)] pub struct Robot { /// A dictionary of skills that this robot can perform. /// The value of each skill is its configuration. /// Mobility is stored here. pub skills: HashMap<String, serde_json::Value>, }

This would simplify OptionalModelData and RobotMarker into one struct and makes the intention of the component more clear.

A complication is that Bundles can't contain optional components, so instead of storing Robot inside of ModelDescriptionBundle, we should store it adjacent, e.g.:

pub struct Site { ... pub model_descriptions: HashMap<u32, ModelDescriptionBundle>, pub robots: HashMap<u32, Robot>, ... }

This way the Robot component can be optionally inserted into whichever model descriptions are included in the robots map. We just need to make sure to process model_descriptions first to spawn the model descriptions and then insert the robots components on top of those spawned entities.

mxgrey · 2025-01-15T14:23:17Z

rmf_site_format/src/model.rs

@@ -163,7 +119,8 @@ pub struct ModelInstance<T: RefTrait> {
    pub marker: ModelMarker,
    #[serde(skip)]
    pub instance_marker: InstanceMarker,
-    pub optional_properties: OptionalModelProperties<T>,
+    #[serde(default, skip_serializing_if = "is_default")]
+    pub optional_data: OptionalModelData,


I don't think shoving Tasks into this struct makes much sense for the long-term vision here.

It implies that users are expected to assign specific tasks to specific robots in a specific order, but Open-RMF should have a task dispatching system.

Tasks would be a good thing to be accounted for in scenarios.

Apologies for complicating the PR, but I would recommend the following:

We change Task to be

pub enum Task { Dispatch(DispatchTaskRequest), Direct(RobotTaskRequest), }

DispatchTaskRequest can be based on the dispatch_task_request schema and RobotTaskRequest can be based on the robot_task_request schema.

Each would internally contain a request: TaskRequest.

In the Site struct we would have

pub struct Site { ... pub tasks: BTreeMap<u32, Task>, }

and then in Scenarios:

pub struct Scenarios<T> { ... pub added_tasks: Vec<T>, pub removed_tasks: Vec<T>, }

Admittedly this will complicate things for the experimental mapf extension since we'll need to introduce a task dispatcher before we can generate the motion plans, but I think this will make the most sense for the site editor in the long-term. We can follow-up this PR with a very rudimentary prototype task dispatcher that just assigns tasks to suitable robot instances in a round-robin.

I just noticed in your PR description

I didn't add in Task schedule / integration with scenarios as this PR is getting a little bloated already, happy to refine this refactor and bring in task schedules after.

If you want to save this change for a follow-up PR that should be okay.

mxgrey · 2025-01-15T14:54:05Z

rmf_site_editor/src/interaction/model.rs

+            if let Some(go_to_place) = tasks
+                .0
+                .first()
+                .and_then(|t| serde_json::from_value::<GoToPlace>(t.config.clone()).ok())


I have some misgivings about the scalability here. This won't work for other task types that might also benefit from being able to highlight entities in the scene. It also requires looping through all locations, which isn't great.

I'd suggest we just remove this functionality for now and we can think more about how to give task-based visual cues some time down the road in a later PR.

mxgrey · 2025-01-15T15:09:09Z

rmf_site_format/src/mobility.rs

+    pub config: serde_json::Value,
+    pub bidirectional: bool,
+    pub collision_radius: f32,
+    pub rotation_center_offset: [f32; 2],


I think we should put all these fields (bidirectional, collision_radius, and rotation_center_offset) into struct DifferentialDrive since they might not make sense for other forms of mobility, e.g. omni wheel drive.

I'm not certain that collision_radius belongs inside of Mobility at all.. collision information probably deserves its own component. But let's keep it in DifferentialDrive for now until we can think it over more.

With reference to the comment above, if we were to simplify RobotMarker/OptionalModelData to Robot, collision_radius might go well as a field under Robot alongside skills.

Yeah I was thinking about what it should look like as its own thing parallel to Mobility.

Here's one way I can imagine it working:

Instead of Robot::skills we can call it Robot::properties

We introduce a Collision struct similar to Mobility that has a kind and config

We have a circle collision kind:

pub struct CircleCollision { pub radius: f32, pub offset: Vec2, }

Signed-off-by: Xiyu Oh <xiyu@openrobotics.org>

Signed-off-by: Michael X. Grey <greyxmike@gmail.com>

Signed-off-by: Michael X. Grey <grey@openrobotics.org>

over Signed-off-by: Xiyu Oh <xiyu@openrobotics.org>

Signed-off-by: Xiyu Oh <xiyu@openrobotics.org>

Fix Vec2 and Mobility not updating correctly Signed-off-by: Xiyu Oh <xiyu@openrobotics.org>

Signed-off-by: Xiyu Oh <xiyu@openrobotics.org>

…optional_model_properties

Signed-off-by: Xiyu Oh <xiyu@openrobotics.org>

xiyuoh · 2025-01-23T02:26:51Z

Thanks @mxgrey for the comments! After another round of iteration here are the main changes with the suggestions:

RobotMarker + OptionalModelData combined as Robot struct with properties. Robot is also its own site data instead of being a field under Model Descriptions
Collision separated out from Mobility as its own Robot property
Tasks no longer stored as OptionalModelData, and instead are stored as their own site data as well
Removed OptionalModelData entirely, brought in changes from Make the inner bundle of Parented transparent #264 for a flattened hierarchy of Parented bundles. serde(transparent) no longer necessary.

mxgrey

This implementation is looking a lot more clean, very nice!

I just left a few more small nitpicks that should be easy to get through, and then a few longer term remarks to think about for a future PR.

mxgrey · 2025-01-23T10:08:56Z

rmf_site_editor/src/interaction/model.rs

@@ -72,34 +73,18 @@ pub fn update_model_instance_visual_cues(
        if let Some(tasks) = tasks {


Technically this isn't hurting anything, but I don't believe any of this block is needed any longer.

mxgrey · 2025-01-23T10:09:12Z

rmf_site_editor/src/interaction/model.rs

-                    }
-                }
-            }
+            // TODO(@xiyuoh) support task-based visual cues
        }
    }

    // When instances are removed, prevent any location from supporting them
    for removed in removed_components.read() {


I don't think this is needed any longer either.

mxgrey · 2025-01-23T10:11:23Z

rmf_site_editor/src/site/load.rs

+                .insert(ModelProperty(robot_data.clone()));
+        } else {
+            // TODO(@xiyuoh) consider creating an entity if not found
+            error!("Robot is pointing to a non-existent model description!");


Let's include the robot_id and robot_data values in this message.

mxgrey · 2025-01-23T10:20:26Z

rmf_site_editor/src/widgets/inspector/inspect_model_description/inspect_circle_collision.rs

+        .get(&Collision::label())
+        .and_then(|c| serde_json::from_value::<Collision>(c.clone()).ok())
+        .filter(|c| c.kind() == CircleCollision::label())
+        .and_then(|c| serde_json::from_value::<CircleCollision>(c.config().clone()).ok())


Since egui is an immediate mode GUI framework, we're going to be parsing these values on every frame update. That's probably not an enormous burden given how lightweight these structs are, but I worry about the scalability in general.

We can move along with this PR as-is, but we should think about how to find the right balance with this extensible structure. For example, maybe CircleCollision should be a component which we add or remove from the entity as needed based on changes in the properties inside Robot. Then this widget can query for CircleCollision instead of doing this parsing.

That's a good point. I'll follow up on this.

mxgrey · 2025-01-23T10:21:50Z

rmf_site_editor/src/widgets/inspector/inspect_model_description/inspect_mobility.rs

+        let mobility = robot
+            .properties
+            .get(&mobility_label)
+            .and_then(|m| serde_json::from_value::<Mobility>(m.clone()).ok());


Similar concerns about the high frequency deserialization here

mxgrey · 2025-01-23T10:23:06Z

rmf_site_editor/src/widgets/inspector/inspect_model_description/inspect_robot_properties.rs

+
+/// This resource keeps track of all the properties that can be configured for a robot.
+#[derive(Resource)]
+pub struct RobotPropertyData(pub HashMap<String, HashMap<String, ShowRobotPropertyWidgetFn>>);


Nitpick: Maybe rename this to RobotPropertyWidgets. Data doesn't seem like the right descriptor to me.

mxgrey · 2025-01-23T10:26:14Z

rmf_site_editor/src/widgets/inspector/inspect_model_description/inspect_circle_collision.rs

+        app.world
+            .resource_mut::<RobotPropertyData>()
+            .0
+            .get_mut(&Collision::label())


You could use

.entry(Collision::label()) .or_default()

here so that this doesn't fail if the entry isn't already present.

mxgrey · 2025-01-23T10:27:18Z

rmf_site_editor/src/widgets/inspector/inspect_model_description/inspect_differential_drive.rs

+        app.world
+            .resource_mut::<RobotPropertyData>()
+            .0
+            .get_mut(&Mobility::label())


Could use .entry here as well.

Signed-off-by: Xiyu Oh <xiyu@openrobotics.org>

xiyuoh added 11 commits January 9, 2025 10:07

Remove MobileRobotMarker from OptionalModelProperties

4f8f1ba

Signed-off-by: Xiyu Oh <xiyu@openrobotics.org>

Use Mobility struct for saving

2923bf6

Signed-off-by: Xiyu Oh <xiyu@openrobotics.org>

Update Tasks for saving

5b35ec3

Signed-off-by: Xiyu Oh <xiyu@openrobotics.org>

Change MobileRobotMarker to RobotMarker

f65bbda

Signed-off-by: Xiyu Oh <xiyu@openrobotics.org>

Merge branch 'main' into xiyu/optional_model_properties

e33e360

Add Mobility to ModelDescriptionBundle

4de0f3a

Signed-off-by: Xiyu Oh <xiyu@openrobotics.org>

Use ComponentId for ModelProperty and update Mobility

b3111a2

Signed-off-by: Xiyu Oh <xiyu@openrobotics.org>

Refactor Tasks

a312a10

Signed-off-by: Xiyu Oh <xiyu@openrobotics.org>

Interactoins with locations for GoToPlace tasks

e332401

Signed-off-by: Xiyu Oh <xiyu@openrobotics.org>

Cleanup

88d0ed1

Signed-off-by: Xiyu Oh <xiyu@openrobotics.org>

Insert RobotMarker

19a8274

Insert RobotMarker if instance has Tasks Signed-off-by: Xiyu Oh <xiyu@openrobotics.org>

xiyuoh requested a review from mxgrey January 14, 2025 06:24

mxgrey requested changes Jan 15, 2025

View reviewed changes

xiyuoh and others added 6 commits January 17, 2025 11:13

Remove task-based interactions and GoToPlace NameInSite

3274b60

Signed-off-by: Xiyu Oh <xiyu@openrobotics.org>

Combine RobotMarker and OptionalModelData into Robot

edd54cc

Signed-off-by: Xiyu Oh <xiyu@openrobotics.org>

Add draft collision

dcdf8b3

Signed-off-by: Xiyu Oh <xiyu@openrobotics.org>

Make the inner bundle of Parented transparent

3fae5da

Signed-off-by: Michael X. Grey <greyxmike@gmail.com>

Use prerelease of ron

a8ff8d7

Signed-off-by: Michael X. Grey <grey@openrobotics.org>

Fix style

c69b6ce

Signed-off-by: Michael X. Grey <grey@openrobotics.org>

xiyuoh mentioned this pull request Jan 21, 2025

Make the inner bundle of Parented transparent #264

Closed

xiyuoh added 8 commits January 21, 2025 12:33

Set up RobotPropertyData to store property kinds and move CollisionKinds

6c09af4

over Signed-off-by: Xiyu Oh <xiyu@openrobotics.org>

Move MobilityKinds into RobotPropertyData

98f3dba

Signed-off-by: Xiyu Oh <xiyu@openrobotics.org>

Fix vec2

5652be2

Fix Vec2 and Mobility not updating correctly Signed-off-by: Xiyu Oh <xiyu@openrobotics.org>

Add InspectRobotPropertyPlugin for common use

538ebd8

Signed-off-by: Xiyu Oh <xiyu@openrobotics.org>

Enable visualize circle collision

7985a80

Signed-off-by: Xiyu Oh <xiyu@openrobotics.org>

Move Tasks into Site and remove OptionalModelData

d93a4db

Signed-off-by: Xiyu Oh <xiyu@openrobotics.org>

Merge remote-tracking branch 'origin/transparent_parented' into xiyu/…

a173474

…optional_model_properties

Cleanup

bc769fa

Signed-off-by: Xiyu Oh <xiyu@openrobotics.org>

xiyuoh requested a review from mxgrey January 23, 2025 02:26

mxgrey reviewed Jan 23, 2025

View reviewed changes

Address comments

3b1e85b

Signed-off-by: Xiyu Oh <xiyu@openrobotics.org>

xiyuoh requested a review from mxgrey January 28, 2025 04:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor optional model properties #263

Refactor optional model properties #263

xiyuoh commented Jan 14, 2025 •

edited

Loading

mxgrey left a comment

mxgrey Jan 15, 2025

mxgrey Jan 15, 2025

mxgrey Jan 15, 2025

mxgrey Jan 15, 2025

mxgrey Jan 15, 2025

mxgrey Jan 15, 2025

xiyuoh Jan 16, 2025 •

edited

Loading

mxgrey Jan 16, 2025

xiyuoh commented Jan 23, 2025

mxgrey left a comment

mxgrey Jan 23, 2025

mxgrey Jan 23, 2025

mxgrey Jan 23, 2025

mxgrey Jan 23, 2025

xiyuoh Jan 28, 2025

mxgrey Jan 23, 2025

mxgrey Jan 23, 2025

mxgrey Jan 23, 2025

mxgrey Jan 23, 2025

		@@ -72,34 +73,18 @@ pub fn update_model_instance_visual_cues(
		if let Some(tasks) = tasks {

Refactor optional model properties #263

Are you sure you want to change the base?

Refactor optional model properties #263

Conversation

xiyuoh commented Jan 14, 2025 • edited Loading

mxgrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiyuoh Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiyuoh commented Jan 23, 2025

mxgrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiyuoh commented Jan 14, 2025 •

edited

Loading

xiyuoh Jan 16, 2025 •

edited

Loading