feat: implement draw_rect and draw_filled_rect
#352
Conversation
|
Hi @edgarriba, Could you please provide your review and approve the workflow for this PR whenever you get a chance? Let me know if any changes are needed — I’m happy to address them. Looking forward to your feedback so I can proceed further. Thank you! |
crates/kornia-imgproc/src/draw.rs
Outdated
|
|
||
| /// Draws a line on an image inplace. | ||
| /// Helper function to set a pixel's color, handling bounds checking. | ||
| #[inline] |
There was a problem hiding this comment.
removed the explicit #[inline] hint and trust the compiler's optimization decisions.
crates/kornia-imgproc/src/draw.rs
Outdated
| #[inline] | ||
| fn set_pixel<const C: usize>(img: &mut Image<u8, C>, x: i64, y: i64, color: [u8; C]) { | ||
| if x >= 0 && x < img.cols() as i64 && y >= 0 && y < img.rows() as i64 { | ||
| let pixel_linear_index = (y * img.cols() as i64 + x) * C as i64; |
There was a problem hiding this comment.
Think if there’s any way that y * cols can be precomputed or similar so that later we iterate over sums
There was a problem hiding this comment.
While set_pixel itself doesn't loop, the optimization concern is valid for its callers. The refactoring of draw_rect and draw_filled_rect to use draw_horizontal_line_segment addresses this effectively. Inside draw_horizontal_line_segment, the base row offset (y_usize * cols_usize) is calculated only once per row segment, and then pixels within that segment are accessed efficiently.
crates/kornia-imgproc/src/draw.rs
Outdated
| /// | ||
| /// # Arguments | ||
| /// | ||
| /// * `img` - The image to draw on. | ||
| /// * `p0` - The start point of the line as a tuple of (x, y). | ||
| /// * `p1` - The end point of the line as a tuple of (x, y). | ||
| /// * `color` - The color of the line as an array of `C` elements. | ||
| /// * `thickness` - The thickness of the line. | ||
| /// * `thickness` - The thickness of the line. (Note: thickness > 1 is approximate). |
There was a problem hiding this comment.
There was a problem hiding this comment.
@edgarriba Pls check your current draw_line implementation approximates thickness > 1 by drawing a small filled square around each point on the Bresenham line. This can look blocky, especially for diagonal lines. The linked Pygame code likely implements a more geometrically accurate method for thick lines (e.g., drawing parallel lines or calculating the line's polygonal bounds and filling it).
For this iteration, we'll keep the current approximate thickness implementation in draw_line but acknowledge its limitation in the documentation more clearly.
There was a problem hiding this comment.
Also I've updated the documentation comment for draw_line to be much clearer about the fact that it uses a square approximation and that this can lead to blocky appearances, especially for diagonal lines, explicitly mentioning it differs from more precise algorithms.
crates/kornia-imgproc/src/draw.rs
Outdated
| let half_thickness = (thickness / 2) as i64; | ||
|
|
||
| // Draw horizontal lines | ||
| draw_line(img, (lx0, ly0), (lx1, ly0), color, thickness); // Top |
There was a problem hiding this comment.
Drawing the horizontal lines like this is very inefficient
There was a problem hiding this comment.
@edgarriba You are absolutely right. Calling draw_line (which uses Bresenham's algorithm) for simple horizontal lines is inefficient. I've introduced a new helper function draw_horizontal_line_segment. This function directly calculates the start and end indices within the image's data slice for the given row (y) and x range, and then fills that memory segment efficiently using chunks_exact_mut and copy_from_slice. The draw_rect function now uses this helper for drawing the top and bottom horizontal parts of the rectangle, significantly improving performance for those segments.
| let data = img.as_slice_mut(); | ||
| let data_len = data.len(); | ||
|
|
||
| for y in y_min..y_max { |
There was a problem hiding this comment.
implement an Interator based draw_horizontal_line and just draw them sequentally
There was a problem hiding this comment.
Agreed. The previous nested loop calculated pixel indices individually. I've refactored draw_filled_rect to iterate only through the necessary rows (y coordinates). Inside the loop, it now calls the same efficient draw_horizontal_line_segment helper function created for draw_rect. This avoids redundant calculations and leverages optimized memory filling for each row segment.
|
draw_rectanddraw_filled_rect