Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make the inner bundle of Parented transparent #264

Closed
wants to merge 3 commits into from

Conversation

mxgrey
Copy link
Collaborator

@mxgrey mxgrey commented Jan 20, 2025

A small tweak to how to things nested inside of Parented get serialized:

Before

"166": {
    "parent": 1,
    "bundle": {
        "name": "tinyRobot2",
        "pose": {
            "trans": [
                20.423693,
                -5.312098,
                0.0
            ],
            "rot": {
                "yaw": {
                    "deg": 0.0
                }
            }
        },
        "description": 164,
        "optional_properties": []
    }
}

After

"166": {
    "parent": 1,
    "name": "tinyRobot2",
    "pose": {
        "trans": [
            20.423693,
            -5.312098,
            0.0
        ],
        "rot": {
            "yaw": {
                "deg": 0.0
            }
        }
    },
    "description": 164,
    "optional_properties": []
}

This PR reduces some unnecessary struct nesting in the serialized output.

Signed-off-by: Michael X. Grey <greyxmike@gmail.com>
@mxgrey mxgrey requested a review from xiyuoh January 20, 2025 08:25
@xiyuoh
Copy link
Member

xiyuoh commented Jan 20, 2025

@mxgrey Thank you for taking a look at this! I experimented with it previously and the issue with flattening the Bundle content is that the fields of the bundle will be stored as strings with "". This results in errors when loading the same site file back in. Tested out this PR and here's the error I'm getting when loading a saved file with serde(flatten):

2025-01-20T10:36:12.246615Z ERROR librmf_site_editor::workspace: Failed loading site SpannedError { code: ExpectedIdentifier, position: Position { line: 321, col: 7 } }

Adding a valid format (model_descriptions) vs. the currently error-prone format (model_instances) here for clarity:

model_descriptions: {
    86: (
      name: "OfficeChairBlack",
      source: (Search("OpenRobotics/OfficeChairBlack")),
      is_static: (true),
      optional_properties: ([]),
    ),
  },
  model_instances: {
    87: {
      "parent": 1,
      "name": "OfficeChairBlack",
      "pose": (
        trans: (6.562113, -6.721654, 0.0),
        rot: yaw(deg(243.52425)),
      ),
      "description": Some(86),
      "optional_properties": ([]),
    },

Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
@mxgrey
Copy link
Collaborator Author

mxgrey commented Jan 20, 2025

Whoops, I hadn't thought of running all the unit tests before opening this PR since I thought it would be a trivial change. Thanks for helping me understand why the RON roundtrip test was failing.

I fell into a bit of a rabbit hole with ron-rs/ron#548 and then ron-rs/ron#555 but it seems everything is working now. For this to work we'll need to target the github repo of RON until its next release, but I think RON will probably have a new release well before we do.

Copy link
Member

@xiyuoh xiyuoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for hunting down the solution, tried out with the demo map and it works great! ✨ The error remains if we add anything to optional_model_properties, i.e. Tasks which would also require serde(transparent) for loading to work. However these struct are pending some refactor in #263 so I'm happy to give an approval first.

@mxgrey
Copy link
Collaborator Author

mxgrey commented Jan 21, 2025

@xiyuoh Considering the merge conflicts that will come up due to the changes in optional model properties and the need to add #[serde(transparent)], I'll close this PR and you can just copy/paste the relevant parts from here over to #263.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants