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

Add Support for Multiple Color Formats in Room Class #306

Merged
merged 13 commits into from
Dec 4, 2024

Conversation

kumar-sanjeeev
Copy link
Contributor

@kumar-sanjeeev kumar-sanjeeev commented Dec 1, 2024

This update allows the Room class to accept the color parameter in multiple formats:

  • RGB list: [R, G, B]
  • Named color string (e.g., "red", "blue")
  • Hexadecimal string (e.g., "#0000FF")

Unit tests have been added to verify the functionality.

Copy link
Owner

@sea-bass sea-bass left a comment

Choose a reason for hiding this comment

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

Great start! Besides my inline comments:

  1. This all needs to work for things besides rooms, such as locations, hallways, objects, and robots.
  2. The YAML loading/saving utilities need to also support serializing and deserializing these color names and hex codes to/from strings.

I'm okay if item 1 is reserved to follow-on PRs if you'd like to split it up, but 2 needs to be done and tested at least for the rooms case.

I'm also okay if no matter how a user specifies colors in code/YAML, they always serialize to a consistent RGB format in the to_yaml() methods. Probably easiest that way.

pyrobosim/examples/demo.py Outdated Show resolved Hide resolved
pyrobosim/pyrobosim/core/room.py Outdated Show resolved Hide resolved
pyrobosim/pyrobosim/core/room.py Outdated Show resolved Hide resolved
pyrobosim/pyrobosim/core/room.py Outdated Show resolved Hide resolved
pyrobosim/test/core/test_room.py Outdated Show resolved Hide resolved
@kumar-sanjeeev kumar-sanjeeev force-pushed the feat-colors-using-strings branch from 9859a4c to 7de16d5 Compare December 2, 2024 20:54
@kumar-sanjeeev
Copy link
Contributor Author

Great start! Besides my inline comments:

  1. This all needs to work for things besides rooms, such as locations, hallways, objects, and robots.
  2. The YAML loading/saving utilities need to also support serializing and deserializing these color names and hex codes to/from strings.

I'm okay if item 1 is reserved to follow-on PRs if you'd like to split it up, but 2 needs to be done and tested at least for the rooms case.

I'm also okay if no matter how a user specifies colors in code/YAML, they always serialize to a consistent RGB format in the to_yaml() methods. Probably easiest that way.

Point 1 : Yes, I am planning to do follow-on PRs for rest of things.

Regarding point 2, I tested the support for multiple color formats for Room object by modifying room args in test_world.yaml. It works as expected without any changes to from_yaml() method of WorldYamlLoader class in yaml utils.

Copy link
Owner

@sea-bass sea-bass left a comment

Choose a reason for hiding this comment

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

Gave it a closer look, and just a few more comments on consistency.

Agree that things should just work with the YAML loading/saving tools since parse_color() is very well factored out 👍🏻

pyrobosim/examples/demo.py Outdated Show resolved Hide resolved
pyrobosim/pyrobosim/core/room.py Outdated Show resolved Hide resolved
pyrobosim/pyrobosim/utils/general.py Outdated Show resolved Hide resolved
pyrobosim/pyrobosim/utils/general.py Outdated Show resolved Hide resolved
pyrobosim/pyrobosim/data/test_world.yaml Outdated Show resolved Hide resolved
- Update the docstring to include `string` as an alternative type.
- Change return type of `parse_color` from `list` to `tuple`.
- Update test cases to reflect modified return type
Copy link
Owner

@sea-bass sea-bass left a comment

Choose a reason for hiding this comment

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

Thank you!

pyrobosim/test/utils/test_general_utils.py Show resolved Hide resolved
@sea-bass sea-bass merged commit e4a501a into sea-bass:main Dec 4, 2024
6 checks passed
@kumar-sanjeeev kumar-sanjeeev deleted the feat-colors-using-strings branch December 18, 2024 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants