Use components for RobotPropertyKind

[xiyuoh]

This PR is a follow up to #263, specifically this review comment about avoiding excessive serialization of Robot property data. The previous approach opted to store all Robot properties and property kinds in json to avoid having to query multiple different components on load/save. This PR expanded on the plugin approach by triggering updates to components via the property plugin's systems and only serialize data when a value has changed.

Another change is the use of a reusable plugin template InspectRobotPropertyKindPlugin with widget systems for all Robot property kinds rather than having the widgets stored in a resource. This allows each of the property kind plugins to have better access to world variables, instead of having to pass variables through ShowRobotPropertyWidgetFn in the previous implementation.

[mxgrey]

This is a very good implementation of what I was suggesting.

I've left some inline comments. A few of them should be addressed before merging, but some are just potential opportunities to reduce boilerplate which could be considered for a future PR.

[mxgrey]

I can't think of why v.as_object().is_some() should be a necessary condition here. I don't think we need to restrict what kind of values the description of a property can have. Testing for !k.is_empty() should be enough as far as I can figure.

[xiyuoh]

Gotcha! I agree we shouldn't decide for users the Value type.

I've updated it to !v.is_null() in 00634b4 instead of removing the condition as there is a possibility of users enabling a RobotProperty (e.g. Mobility) for robots, but have not selected a specific RobotPropertyKind (e.g. DifferentialDrive) and we don't want this half-configured property to be saved to file. The pending Value::Null value is set here.

[mxgrey]

I think I remember this was originally named RobotPropertyData and then I suggested we rename it to RobotPropertyWidgets. Did the name revert due to a merge conflict?

[xiyuoh]

Oh I reverted it back to RobotPropertyData in this PR as the resource no longer stores widget-related callbacks. Instead it stores insert/remove component functions for each RobotPropertyKind. The RobotPropertyKind widget is displayed only if the relevant component is present.

The previous approach of storing the ShowRobotPropertyWidgetFn was limiting for this case as the property kinds widgets did not have access to world data such as Queries and Gizmos.

[mxgrey]

I see, I still think _Data is misleading since I think people will assume that this resource stores the actual data to describe the robot properties.

I would argue that it does still contain widgets because the Entity inside of it provides the ID of a widget.

I would recommend naming it something like RobotPropertyWidgetRegistry.

I'd also recommend changing the tuples into concrete structs, maybe like

struct RobotPropertyWidgetRegistration {
    property_widget: Entity,
    kinds: HashMap<String, RobotPropertyKindRegistration>,
}

struct RobotPropertyKindRegistration {
    insert: InsertPropertyKindFn,
    remove: RemovePropertyKindFn,
}

This could make future refactors less disruptive.

[mxgrey]

This feels off to me. Is this supposed to be circle_collision.clone() here? I think it happens to work out currently because this code will typically only get triggered the first time a circle collision property gets added to a robot, but I don't see a reason to encode that assumption.

[xiyuoh]

I apologise for the confusing implementation. This is called when a user has freshly enabled the Collision RobotProperty and selected the CircleCollision RobotPropertyKind. In this moment, both Collision and CircleCollision components would be inserted to the entity, but the Collision RobotProperty config Value still looks like this:

{
    kind: CircleCollision
    config: {}
}

The highlighted codeblock is only triggered if config is empty like above, and inserts default CircleCollision values into Robot properties. For any future changes to existing CircleCollision config, serialize_and_change_robot_property would be called here instead.

[mxgrey]

Right, I understood all of that context. But it stands out to me that

let Ok((ModelProperty(robot), circle_collision)) =
    params.model_descriptions.get_mut(description_entity)
else {
    return;
};

implies that circle_collision already has a specific value for this entity. If it didn't then the function would have returned early. From what I can tell, the current code makes an assumption that if the circle_collision component has a value while robot.properties["collision"]["config"] does not have an appropriate kind of value, then we are initializing the circle collision properties to their default values.

