Items to follw up on for Scenarios

[mxgrey]

This issue tracks follow-up action items for #242

• SiteParent should be replaced by querying for Option<&Parent>
• OptionalModelProperties should be refactored
  • It does not make sense to allow multiple instances of DifferentialDrive or MobileRobotMarker
  • MobileRobotMarker should not need to be explicit since DifferentialDrive should always imply MobileRobotMarker
    • Consider having a single property of Mobility { kind: String, config: serde_json::Value }
    • We can keep the DifferentialDrive struct and just parse to/from it as needed
    • Have a Resource containing a map from String (kind) to a Widget that can read and modify the serde_json::Value.
  • Task should not be a model property
    • Instead of being an enum, it should be similar to Mobility mentioned above, for example Task { kind: String, config: serde_json::Value }
    • Each task should be associated with an instance, not with a model description
      • We should have a schedule of tasks that include which instance performs it and at what time
      • This should be tied into scenarios so that each child scenario can introduce changes to the task schedule
      • We should have a widget for creating new tasks that uses a Resource to let downstream developers inject new task types with their own creation/editing widget
• Remove the agent.rs module since it is superseded by model instances
• Add an optional field for a default scenario that determines which scenario will be chosen in situations where none was specified
• Consider introducing a concept of include/hide instead of add/remove for scenarios.
  • All model instances that were ever created by the user exist in a pool that is accessible to all scenarios
  • Have a drop-down tree widget that shows every model description and expands to show all the instances of that description. Unaffiliated instances can have their own drop-down.
    • To fully delete an instance, a user must click a trash can icon next to the instance in this list
    • For each instance we can indicate how many scenarios it shows up in
    • We can add a diagnostic that warns users when an instance is not being used in any scenario
  • Each scenario can choose to include or hide each created model instance, but can only choose one or the other for each instance (i.e. the same scenario cannot both include and hide the same model instance)
  • This means a child scenario can choose to include a model instance that was hidden by one of its ancestor scenarios, and this does not involve creating a new model instance
  • We can probably simplify added_instances and move_instances to a single included_instances that indicates the location
• Add a diagnostic for when a moved instance in a child scenario is in very nearly the same position as it is in its parent scenario since this is probably an accidental shift
• Automatic import/export of sdformat plugins as initially described here
  • When loading a model, automatically convert its slotcar parameters to a Mobility component with a DifferentialDrive configuration
  • Provide an extensible way to export instances with additional plugins
    • Add a pub struct ExportWith(pub HashMap<String, serde_json::Value>) component for model description entities
    • Add a pub struct ExportHandlers(pub HashMap<String, ExportHandler>) resource that contains a map of export handlers
    • When exporting an instance, apply the export handlers to it
      • Each ExportHandler is a BoxedSystem<In = (Entity, serde_json::Value), Out = sdformat_rs::XmlElement>
      • For each entry in ExportWith, get the associated ExportHandler from ExportHandlers and pass in the (Entity, serde_json::Value) where the Entity is for the instance and the serde_json::Value is from the ExportWith.
      • The output from the ExportHandler gets inserted as an element into the <model> for the instance
    • Users can provide widgets to enable/disable and configure their export handlers
  • We will provide an ExportHandler for the slotcar plugin out of the box

[luca-della-vedova]

• Automatic import/export of sdformat plugins as described here

  • When loading a model, automatically convert its slotcar parameters to a Mobility component with a DifferentialDrive configuration

  • Provide an extensible way to export instances with additional plugins

    • Add a pub struct ExportWith(pub HashMap<String, serde_json::Value>) component for model description entities

    • Add a pub struct ExportHandlers(pub HashMap<String, ExportHandler>) resource that contains a map of export handlers

    • When exporting an instance, apply the export handlers to it

      • Each ExportHandler is a BoxedSystem<In = (Entity, serde_json::Value), Out = serde_json::Value>
      • For each entry in ExportWith, get the associated ExportHandler from ExportHandlers and pass in the (Entity, serde_json::Value) where the Entity is for the instance and the serde_json::Value is from the ExportWith.
      • The output of the ExportHandler gets inserted as an element into the <model> for the instance

    • Users can provide widgets to enable/disable and configure their export handlers

  • We will provide an ExportHandler for the slotcar plugin out of the box

A few thoughts on this.
First of all SDF plugins would use XML, not JSON. This is especially important because right now we don't have a public dependency on the XML library and we just use sdformat_rs types, having such an API would bind us into a specific XML library (XML support is really not too good in the Rust ecosystem, we learned this the hard way in urdf-rs that had to switch libraries several times).
My recommendation here would be to use the sdformat_rs SdfPlugin type. Its API is not terribly great but nested arbitrary structures that can contain arbitrary sequences of values are a bit tough to make user friendly.
This is of course assuming we only want to export plugins, if we want to be able to export arbitrary tags this won't really work.

I would also be happy to simplify / streamline some the simulation properties to bring them more inline to what is directly relevant for Open-RMF / the site editor, for example, looking at a slotcar tag from TinyRobot:

      <nominal_drive_speed>0.5</nominal_drive_speed>
      <nominal_drive_acceleration>0.25</nominal_drive_acceleration>
      <max_drive_acceleration>0.75</max_drive_acceleration>

      <nominal_turn_speed>0.6</nominal_turn_speed>
      <nominal_turn_acceleration>1.5</nominal_turn_acceleration>
      <max_turn_acceleration>2.0</max_turn_acceleration>

      <tire_radius>0.1</tire_radius>
      <base_width>0.3206</base_width>

      <stop_distance>0.75</stop_distance>
      <stop_radius>0.75</stop_radius>

      <!-- Can the robot drive backwards -->
      <reversible>true</reversible>

      <!-- Battery params -->
      <nominal_voltage>12.0</nominal_voltage>
      <nominal_capacity>24.0</nominal_capacity>
      <charging_current>5.0</charging_current>

      <!-- Physical params -->
      <mass>20.0</mass>
      <inertia>10.0</inertia>
      <friction_coefficient>0.22</friction_coefficient>

      <!-- Power systems -->
      <nominal_power>20.0</nominal_power>

