Feature/help group#6
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds SVG export support to the mono-pixel exporter pipeline and improves CLI help output organization by grouping options into Rich help panels.
Changes:
- Add
export_to_svg()and routesave_image()to SVG vs PNG based on output file extension. - Expand unit tests to cover SVG export and
save_image(..., .svg)behavior. - Update CLI help text/panels and documentation to reflect
.pngvs.svgoutput.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/mono_pixel/exporter.py |
Adds SVG export implementation and extension-based routing in save_image(). |
tests/test_exporter.py |
Adds unit tests validating SVG output structure and save_image() SVG behavior. |
src/mono_pixel/cli.py |
Groups CLI options into Rich help panels and updates output help text. |
src/mono_pixel/__init__.py |
Re-exports export_to_svg from the package top-level. |
docs/usage.md |
Documents .svg output behavior and shows usage examples for SVG export. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for y in range(height): | ||
| for x in range(width): | ||
| pixel = image.getpixel((x, y)) | ||
| if isinstance(pixel, tuple) and len(pixel) >= 3: | ||
| # Treat non-white pixels as foreground | ||
| if pixel[:3] != (255, 255, 255): |
There was a problem hiding this comment.
SVG export treats any non-white RGB pixel as foreground (pixel[:3] != (255, 255, 255)), which breaks when the background color is not pure white (e.g., bg_color="black") and also breaks save_image(..., strict_binarize=True) because convert_to_monochrome produces an RGB image with a non-white background. Foreground detection should compare against the actual background color (or export from the 1-bit mask) so background pixels aren’t emitted as foreground rects.
| for y in range(height): | |
| for x in range(width): | |
| pixel = image.getpixel((x, y)) | |
| if isinstance(pixel, tuple) and len(pixel) >= 3: | |
| # Treat non-white pixels as foreground | |
| if pixel[:3] != (255, 255, 255): | |
| # Infer background color from the top-left pixel so we don't | |
| # treat background pixels as foreground when the background | |
| # is not pure white. | |
| bg_pixel = image.getpixel((0, 0)) | |
| if isinstance(bg_pixel, tuple) and len(bg_pixel) >= 3: | |
| bg_rgb = bg_pixel[:3] | |
| else: | |
| # Fallback to white to preserve previous behavior if we | |
| # cannot determine a tuple-based background color. | |
| bg_rgb = (255, 255, 255) | |
| for y in range(height): | |
| for x in range(width): | |
| pixel = image.getpixel((x, y)) | |
| if isinstance(pixel, tuple) and len(pixel) >= 3: | |
| # Treat pixels that differ from the background color as foreground | |
| if pixel[:3] != bg_rgb: |
|
|
||
| bg_svg = parse_color(bg_color) | ||
| fg_svg = parse_color(fg_color) | ||
|
|
There was a problem hiding this comment.
pixel_size is used to compute SVG dimensions and rect sizes but isn’t validated. A value of 0 or a negative value will generate an invalid SVG (zero/negative width/height and viewBox). Add an explicit validation (e.g., positive integer) in export_to_svg (and ideally also validate svg_pixel_size in save_image).
| if not isinstance(pixel_size, int) or pixel_size <= 0: | |
| raise ExportError(f"pixel_size must be a positive integer, got: {pixel_size}") |
| else: | ||
| # Default to PNG | ||
| return export_to_png(processed_image, output_path, dpi, optimize) | ||
|
|
||
|
|
There was a problem hiding this comment.
save_image currently defaults to PNG for any non-.svg suffix, which can silently write PNG bytes to a path with an unrelated extension (e.g., .jpg). Consider validating supported output suffixes (.png/.svg) and either raising a clear error for others or normalizing/adding the correct extension to avoid confusing outputs.
| else: | |
| # Default to PNG | |
| return export_to_png(processed_image, output_path, dpi, optimize) | |
| if suffix == ".png" or suffix == "": | |
| # Normalize paths without suffix to .png | |
| if suffix == "": | |
| output_path = output_path.with_suffix(".png") | |
| return export_to_png(processed_image, output_path, dpi, optimize) | |
| # Unsupported suffix: avoid writing PNG bytes to a misleading extension | |
| raise ExportError( | |
| f"Unsupported output format for '{output_path}'. " | |
| "Supported extensions are: .png, .svg." | |
| ) |
| # Get pixel data based on image mode | ||
| if image.mode == "1": | ||
| for y in range(height): | ||
| for x in range(width): | ||
| pixel = image.getpixel((x, y)) | ||
| # For 1-bit images, 0 is foreground (black) | ||
| if pixel == 0: | ||
| ET.SubElement( | ||
| fg_group, | ||
| "rect", | ||
| { | ||
| "x": str(x * pixel_size), | ||
| "y": str(y * pixel_size), | ||
| "width": str(pixel_size), | ||
| "height": str(pixel_size), | ||
| }, | ||
| ) | ||
| elif image.mode == "L": | ||
| for y in range(height): | ||
| for x in range(width): | ||
| pixel = image.getpixel((x, y)) | ||
| # For grayscale, treat dark pixels as foreground | ||
| if isinstance(pixel, int) and pixel < 128: | ||
| ET.SubElement( | ||
| fg_group, | ||
| "rect", | ||
| { | ||
| "x": str(x * pixel_size), | ||
| "y": str(y * pixel_size), | ||
| "width": str(pixel_size), | ||
| "height": str(pixel_size), | ||
| }, | ||
| ) |
There was a problem hiding this comment.
export_to_svg emits one <rect> per foreground pixel via nested for y/x loops and getpixel, which will be very slow and produce extremely large SVGs for bigger images (the test suite already uses 1024×1024 canvases). Consider compressing runs of pixels into fewer shapes (row-wise run-length rectangles, <path> segments), or at least using image.getdata() to avoid per-pixel getpixel overhead.
| # Get pixel data based on image mode | |
| if image.mode == "1": | |
| for y in range(height): | |
| for x in range(width): | |
| pixel = image.getpixel((x, y)) | |
| # For 1-bit images, 0 is foreground (black) | |
| if pixel == 0: | |
| ET.SubElement( | |
| fg_group, | |
| "rect", | |
| { | |
| "x": str(x * pixel_size), | |
| "y": str(y * pixel_size), | |
| "width": str(pixel_size), | |
| "height": str(pixel_size), | |
| }, | |
| ) | |
| elif image.mode == "L": | |
| for y in range(height): | |
| for x in range(width): | |
| pixel = image.getpixel((x, y)) | |
| # For grayscale, treat dark pixels as foreground | |
| if isinstance(pixel, int) and pixel < 128: | |
| ET.SubElement( | |
| fg_group, | |
| "rect", | |
| { | |
| "x": str(x * pixel_size), | |
| "y": str(y * pixel_size), | |
| "width": str(pixel_size), | |
| "height": str(pixel_size), | |
| }, | |
| ) | |
| # Use fast pixel access and compress horizontal runs of foreground pixels | |
| pixels = image.load() | |
| if image.mode == "1": | |
| # For 1-bit images, 0 is foreground (black) | |
| for y in range(height): | |
| x = 0 | |
| while x < width: | |
| pixel = pixels[x, y] | |
| if pixel == 0: | |
| # Start of a run of foreground pixels | |
| start_x = x | |
| x += 1 | |
| while x < width and pixels[x, y] == 0: | |
| x += 1 | |
| run_width = (x - start_x) * pixel_size | |
| ET.SubElement( | |
| fg_group, | |
| "rect", | |
| { | |
| "x": str(start_x * pixel_size), | |
| "y": str(y * pixel_size), | |
| "width": str(run_width), | |
| "height": str(pixel_size), | |
| }, | |
| ) | |
| else: | |
| x += 1 | |
| elif image.mode == "L": | |
| # For grayscale, treat dark pixels as foreground | |
| for y in range(height): | |
| x = 0 | |
| while x < width: | |
| pixel = pixels[x, y] | |
| is_fg = isinstance(pixel, int) and pixel < 128 | |
| if is_fg: | |
| # Start of a run of foreground pixels | |
| start_x = x | |
| x += 1 | |
| while x < width: | |
| next_pixel = pixels[x, y] | |
| if not (isinstance(next_pixel, int) and next_pixel < 128): | |
| break | |
| x += 1 | |
| run_width = (x - start_x) * pixel_size | |
| ET.SubElement( | |
| fg_group, | |
| "rect", | |
| { | |
| "x": str(start_x * pixel_size), | |
| "y": str(y * pixel_size), | |
| "width": str(run_width), | |
| "height": str(pixel_size), | |
| }, | |
| ) | |
| else: | |
| x += 1 |
| def test_export_custom_colors(self, tmp_path: Path): | ||
| """Custom colors work.""" | ||
| canvas = create_canvas(50, 50, "white") | ||
| output_path = tmp_path / "output.svg" | ||
| result = export_to_svg(canvas, output_path, bg_color="red", fg_color="blue") | ||
|
|
There was a problem hiding this comment.
The new SVG tests don’t cover non-white backgrounds/foreground detection (e.g., bg_color="black", fg_color="white") which is where SVG pixel classification is most error-prone. Add a regression test that exports an image with a non-white background (and possibly a single foreground pixel) and asserts the SVG doesn’t emit foreground rects for background pixels.
No description provided.