But what if, from some other part of the code, I did this to add a circle collision instead of using the UI:

world.entity_mut(model_description).insert(CircleCollision {
    radius: 100.0,
    offset: [10.0, 10.0],
});

If that were to happen when CircleCollision didn't previously exist, then we'd initialize the serialized robot property to a default value that doesn't match the actual value of the component. If instead we did

serialize_and_change_robot_property::<Collision, CircleCollision>(
    params.change_robot_property,
    circle_collision.clone(),
    robot,
    description_entity,
);

then we'd be putting in a matching value.

I think in general it's a bit questionable for us to be synchronizing the ModelProperty<Robot> component with the CircleCollision component inside of a widget system. That should probably be done in its own system that runs in the post-update schedule. But it would probably be best to implement this with the relationships feature, so we can keep this approach until we've upgraded to the latest bevy.

[mxgrey]

This looks extremely similar to InspectCollision::show. It's fine to keep them as their own implementations for now, but if/when we introduce another property type we might want to see if we can refactor them all into one generic implementation.

[xiyuoh]

Thanks for the insight! My goal was to put as much common code into show_robot_property_widget() for users to reuse, at the same time leaving space for them to add any custom logic within show(). Will look into this again.

[mxgrey]

I'm not sure that we even need to check for !v.is_null(). I think it's plausible that some properties don't need any config structure at all, so the config field would just be null.

Did you run into a specific situation that needed this check?

[xiyuoh]

Oh! You might have missed my comment here, it is used to filter out RobotProperties that have not been configured (i.e. no property kind selected).

[mxgrey]

What do you think about cases where a property can simply exist or not exist without needing a configuration? At the moment we have a { "kind": _, "config": _ } pattern for all properties, but I wouldn't assume that all properties need to work this way, especially user-defined properties.

In the event that a user has a custom property type that doesn't need any description, I expect they'd use null for its value. The current implementation would prevent that from being saved.

Even in the case of Collision or Mobility not having a kind selected, I think it's okay to save that they've been selected without a kind. We could have a diagnostic to point out when no kind is selected, and also put warnings in the log when exporting a fleet configuration where a relevant property isn't configured correctly.

[xiyuoh]

Thanks for raising some very valid concerns! I've iterated on them and I believe I found a better approach for updating config values instead of making assumptions that are prone to bugs.

• Updated RobotPropertyWidgetRegistry as suggested (2c77c7f), but I took out insert and remove as I found that they're not being used anymore since we're inserting/removing components with systems instead. (5375d09)
• In response to this comment, with eeba081, we now populate config with default values together with the kind field when selecting RobotPropertyKind. This removes the need to overwrite with Kind::default() separately from plugin widgets and possibly making unwanted re-initialization of RobotPropertyKind values. Also removed a bunch of methods enforced by the RobotProperty trait as they are not/no longer required by the plugins. Also, happy to look into updating the implementation with relationships when the migration is done!
• Minor updates to widgets/save to avoid making RobotPropertyKinds compulsory, and added a diagnostic to check for missing property kinds for cases where they do require one. (46311eb and e7f88c8)

[mxgrey]

Small tip: When you have a newtype like this in bevy, if you add #[derive(Deref, DerefMut)] then you don't need to do .0 to access the inner data. I.e. instead of

robot_property_widgets.0.get(property)

you can just do

robot_property_widgets.get(property)

[mxgrey]

I think this implementation is looking really good.

One last tweak to consider would be using the RecallPlugin for the property values. With the recall plugin you could save the last value that was used for each property (including the property kind) before it was toggled off. That way when a user switches the kind or toggles a property off and then on again, we automatically fill back in the last value they were using for it so they don't have to fill it back in. I find this useful when scrolling through different options or playing around with toggling things on/off.

You can see an example of the Recall trait being implemented for AssetSource here.

All that being said, I don't think this is urgently needed, and this PR is already quite hefty, so I don't mind if we just make this a followup item.