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

Use vector based graphic (SVG) for map rendering #373

Merged
merged 10 commits into from
Dec 23, 2023

Conversation

lukakama
Copy link
Contributor

@lukakama lukakama commented Dec 13, 2023

Use vector based graphic (SVG) for map rendering.

Why? At least for my environment, the map rendered using only raster graphic results in a very low-res map representation. Vector based graphic allows to produce pretty looking maps with high-res elements for bot, charging station, traces and so on.

For comparison:

Raster based rendering (with a black background due DeebotUniverse/Deebot-4-Home-Assistant#268 )
image

Vector based rendering (not the same cleaning job)
image

TODO:

  • Integrate tests
  • Code cleanups

@lukakama lukakama changed the title Use vector based graphic ((SVG)) for map rendering Use vector based graphic (SVG) for map rendering Dec 13, 2023
@lukakama lukakama force-pushed the svg-map branch 3 times, most recently from 8d5c6f7 to 8d8259a Compare December 13, 2023 15:10
Copy link
Contributor

@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 for your PR :)
I'm happy to someone takes action on the map, which is nearly untouched since I forked the lib

deebot_client/map.py Outdated Show resolved Hide resolved
deebot_client/map.py Show resolved Hide resolved
@edenhaus edenhaus added the pr: enhancement PR with Improve something label Dec 14, 2023
deebot_client/map.py Show resolved Hide resolved
deebot_client/map.py Outdated Show resolved Hide resolved
deebot_client/map.py Outdated Show resolved Hide resolved
deebot_client/map.py Outdated Show resolved Hide resolved
deebot_client/map.py Show resolved Hide resolved
deebot_client/map.py Outdated Show resolved Hide resolved
deebot_client/map.py Outdated Show resolved Hide resolved
deebot_client/map.py Outdated Show resolved Hide resolved
lukakama and others added 4 commits December 16, 2023 14:10
Co-authored-by: Robert Resch <robert@resch.dev>
Fixed some comments.

Fixed a pylance error on svg_map to str conversion.
Copy link

codecov bot commented Dec 16, 2023

Codecov Report

Attention: 58 lines in your changes are missing coverage. Please review.

Comparison is base (2840030) 78.60% compared to head (537bf89) 79.66%.
Report is 1 commits behind head on dev.

Files Patch % Lines
deebot_client/map.py 21.62% 58 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #373      +/-   ##
==========================================
+ Coverage   78.60%   79.66%   +1.05%     
==========================================
  Files          64       64              
  Lines        2618     2582      -36     
  Branches      475      468       -7     
==========================================
- Hits         2058     2057       -1     
+ Misses        516      481      -35     
  Partials       44       44              

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

Copy link
Contributor

@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.

Are you planning to add some tests? I know the map is currently not tested, but it would be great to add some tests too (optional)

deebot_client/map.py Outdated Show resolved Hide resolved
deebot_client/map.py Outdated Show resolved Hide resolved
deebot_client/map.py Outdated Show resolved Hide resolved
deebot_client/map.py Outdated Show resolved Hide resolved
deebot_client/map.py Outdated Show resolved Hide resolved
deebot_client/map.py Outdated Show resolved Hide resolved
deebot_client/map.py Outdated Show resolved Hide resolved
@lukakama
Copy link
Contributor Author

Are you planning to add some tests? I know the map is currently not tested, but it would be great to add some tests too (optional)

I would, but I'm a bit overwhelmed on what and how to test the map code :\ .. Maybe I can start adding a test on point to path conversion code and on the trace path generation code (better than nothing..). I will then look for other thing to test if the time allows it.

Co-authored-by: Robert Resch <robert@resch.dev>
@edenhaus
Copy link
Contributor

pre-commit.ci autofix

@edenhaus
Copy link
Contributor

@lukakama I'm not sure if you missed my two questions on the remaining conversations. I'm planning to create a new release during Christmas, and would be nice to include your PR :)

@lukakama
Copy link
Contributor Author

@lukakama I'm not sure if you missed my two questions on the remaining conversations. I'm planning to create a new release during Christmas, and would be nice to include your PR :)

Yea, I read them but I wasn't able to quickly reply. I will be happy if the PR could be merged for Christmas, but I don't how much time I can dedicate to changes/tests. I will try.

@edenhaus
Copy link
Contributor

The map increase for my vacuum from 14kB (png) to 33kB (SVG) but I think that's not a problem and we can improve it later

@edenhaus edenhaus merged commit cd4a052 into DeebotUniverse:dev Dec 23, 2023
7 of 8 checks passed
@lukakama
Copy link
Contributor Author

The map increase for my vacuum from 14kB (png) to 33kB (SVG) but I think that's not a problem and we can improve it later

Yeah, it should be mainly due the trace path. When creating raw SVG it could be compressed a little (whitespaces can be removed from Path instructions, and that can reduce the size of a 20-30%, but I didn't found a way to do it with the library :( ). An alternative could be to use gzip compressed SVG (it should be supported by almost all browser). I will be glad to add it as a future improvement :) .

@lukakama
Copy link
Contributor Author

Ah, I need to do a pull request on the HA integration, as the SVG map requires correct content type to be reported from the image component.

@edenhaus
Copy link
Contributor

The map increase for my vacuum from 14kB (png) to 33kB (SVG) but I think that's not a problem and we can improve it later

Yeah, it should be mainly due the trace path. When creating raw SVG it could be compressed a little (whitespaces can be removed from Path instructions, and that can reduce the size of a 20-30%, but I didn't found a way to do it with the library :( ). An alternative could be to use gzip compressed SVG (it should be supported by almost all browser). I will be glad to add it as a future improvement :) .

See #375

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