Skip to content

Conversation

@shmuelzon
Copy link
Contributor

@shmuelzon shmuelzon commented Oct 18, 2025

This PR adds support for rotating the map according to the user's preference in the app. The value is a part of the cached map info structure so, if it's there, we include it in the map information, otherwise, we use the default 0 degrees.

This time I chose not to make use of the built-in SVG transformations as we still needed to calculate the new viewbox, taking into consideration the rotation, so we needed to calculate the new points anyway. This made the code more complicated as everything was done twice. In addition, I opted to support only 90-degree turns to avoid "heavy" trigonometry functions, I'll soon find out how much it hurt performance when the benchmark pipeline runs.

A few notes to consider when reviewing:

  • I'm sure there are better ways to "Rustify" my code, any suggestions are welcome
  • There are 3 places that need to handle the rotation where each does it a bit differently (different axes, offsets, etc.) so it might seem a bit cumbersome. I tried to separate those to functions to make the code a bit easier to read
  • The map info version of the map calculated all of the points when the information has arrived but the rotation angle might be set before/after. To handle that I save the "raw" X,Y values from the robot and only calculate the new positions when generating the SVG. This is a bit disappointing as it's now calculated each time the SVG is generated even though it's not likely to change, but I don't think I can rely on the order of the messages. It's also, for better or worse, similar to how the background image is generated
  • Regarding the tests, I currently generate each map data with all rotations (using the private _map_data.set_rotation_deg()), this seemed a bit wrong to me but I though it would cover more than adding one type of rotation to each map type (map_info and background image) or adding 8 more map data items to cover all the possibilities. Let me know what you think of it

As always, thank you for your continued work on this, I am now hooked on seeing the map updated in HA when the robot is cleaning :)

Edit - Since the map generation tests were changed to support rotation, the benchmark doesn't verify them against dev so I manually compared with https://codspeed.io/DeebotUniverse/client.py/runs/68eea224abcd9f647da55998:

test dev PR (with 0 degree rotation)
test1 26.3 ms 26.7 ms
test2 22.1 ms 22.4 ms
test3 8.5 ms 8.5 ms
test4 7.2 ms 7.2 ms

So it seems there is some loss on maps that are based on the background image. I'll try to inline the functions as you've done elsewhere and see if that makes any difference.

Edit 2 - It doesn't... I tried changing the functions to closures that would avoid the match on each iteration but the end result is still the same. Weird...

@codecov
Copy link

codecov bot commented Oct 18, 2025

Codecov Report

❌ Patch coverage is 98.94737% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 94.51%. Comparing base (35dd830) to head (ea66495).
⚠️ Report is 1 commits behind head on dev.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
deebot_client/map.py 88.88% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #1252      +/-   ##
==========================================
+ Coverage   94.47%   94.51%   +0.04%     
==========================================
  Files         148      148              
  Lines        5643     5707      +64     
  Branches      349      350       +1     
==========================================
+ Hits         5331     5394      +63     
  Misses        251      251              
- Partials       61       62       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@codspeed-hq
Copy link

codspeed-hq bot commented Oct 18, 2025

CodSpeed Performance Report

Merging #1252 will not alter performance

Comparing shmuelzon:add-support-for-map-rotation (ea66495) with dev (35dd830)

Summary

✅ 106 untouched
🆕 1 new

Benchmarks breakdown

Benchmark BASE HEAD Change
🆕 test_onCachedMapInfo[info2-expected_event2-expected_args2] N/A 1.9 ms N/A

@edenhaus edenhaus requested a review from Copilot October 23, 2025 11:33
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for rotating maps according to user preferences by integrating rotation angles from cached map information. The rotation angle is stored in the map data structure and applied during SVG generation, with support for 90-degree increments (0, 90, 180, 270 degrees) to avoid complex trigonometry.

Key changes:

  • Map rotation angle is extracted from cached map info events and stored in the map data structure
  • Rotation transformations are applied at three different levels: trace points, map subsets, and background images
  • Test coverage expanded to verify all rotation angles across existing test cases

