Support Multiple Sources of Location and Object Metadata

[kumar-sanjeeev]

This is a draft PR to showcase proof-of-concept of accepting multiple sources of locations an objects metadata.

• add an add_metadata method to the World, Location, and Object classes, replacing the set_metadata method.
• update the EntityMetadata class to support multiple sources of location and object metadata.
• split the example_location_data.yaml and example_object_data.yaml files into separate files to simulate scenarios with multiple sources of metadata.
• tested the functionality using the demo.py script with various test worlds specified in form of YAML files in data directory,

Note: Test cases have not been updated yet to reflect the new changes. Will do if this proof-of-concept is okay.

[sea-bass]

Looks great! Left a combination of high- and low-level comments, let me know what you think.

[kumar-sanjeeev]

Looks great! Left a combination of high- and low-level comments, let me know what you think.

Thanks for reviewing. I will go through the comments in detail and get back to you. In short, I agree with keeping set_metadata and also in favor of adding add_metadata as an additional functionality.

[kumar-sanjeeev]

I tried to address all the comments and tested the changes with existing test cases and documentation generation. Everything seems fine. Please let me know if I missed anything. If all looks good, I will add new test cases for the add_metadata method.

[sea-bass]

Looks great! You can go ahead and add tests.

I think things could be further simplified all around if we allow set_metadata() and add_metadata() to accept lists of file paths in addition to single paths. See my comments throughout.

Also, remember to update docs as well:

• https://pyrobosim.readthedocs.io/en/latest/yaml/index.html
• https://pyrobosim.readthedocs.io/en/latest/yaml/world_schema.html

[kumar-sanjeeev]

Looks great! You can go ahead and add tests.

I think things could be further simplified all around if we allow set_metadata() and add_metadata() to accept lists of file paths in addition to single paths. See my comments throughout.

Also, remember to update docs as well:

• https://pyrobosim.readthedocs.io/en/latest/yaml/index.html
• https://pyrobosim.readthedocs.io/en/latest/yaml/world_schema.html

Thanks for reviewing !

I reviewed the existing test cases and updated some of them to test the add_metadata method. However, I haven't added any entirely new test cases specifically for add_metadata. If we need to add new test-cases, could you please guide me where I should add them?

I have also updated the documentation and tried to address all the comments. Let me know if there's any room for improvement or if I missed anything.

[sea-bass]

This looks way cleaner -- awesome!

Besides my comments, I think that World.set_metadata() should also support the list option, for consistency with add_metadata().

[kumar-sanjeeev]

This looks way cleaner -- awesome!

Besides my comments, I think that World.set_metadata() should also support the list option, for consistency with add_metadata().

I modified World.set_metadata to support list option.

[sea-bass]

Cool, just found one more thing and we should be good.

BTW, specifying a single metadata item in a world YAML file (without the - list syntax) still works, right?

[kumar-sanjeeev]

Cool, just found one more thing and we should be good.

BTW, specifying a single metadata item in a world YAML file (without the - list syntax) still works, right?

Yes it is working, I just checked once again to be sure.

[kumar-sanjeeev]

One observation regarding World.add_metadata and World.set_metadata, in the new implementation, we rely on Location.add_metadata and Object.add_metadata. The set_metadata method in both of these classes appears redundant. Should we remove it?

[sea-bass]

One observation regarding World.add_metadata and World.set_metadata, in the new implementation, we rely on Location.add_metadata and Object.add_metadata. The set_metadata method in both of these classes appears redundant. Should we remove it?

I think it's fine to remove since the add_metadata() and new clearout_metadata() methods will do the job.

by the way, can you rename to clear_metadata() instead? "clearout" isn't very standard naming.

[sea-bass]

I renamed the issue to remove the "Draft: Proof of Concept", as I saw you've taken this out of draft now.

Is there anything else remaining on your end (some tests or anything), or is this ready to go?

[kumar-sanjeeev]

I renamed the issue to remove the "Draft: Proof of Concept", as I saw you've taken this out of draft now.

Is there anything else remaining on your end (some tests or anything), or is this ready to go?

Thanks for renaming.

From my-side it is ready, I have added one test case that you suggested above:

To this end, would be good to have at least one test that checks this by having non-empty metadata, calling set_metadata(), and verifying that anything set before this call is gone.

[sea-bass]

LGTM -- thanks again @kumar-sanjeeev!