• I would argue that max_[drive, turn]_acceleration is a pretty obscure parameter and just setting it to (for example) 2x nominal_[drive, turn]_acceleration will cover pretty much everything. From what I understand the next generation only has velocity parameters and not acceleration as well? If that's the case we could also remove the acceleration? Or we just leave the acceleration in and hardcode its value?
• tire_radius and base_width are not relevant anymore since we just move robots as a whole and don't do differential drive wheel control anymore.
• stop_distance and stop_radius are both somewhat obscure, we can just improve the plugins to calculate the robot bounding box and set the e-stop distance to [bounding box x size] * [safety_factor].
• For physical params, mass and inertia should just be calculated from the model itself, I think it should be possible to just go through all links and add up all their masses and get rid of these parameters. Unclear about friction_coefficient, I suppose we can keep it.
• We should keep all the battery related parameters.

So it can be probably simplified to:

      <nominal_drive_speed>0.5</nominal_drive_speed>
      <nominal_drive_acceleration>0.25</nominal_drive_acceleration>

      <nominal_turn_speed>0.6</nominal_turn_speed>
      <nominal_turn_acceleration>1.5</nominal_turn_acceleration>
      
      <!-- Can the robot drive backwards -->
      <reversible>true</reversible>

      <!-- Battery params -->
      <nominal_voltage>12.0</nominal_voltage>
      <nominal_capacity>24.0</nominal_capacity>
      <charging_current>5.0</charging_current>

      <!-- Physical params -->
      <friction_coefficient>0.22</friction_coefficient>

      <!-- Power systems -->
      <nominal_power>20.0</nominal_power>

[mxgrey]

First of all SDF plugins would use XML, not JSON.

Good call, I've updated the description slightly so that ExportHandler should output an sdformat_rs::XmlElement. I don't want to limit the export extension system to only plugins because I can imagine users wanting to export other things based on custom data that they put into the application. It should be easy enough to add a impl From<SdfPlugin> for XmlElement into sdformat_rs.

I think we should continue using serde_json::Value for the in-app (inside site editor) extension data, because that's the data that will get serialized and saved in the .site[.ron][.json][.yaml] file.

having such an API would bind us into a specific XML library (XML support is really not too good in the Rust ecosystem, we learned this the hard way in urdf-rs that had to switch libraries several times).

This is a fair point. I noticed sdformat_rs has a build dependency on xmltree which seems to have a very nice Element class. It's not obvious to me why sdformat_rs sort of reimplements that with its own simpler XmlElement class. Is that just to avoid a public dependency on xmltree?

I think in the long term we should consider having sdformat_rs depend publicly on xmltree and just re-exporting xmltree's data structures. After looking around at various xml crates, xmltree looks several leagues better than the rest and seems to support everything we'd want and more.

[luca-della-vedova]

This is a fair point. I noticed sdformat_rs has a build dependency on xmltree which seems to have a very nice Element class. It's not obvious to me why sdformat_rs sort of reimplements that with its own simpler XmlElement class. Is that just to avoid a public dependency on xmltree?

I think in the long term we should consider having sdformat_rs depend publicly on xmltree and just re-exporting xmltree's data structures. After looking around at various xml crates, xmltree looks several leagues better than the rest and seems to support everything we'd want and more.

Interesting, I can't remember the exact reason for this choice other than simplicity / API convenience, on the top of my head:

• The API for SdfPlugin is more convenient since it makes our common use case of "just get an element by name" easier and faster (it has a hashmap based storage, so we can just do a get(name), Element seems to only have a vector of children so it would always be a .iter().find(name).)
• Again API wise, the text that is wrapped in the Element is a String in xmltree while it's a custom type for our own implementation. That allows us to create traits to make our API simpler (such as TryFrom to convert to numbers).
• SDF plugins are very simple and I wouldn't expect anything like namespaces, prefixes, or complex data types XMLNode has 5 when we really only care about nested children, plain text and possibly comments.

So for example let's say we have an element and we want to look for a child called foo that contains an f64, in sdformat_rs that would be:

let val = bar.data.get("foo").map(|v| v.try_into().ok())?;

While in XMLTree I believe it would be something like:

let val = bar
             .children
             .iter()
             .find(|c| {
                 // Filter by children that are elements
                 let Element(e) = c else {
                     return None;
                 };
                 // Make sure the name of the child is correct
                 if e.name != "foo" {
                     return None;
                 };
                 // Find the child of the correct element that has text in it
                 let t = e.children.iter().find(|c| {
                     match c {
                         Text(t) => Some(t),
                         _ => None,
                     }
                 })?;
                 // Parse as the desired type
                 t::parse::<f64>().ok()
             })?;

[mxgrey]

the text that is wrapped in the Element is a String in xmltree

Unless I'm misunderstanding it looks like a vector of XmlNode which is similar to our custom type but more comprehensive.

But this is pretty off-topic for this issue ticket, so I've opened open-rmf/sdf_rust_experimental#17 to continue the conversation if there's any interest in that in the future.