Simplify Drawing with new MCVrender #16
Conversation
…ean tested for rand. Signed-off-by: Sandro Cantarella <sandro@Sandros-Mac-mini.fritz.box>
Signed-off-by: Sandro Cantarella <sandro@Sandros-Mac-mini.fritz.box>
Signed-off-by: Sandro Cantarella <sandro@Sandros-Mac-mini.fritz.box>
…unction with C based from mcvrender. Signed-off-by: Sandro Cantarella <sandro@509dc541-f930-4e46-87da-4482e9d9a7bb.fritz.box>
WalkthroughRefactors rendering to use mvcrender primitives and preblended colors, removes the EnhancedDrawable implementation and its usages, adjusts initialize_drawing_config to return (DrawingConfig, Drawable), expands and reworks public exports/types and shared image attributes, consolidates logging via shared LOGGER, tweaks parsers/room flows, and bumps version and mvcrender dependency. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant H as Handler (before)
participant U as utils.initialize_drawing_config
participant D as Drawable
participant ED as EnhancedDrawable
participant M as mvcrender
rect rgba(230,240,255,0.5)
note over H,U: Old flow (pre-change)
H->>U: initialize_drawing_config(handler)
U-->>H: (drawing_config, D, ED)
H->>ED: async draw_map(...)
ED->>D: orchestration & element draws
D->>M: inline blending / stamps
end
sequenceDiagram
autonumber
participant H as Handler (after)
participant U as utils.initialize_drawing_config
participant D as Drawable
participant M as mvcrender
rect rgba(230,255,230,0.5)
note over H,U: New flow (post-change)
H->>U: initialize_drawing_config(handler)
U-->>H: (drawing_config, D)
H->>D: draw operations
D->>M: blend.get_blended_color / sample_and_blend_color
D->>M: draw.line_u8 / draw.circle_u8
note right of D: Preblend colors when alpha < 255
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
SCR/valetudo_map_parser/config/color_utils.py (1)
13-13: RemoveOptionalfromarrparameter
InSCR/valetudo_map_parser/config/color_utils.py:13, changearr: Optional[NumpyArray],to
arr: NumpyArray,since all call sites always pass a valid array.
SCR/valetudo_map_parser/rand256_handler.py (1)
117-124: ReturnNoneon failure inextract_room_properties.The method was refactored to return only the rooms dict, but the exception handler still returns
(None, None, None). When hit,self.room_proprietybecomes a tuple, and downstream code expecting a mapping (and the BaseHandler update path) misbehaves. ReturnNone(or re-raise) to keep the contract consistent.Consider:
- return None, None, None + return NoneSCR/valetudo_map_parser/hypfer_handler.py (1)
49-355: Fix synchronous call to async auto-crop helper.
AutoCrop.auto_trim_and_zoom_imageis still defined asasync definconfig/auto_crop.py, but here it's invoked withoutawait. That drops the coroutine object intoimg_np_array, so downstream code treats an async coroutine as a NumPy array and blows up (AttributeError: 'coroutine' object has no attribute 'shape', etc.).Refactor this block so the call is awaited (and, if necessary, adjust surrounding code to remain async-friendly). Without that, the first frame processed will crash.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
SCR/valetudo_map_parser/__init__.py(1 hunks)SCR/valetudo_map_parser/config/color_utils.py(2 hunks)SCR/valetudo_map_parser/config/drawable.py(8 hunks)SCR/valetudo_map_parser/config/drawable_elements.py(0 hunks)SCR/valetudo_map_parser/config/enhanced_drawable.py(0 hunks)SCR/valetudo_map_parser/config/rand256_parser.py(7 hunks)SCR/valetudo_map_parser/config/shared.py(4 hunks)SCR/valetudo_map_parser/config/status_text/status_text.py(1 hunks)SCR/valetudo_map_parser/config/types.py(4 hunks)SCR/valetudo_map_parser/config/utils.py(5 hunks)SCR/valetudo_map_parser/hypfer_handler.py(4 hunks)SCR/valetudo_map_parser/hypfer_rooms_handler.py(0 hunks)SCR/valetudo_map_parser/map_data.py(3 hunks)SCR/valetudo_map_parser/rand256_handler.py(10 hunks)SCR/valetudo_map_parser/reimg_draw.py(11 hunks)SCR/valetudo_map_parser/rooms_handler.py(1 hunks)pyproject.toml(2 hunks)
💤 Files with no reviewable changes (3)
- SCR/valetudo_map_parser/config/drawable_elements.py
- SCR/valetudo_map_parser/hypfer_rooms_handler.py
- SCR/valetudo_map_parser/config/enhanced_drawable.py
🧰 Additional context used
🧬 Code graph analysis (8)
SCR/valetudo_map_parser/config/shared.py (1)
SCR/valetudo_map_parser/config/types.py (1)
TrimsData(631-672)
SCR/valetudo_map_parser/rooms_handler.py (1)
SCR/valetudo_map_parser/map_data.py (1)
RandImageData(411-709)
SCR/valetudo_map_parser/config/utils.py (6)
SCR/valetudo_map_parser/map_data.py (2)
HyperMapData(713-817)Size(110-112)SCR/valetudo_map_parser/config/async_utils.py (1)
AsyncNumPy(17-35)SCR/valetudo_map_parser/config/status_text/status_text.py (1)
StatusText(16-96)SCR/valetudo_map_parser/config/types.py (1)
Destinations(38-42)SCR/valetudo_map_parser/rand256_handler.py (2)
get_rooms_attributes(367-377)get_calibration_data(507-528)SCR/valetudo_map_parser/hypfer_handler.py (2)
async_get_rooms_attributes(385-394)get_calibration_data(396-412)
SCR/valetudo_map_parser/__init__.py (3)
SCR/valetudo_map_parser/config/status_text/status_text.py (1)
StatusText(16-96)SCR/valetudo_map_parser/rand256_handler.py (1)
ReImageHandler(41-528)SCR/valetudo_map_parser/rooms_handler.py (2)
RandRoomsHandler(220-459)RoomsHandler(21-217)
SCR/valetudo_map_parser/config/drawable.py (2)
SCR/valetudo_map_parser/config/color_utils.py (1)
get_blended_color(8-104)SCR/valetudo_map_parser/config/colors.py (1)
sample_and_blend_color(457-519)
SCR/valetudo_map_parser/reimg_draw.py (1)
SCR/valetudo_map_parser/config/drawable.py (1)
create_empty_image(43-48)
SCR/valetudo_map_parser/rand256_handler.py (5)
SCR/valetudo_map_parser/config/auto_crop.py (1)
AutoCrop(27-452)SCR/valetudo_map_parser/config/async_utils.py (1)
AsyncPIL(38-66)SCR/valetudo_map_parser/config/utils.py (3)
initialize_drawing_config(701-724)async_zone_propriety(604-623)async_points_propriety(626-645)SCR/valetudo_map_parser/config/types.py (1)
RoomStore(97-129)SCR/valetudo_map_parser/map_data.py (1)
get_rrm_image_size(632-640)
SCR/valetudo_map_parser/hypfer_handler.py (5)
SCR/valetudo_map_parser/config/auto_crop.py (1)
AutoCrop(27-452)SCR/valetudo_map_parser/config/async_utils.py (1)
AsyncPIL(38-66)SCR/valetudo_map_parser/config/shared.py (1)
CameraShared(52-220)SCR/valetudo_map_parser/config/utils.py (1)
initialize_drawing_config(701-724)SCR/valetudo_map_parser/config/types.py (1)
RoomStore(97-129)
🔇 Additional comments (1)
SCR/valetudo_map_parser/config/color_utils.py (1)
3-3: LGTM! Unused import removed.The removal of
Tuplefrom the imports is appropriate since it's no longer used in the file.
| Pillow = ">=10.3.0" | ||
| scipy = ">=1.12.0" | ||
| mvcrender = ">=0.0.2" | ||
| mvcrender = ">=0.0.5" |
There was a problem hiding this comment.
mvcrender ≥0.0.5 requirement is unsatisfiable
There is no published mvcrender release ≥0.0.5 (latest on PyPI is 0.0.4), so installs will break until such a version exists. Please either pin to the latest released build or switch to a VCS/URL dependency that actually provides the new code.
Based on learnings
-mvcrender = ">=0.0.5"
+mvcrender = ">=0.0.4"🤖 Prompt for AI Agents
In pyproject.toml around line 21, the mvcrender requirement ">=0.0.5" is
unsatisfiable because PyPI only has 0.0.4; change the dependency to a valid
source: either pin to the latest released version by replacing with mvcrender =
"==0.0.4" or point to the actual VCS/URL that contains the new code (e.g., a
git+https://...@branch-or-commit) so installs will succeed; update the
dependency entry accordingly and run a local install to verify resolution.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
SCR/valetudo_map_parser/rand256_handler.py (1)
18-30: Remove duplicate LOGGER import.
LOGGERis imported twice fromSCR.valetudo_map_parser.config.types(lines 22 and 29). Remove one of the duplicate imports to improve code clarity.Apply this diff to remove the duplicate:
from .config.types import ( COLORS, DEFAULT_IMAGE_SIZE, DEFAULT_PIXEL_SIZE, LOGGER, Colors, JsonType, PilPNG, RobotPosition, RoomsProperties, RoomStore, - LOGGER, )
🧹 Nitpick comments (1)
SCR/valetudo_map_parser/rand256_handler.py (1)
116-116: Consider refactoring RoomStore side-effect pattern.
RoomStore(...)is called but its return value is immediately discarded with_. WhileRoomStoreuses a singleton pattern with side effects (storing instances in a class-level dict), calling a constructor just for side effects is a code smell.Consider one of these alternatives:
- If the instance is needed later, store it in a class/instance attribute
- If only registration is needed, extract a clearer method like
RoomStore.register(vacuum_id, room_properties)- If the call is unnecessary, remove it entirely
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
SCR/valetudo_map_parser/config/rand256_parser.py(3 hunks)SCR/valetudo_map_parser/rand256_handler.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- SCR/valetudo_map_parser/config/rand256_parser.py
🧰 Additional context used
🧬 Code graph analysis (1)
SCR/valetudo_map_parser/rand256_handler.py (4)
SCR/valetudo_map_parser/config/auto_crop.py (1)
AutoCrop(27-452)SCR/valetudo_map_parser/config/async_utils.py (1)
AsyncPIL(38-66)SCR/valetudo_map_parser/config/utils.py (1)
initialize_drawing_config(701-724)SCR/valetudo_map_parser/config/types.py (1)
RoomStore(97-129)
🔇 Additional comments (3)
SCR/valetudo_map_parser/rand256_handler.py (3)
42-50: LGTM! AutoCrop mixin correctly integrated.The AutoCrop mixin integration follows the expected pattern from the mvcrender library. BaseHandler is initialized first, followed by AutoCrop with the handler reference, which aligns with the documented usage pattern.
Based on learnings.
58-58: Approve removal of enhanced_draw usage. No references toenhanced_drawremain; change aligns with the updatedinitialize_drawing_configsignature.
274-274: Verify caught exceptions align with those actually thrown.No direct
raiseinasync_extract_room_properties; confirm onlyValueError,KeyError, orTypeErrorcan propagate—ifRuntimeErroris possible, include it in the except clause.
Signed-off-by: Sandro Cantarella <sandro@13de464c-e6f5-4665-b8d7-d6acf4207b24.fritz.box>
Signed-off-by: Sandro Cantarella <sandro@13de464c-e6f5-4665-b8d7-d6acf4207b24.fritz.box>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
SCR/valetudo_map_parser/rand256_handler.py (1)
69-119: Fix return type annotation of get_rooms_attributes
InSCR/valetudo_map_parser/rand256_handler.pyat line 365, change the signature fromasync def get_rooms_attributes(...) -> tuple[RoomsProperties, Any, Any]:to
async def get_rooms_attributes(...) -> RoomsProperties | None:
🧹 Nitpick comments (1)
SCR/valetudo_map_parser/rand256_handler.py (1)
266-266: Consider logging the exception type for better debugging.The broad exception handling (
ValueError, KeyError, TypeError) is appropriate for handling various room extraction failures, but the code doesn't log which exception occurred. This could make debugging harder if room extraction consistently fails.Consider adding a debug log:
- except (ValueError, KeyError, TypeError): + except (ValueError, KeyError, TypeError) as e: + LOGGER.debug( + "%s: Room extraction failed (%s), falling back to position-based zoom", + self.file_name, type(e).__name__ + ) # Fallback to robot-position-based zoom if room extraction fails
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
SCR/valetudo_map_parser/rand256_handler.py(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
SCR/valetudo_map_parser/rand256_handler.py (1)
SCR/valetudo_map_parser/config/auto_crop.py (1)
AutoCrop(27-452)
🔇 Additional comments (6)
SCR/valetudo_map_parser/rand256_handler.py (6)
217-224: Robot position detection logic is sound.The condition correctly updates
robot_posonly when:
- Room data is available (
self.rooms_pos)- Robot position is known
- Position hasn't been determined yet (
robot_pos is None) or is missing room information ("in_room" not in robot_pos)This prevents unnecessary recomputation while ensuring position data stays current.
393-398: Debug logging improves observability.The added debug log provides useful context for troubleshooting robot position detection and zoom behavior. The debug level is appropriate since this runs frequently during cleaning.
184-184: Documentation improvements enhance code clarity.The added docstrings for
_setup_robot_and_imageand_draw_map_elementshelp clarify the purpose of these internal helper methods.Also applies to: 280-280
5-5: Version bump appropriate for this release.The version increment to 0.1.10 is appropriate given the significant refactoring and API changes in this PR.
14-14: Verify mvcrender version and AutoCrop interface
The project’s pyproject.toml specifies mvcrender >=0.0.5, which meets the mixin’s minimum version. Confirm that callingAutoCrop.__init__(self, self)at line 49 aligns with the external constructor signature and that all invoked methods (validate_crop_dimensions,async_get_room_bounding_box,async_check_if_zoom_is_on) exist in the mvcrender.autocrop.AutoCrop class.
57-57: All callers updated for the two‐value return ofinitialize_drawing_config. Bothrand256_handler.pyandhypfer_handler.pynow unpack exactly(drawing_config, draw), and noenhanced_drawreferences remain.
This pull optimize the code removing unsued variables, isort and ruff the code as well drawing refactored with new blending and drawing lines and filled circles routines in C.
Summary by CodeRabbit
New Features
Changes
Bug Fixes
Chores