Skip to content

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

@edenhaus edenhaus added the pr: enhancement PR with Improve something label Dec 14, 2023
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.
@codecov
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
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.

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)

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

pre-commit.ci autofix

@edenhaus
Copy link
Member

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

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

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