-
-
Notifications
You must be signed in to change notification settings - Fork 144
Add support for colorized rooms #1281
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 colorized rooms #1281
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #1281 +/- ##
==========================================
+ Coverage 94.51% 94.54% +0.02%
==========================================
Files 148 148
Lines 5707 5775 +68
Branches 350 350
==========================================
+ Hits 5394 5460 +66
- Misses 251 253 +2
Partials 62 62 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
CodSpeed Performance ReportMerging #1281 will not alter performanceComparing Summary
|
src/map/map_info.rs
Outdated
| CSSClass::RoomColor1, | ||
| CSSClass::RoomColor2, | ||
| CSSClass::RoomColor3, | ||
| CSSClass::RoomColor4, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why only 4 rooms? I'm expecting a normal flat has often more rooms
Can we have a different blue? It's nearly the same as the wall color and the other colors look a lot better. Maybe even don't have a blue et all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took the 4 colors that seem to repeat themselves from the app. The app also has the walls as gray (see #1177 (comment)) so that's another option but then it'll be different than what your app shows.
If we're good with deviating from the app, than I'd suggest adding more colors to reduce the risk of two rooms that have a shared edge having the same color. I couldn't understand yet the logic of the app to decide the color for each room but somehow (might be by chance) that doesn't happen even with "only" 4 colors.
Anyway, let me know what you prefer and I'll do that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember which command returns the connection between rooms, but maybe the app uses it to pick the correct color. Nevertheless, we should add more colors and make it better :)
We can try to go with the gray walls for all and see how it looks :)
Not sure if you want to add also the room colors for the png based map. If yes, you should only need to modify https://github.com/DeebotUniverse/client.py/blob/dev/src/map/background_image.rs#L70-L74
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I switched the walls to gray but, if you don't like it, #c6d5eb seems like a good replacement from the RoomColor1's blue. Also added two more colors:
ilt3k8 |
kr0277 |
|---|---|
![]() |
![]() |
Not sure if you want to add also the room colors for the png based map
I tried adding the following change:
diff --git a/src/map/background_image.rs b/src/map/background_image.rs
index 99a5101..7f29d6d 100644
--- a/src/map/background_image.rs
+++ b/src/map/background_image.rs
@@ -20,8 +20,14 @@ const MAP_IMAGE_PALETTE: &[u8] = &[
0x1a, 0x81, 0xed, // Carpet
0xde, 0xe9, 0xfb, // Not scanned space
0xed, 0xf3, 0xfb, // Possible obstacle
+ 0xa2, 0xbc, 0xe7, // Room color 1
+ 0xec, 0xd0, 0x99, // Room color 2
+ 0x9b, 0xd4, 0xda, // Room color 3
+ 0xec, 0xc6, 0xc9, // Room color 4
+ 0xd7, 0xbc, 0xe3, // Room color 5
+ 0xc3, 0xe2, 0xb6, // Room color 6
];
-const MAP_IMAGE_PALETTE_LEN: u8 = 6;
+const MAP_IMAGE_PALETTE_LEN: u8 = 12;
// 0 -> Transparent, 255 -> Fully opaque
// (first entry is transparent, rest opaque)
const MAP_IMAGE_PALETTE_TRANSPARENCY: &[u8] = &[0u8, 255, 255, 255, 255, 255];But the maps in the test didn't change so I guess they don't have that added information in them (?). Not sure if we should add that without a map that we can see it working on
| HashMap::from([( | ||
| Definition::DiagonalStripes, | ||
| (|| { | ||
| Box::new( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you wrapping everything in a Box? I removed some of them already in your previous PRs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably because I don't know any better and rely on compilation errors and LLMs 🤷♂️.
The structure of the hash map changed now because of the below request and it swears that it can't be removed anymore.
src/map/style.rs
Outdated
| pub identifier: &'static str, | ||
| pub value: &'static str, | ||
| pub class_name: &'static str, | ||
| pub required_defs: &'static [Definition], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to have a list if we only have at most one element
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought there might be a case in the future but I guess we can complicate it in the future, it needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @shmuelzon 👍


This PR adds support for coloring rooms/areas in the SVG map and it's only available for robots that support MapInfo.
Most of the elements remain the same as before. The main change is with the
Roomitems which now need to be drawn twice. Once with the color of the room and another time with the striped pattern overlayed on top of it. I couldn't find a way to have both a background color and an overlay on the same path. The new styles and striped line definitions are only added when needed to keep the SVG small.Examples:
Note that the background image-based maps have changed as a result but their content is the same and only the
defselement was moved to simplify the code.