Refactor RobotProperty implementation

[xiyuoh]

The current RobotProperty/RobotPropertyKind/ModelPropertyData implementation exist largely within the widget module in the site editor. This should not be the case, especially if we want to operate in headless mode. With this PR we move non-UI related logic into their own submodules within the site mod.

Also added a fix for the headless roundtrip test failing CI (example #345), which occurs because slotcar properties are overwriting existing RobotProperty/RobotPropertyKind imported from file. This is caused by using commands.entity() to insert these components during load, but using world.entity_mut() when inserting SDF/slotcar parameters, so the sequence was messed up. Added some checks in insert_slotcar_components to make sure that we're not overwriting existing RobotProperty imported from loading.

[luca-della-vedova]

A few minor comments on cleanups but there is one on functionality about using commands in the SDF Loader that is a bit more substantial

[luca-della-vedova]

I might be wrong but I can't figure out if this would ever fail. The query only looks for optional components so I suspect it would actually always succeed, if any component is missing it will be set to None itself.
The only case this could possibly fail is if the entity doesn't exist but that's probably not going to happen since it comes from another query.
Is this intentional? What are you trying to test here?

[xiyuoh]

Ah I'm trying to figure out if any combination of RobotProperty was already present in the description, and only overwrite components that are not present. So it was intentional to query for optional RobotProperty components for all model descriptions.

But on second thought if a particular component is present in a saved JSON and another is not, the latter is likely meant to be disabled. I'll remove the Option and check for present RobotProperty components as a group.

[xiyuoh]

19e9e93, turned it into a QueryFilter instead since we don't need the actual values

[xiyuoh]

Note, IsStatic has to be optional else slotcar parameters may not be inserted at all, even if there's no existing params

[luca-della-vedova]

This looks a bit odd. What the SDF loader does is serialize the whole content of the world into a bevy::Scene then later we will spawn the Scene.
I think the scene is later spawned through write_to_world_with and that only spawns entities and components, not commands.
So I wonder, are you sure the command here is being applied?

[xiyuoh]

So this is the main change that fixes the "saved robot properties are being overwritten by the SDF loader" bug for me. If I undo this change cargo test would fail, and after some logging it turned out the original and destination JSONs are different, with RobotProperty being overwritten. This is what I think is happening without commands():

1. load.rs inserts the Robot component into relevant model description entities. This would prepare the linked systems to insert appropriate RobotProperty (here) and RobotPropertyKind (here) components.
2. Later in the model loading workflow, the SDF loader iterates through all SDF parts (links/visual/etc.) and inserts the slotcar components into those descendant entities instantly with direct world access. This is because within the SDF loader we don't know what the affiliated descriptions are, since they're not in the same World as where everything else is.
3. The insert_slotcar_components system searches for non-description entities with these RobotPropertyKind components, and insert them into the appropriate affiliated description entities. This is done by writing a Change event with the updated Robot serialized properties.
4. The ChangePlugin updates ModelProperty<Robot> in the next cycle, and eventually propagates to the affiliated model instances via the update_model_instances system.

TLDR I think steps (1) to (3) take place in cycle N, and step (4) takes place in cycle N+1. This means the inserted components in (1) are not known to insert_slotcar_components in step (3) and will be overwritten. By using commands() we're essentially forcing step 3. to only find slotcar models with robot properties in cycle N+1, and be able to query components from step 1. to check if properties are already present.

[luca-della-vedova]

My assertion here is that the world you see here is not the App's world, but a brand new one that is immediately serialized into a scene, and in this process only entities and components are saved, the commands are effectively dropped.
I tested this by actually removing the whole statement that uses a command to add components altogether and verified that the unit test still passes. This makes me think that our unit test doesn't actually verify that this statement is working

[xiyuoh]

Thanks for pointing that out, I just tried opening the demo map and the tinyRobots do not have the properties inserted in. Let me think of a more reliable fix!

[xiyuoh]

Some changes from the last review (significant enough for a PR title change):

• Use observers instead of scheduled systems to update changes to ModelProperty<Robot> and insert/remove relevant components into/from description entities. This should help make sure changes are propagated and present by the next cycle.
• Cleaned up slotcar implementation to fix overwriting issue
• Tackled TODO for supporting multi-kind robot properties (e.g. PowerDissipation that supports multiple robot property kinds, i.e. AmbientSystem and MechanicalSystem) instead of having to write a separate plugin for these use cases
• Introduced EmptyRobotProperty<T> - can be used to represent RobotProperty intentionally left empty in the site. Used EmptyRobotPropertyPlugin::<T>::new() to enable this for a chosen robot property.
• Moved RobotProperty and RobotPropertyKind registration logic out of widget module and into site. This ensures that we're still registering and tracking these properties in headless mode.

[xiyuoh]

I believe with these changes it should also close #382

[luca-della-vedova]

Since properties are optional I played a bit with enabling / disabling them but that doesn't really seem to work. Specifically:

If I open the demo site, disable a robot property (i.e. Collision) and save the site, when reloading it the property will be enabled again.

More puzzlingly, if I open the demo site, change a property value (i.e. power dissipation), then disable the property, save and reload I get this weird state that the property can't actually be enabled again:

Screencast.From.2025-10-08.20-24-16.mp4

[xiyuoh]

If I open the demo site, disable a robot property (i.e. Collision) and save the site, when reloading it the property will be enabled again.

Collision components will be automatically inserted into all robot model descriptions following #373, unless the entity already has a Collision RobotProperty. de31bf4 introduced EmptyRobotProperty<T> for Collision to deal with cases where the robot is intended to have no Collision properties.

More puzzlingly, if I open the demo site, change a property value (i.e. power dissipation), then disable the property, save and reload I get this weird state that the property can't actually be enabled again:

Thanks for finding this bug! Fixed in 15bff39. It happens for multi-kind RobotProperty as their serde_json::Value is set to Value::Null when newly initialized, but when inserting RobotPropertyKind (e.g. Ambient or Mechanical System) they are looking for Value::Object. Fixed by checking for Value::Null for such cases, and inserting an empty Value::Object(Map).

[luca-della-vedova]

If I open the demo site, disable a robot property (i.e. Collision) and save the site, when reloading it the property will be enabled again.

Collision components will be automatically inserted into all robot model descriptions following #373, unless the entity already has a Collision RobotProperty. de31bf4 introduced EmptyRobotProperty<T> for Collision to deal with cases where the robot is intended to have no Collision properties.

I only used Collision as an example, this happens for all properties.

[xiyuoh]

Ah thanks for providing more context, can I double check that when you disabled the properties, you disabled all of them?

If the robot properties map loaded from file is completely empty, it will behave as if we're loading the demo map, i.e. the editor will insert all the slotcar related properties. After testing a little bit, any other combination aside from a completely empty properties map should yield the correct data. I think we should keep this behavior (insert slotcar components if they're found + there are no pre-existing robot properties), but one way to ensure a non-empty properties map in this scenario is to set Collision to Empty Collision.

In any case, I've added the EmptyRobotPropertyPlugin for Mobility and Power Source as well in 9a1325d, so there are other ways to keep the properties map non-empty without changes to the robot properties themselves. Power Dissipation does not need once as we can just enable it without enabling it's property kinds.

[luca-della-vedova]

Understood on the behavior, I left some inline reviews, behavior wise lgtm

[luca-della-vedova]

Thanks for iterating! LGTM