Reviewed Changes

Copilot reviewed 12 out of 13 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
deebot_client/events/map.py Added angle field to Map dataclass with default value of 0
deebot_client/messages/json/map/cached_map_info.py Extract angle from map info dictionary with default fallback
deebot_client/map.py Store rotation angle and pass it to SVG generation; set rotation from cached map info event
deebot_client/rs/map.pyi Updated type stub to include rotation_deg parameter
src/map/mod.rs Added rotation parameter to coordinate calculation functions and updated all callers
src/map/map_info.rs Transform map info points using rotation before generating SVG paths
src/map/background_image.rs Implemented rotation-aware position calculators for map pieces and pixels
src/map/points.rs Convert trace points with rotation transformation
tests/init.py Modified extractor to return list of parameters instead of single parameter
tests/test_map.py Generate test cases for all rotation angles using private API
tests/messages/json/map/test_cached_map_info.py Added angle values to test data
tests/commands/json/map/test_cached_map_info.py Added angle values to test data

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines 197 to 201
path_points.push(Point {
x,
y,
connected: coords.next().unwrap_or("1").trim() != "3-1-0",
});
Copy link

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

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

The raw x and y coordinates are now stored without normalization, but the calc_point transformation is applied later during generate(). Consider adding a comment explaining that these coordinates will be normalized and rotated when generate() is called, as this is a significant change from the previous behavior where calc_point was applied immediately.

Copilot uses AI. Check for mistakes.
Comment on lines +243 to +247
.map(|(x, y)| Point {
x,
y,
connected: true,
})
Copy link

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

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

Similar to the outline processing, these raw coordinates are stored without normalization. Consider adding a comment explaining that calc_point transformation will be applied during generate() to clarify this deferred processing pattern.

Copilot uses AI. Check for mistakes.
Copy link
Member

@edenhaus edenhaus left a comment

Choose a reason for hiding this comment

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

I was not reviewing it in detail, but merging it as it is will increase the map generation for old devices up to 30%. Can you maybe ask AI to improve the code?
Just so you know, the performance tests are executed on cloud servers, which are much more powerful than the end devices users will use. I moved the map code to Rust, as some users had issues with the Python code back in the day.

angle,
id=f"{filename}-{module.DEVICE_CLASS}-{angle}",
)
for angle in [0, 90, 180, 270]
Copy link
Member

Choose a reason for hiding this comment

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

We should only create rotation tests for devices that support it and not everyone.
Especially, these tests should be maps that are also generated from the actual model.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey, it's closer to 3%, even less, not 30%.
If we assume that older models don't support rotation anyway, then I can remove the support for it when generating the background image but I don't know if there are any versions in between that use the background image but also allow rotation in the app, and those will break, I can remove and wait to see if anyone files a bug 🤷

BTW - It may also be, as you've found from your robot, that other robots can switch their hardware file to use the new map API and cut the SVG generation time to a third.

Please let me know your thoughts and I'll modify accordingly.

Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

I have considered the worst case:

  • yna5xi: 33.3/26.3 => ~26%
  • kr0277: 29/22.1 => ~37%

Only bots that have been recently updated support the new format. So any older bot will never get an update.
Are we sure that the angle can only be 0, 90, 180, 270? If not, we should create a warning log entry, so users will report it if there are different values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My app only allows 90 degree rotation
IMG_7530

I can add an error/warning just the same.

What do you say about rotation of the background image? Should I remove that as well with an error if it's ever attempted?

Copy link
Member

Choose a reason for hiding this comment

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

I would remove the background image rotation for now (as I'm expecting that it will not be used). Also, my bot is not even showing the rotation setting in the app.

As we are using an enum, we should create a log entry for invalid angles and set it to 0.
Please also remove the rotation from all tests expect your bot as the other don't support rotation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done (I hope the enum is what you were thinking of)

name: str
using: bool
built: bool
angle: int = field(default=0, kw_only=True)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
angle: int = field(default=0, kw_only=True)
angle: int

angle,
id=f"{filename}-{module.DEVICE_CLASS}-{angle}",
)
for angle in [0, 90, 180, 270]
Copy link
Member

Choose a reason for hiding this comment

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

I have considered the worst case:

  • yna5xi: 33.3/26.3 => ~26%
  • kr0277: 29/22.1 => ~37%

Only bots that have been recently updated support the new format. So any older bot will never get an update.
Are we sure that the angle can only be 0, 90, 180, 270? If not, we should create a warning log entry, so users will report it if there are different values.

name=map_info.get("name", ""),
using=map_info["using"] == 1,
built=map_info["built"] == 1,
angle=map_info.get("angle", 0),
Copy link
Member

Choose a reason for hiding this comment

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

Check the angle here and create a warning if it's not a valid one. In case of an invalid one, set it to 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The check/log and default values are implemented in Rust

const MAP_PIECE_SIZE: u16 = 100;
pub(super) const MAP_MAX_SIZE: u16 = 8 * MAP_PIECE_SIZE;

type PiecePositionCalculator = Box<dyn Fn(usize) -> (u16, u16)>;
Copy link
Member

Choose a reason for hiding this comment

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

Remove rotation support for now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}

pub(super) fn generate(&self) -> MapInfoGenerateResult {
pub(super) fn generate(&self, rotation_deg: i16) -> MapInfoGenerateResult {
Copy link
Member

Choose a reason for hiding this comment

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

Create an enum for the rotation with the 4 valid cases and use it directly in Python. See PositionType as an example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

angle,
id=f"{filename}-{module.DEVICE_CLASS}-{angle}",
)
for angle in [0, 90, 180, 270]
Copy link
Member

Choose a reason for hiding this comment

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

I would remove the background image rotation for now (as I'm expecting that it will not be used). Also, my bot is not even showing the rotation setting in the app.

As we are using an enum, we should create a log entry for invalid angles and set it to 0.
Please also remove the rotation from all tests expect your bot as the other don't support rotation


await block_till_done(event_bus)

map_obj._map_data.set_rotation_deg(angle)
Copy link
Member

Choose a reason for hiding this comment

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

We should not set the rotation here. Instead set the angle in the correct event

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@shmuelzon shmuelzon requested a review from edenhaus October 29, 2025 21:31
Copy link
Member

@edenhaus edenhaus left a comment

Choose a reason for hiding this comment

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

It looks amazing some small improvement and we can merge :)
I will bump afterwards the lib and try to include in HA 2025.11

"built": 1,
"name": "Erdgeschoss",
"isFastBuilding": 1,
"angle": 90,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"angle": 90,

Don't modify this test as I want to check the data as much possible to the original.
Please add a new tests with the result of your bot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

src/map/mod.rs Outdated
impl RotationAngle {
#[staticmethod]
fn from_int(value: i16) -> Self {
RotationAngle::from(value)
Copy link
Member

Choose a reason for hiding this comment

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

Please move the mapping from above here and raise instead of creating a log entry.

The reason behind this is that I had a discussion today with a colleague about it and it's easier to add new functionality instead of providing the wrong one (Using 0 instead the correct angle)

I don't expect that different values currently exist but we will see if users are reporting errors

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, done

@edenhaus edenhaus added the pr: enhancement PR with Improve something label Oct 31, 2025
@shmuelzon shmuelzon requested a review from edenhaus October 31, 2025 12:21
Copy link
Member

@edenhaus edenhaus left a comment

Choose a reason for hiding this comment

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

Thanks @shmuelzon 👍

@edenhaus edenhaus merged commit fda85af into DeebotUniverse:dev Oct 31, 2025
36 checks passed
@shmuelzon shmuelzon deleted the add-support-for-map-rotation branch October 31, 2025 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr: enhancement PR with Improve something

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants