feat: add SVG export functionality and update save_image to support S…#5
feat: add SVG export functionality and update save_image to support S…#5Czy014 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds SVG export support to the mono_pixel exporter pipeline, enabling save_image to auto-select PNG vs SVG based on the output file extension and documenting the new capability.
Changes:
- Added
export_to_svg()and updatedsave_image()to route by file suffix (.svg-> SVG, otherwise PNG). - Expanded unit tests to cover SVG export behavior and
save_imageSVG routing. - Exposed
export_to_svgfrom the package API and updated usage docs to describe SVG output.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/mono_pixel/exporter.py |
Implements SVG export and updates save_image to choose export format by extension. |
tests/test_exporter.py |
Adds new SVG-related tests for export_to_svg and save_image. |
src/mono_pixel/__init__.py |
Re-exports export_to_svg as part of the public package API. |
docs/usage.md |
Documents SVG output behavior and shows examples using save_image(..., .svg) / export_to_svg. |
💡 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)) | ||
| # 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.
The per-pixel getpixel calls inside nested Python loops will be very slow for larger images and will generate extremely large SVGs (one <rect> per foreground pixel). Consider using image.load()/bulk pixel access and emitting fewer elements (e.g., run-length encode rows into wider rects, or generate a <path>). This will significantly improve export speed and output size for typical large canvases.
| 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), | |
| }, | |
| ) | |
| pixels = image.load() | |
| for y in range(height): | |
| run_start = None | |
| for x in range(width): | |
| pixel = pixels[x, y] | |
| # For 1-bit images, 0 is foreground (black) | |
| is_fg = pixel == 0 | |
| if is_fg: | |
| if run_start is None: | |
| run_start = x | |
| else: | |
| if run_start is not None: | |
| run_width = x - run_start | |
| ET.SubElement( | |
| fg_group, | |
| "rect", | |
| { | |
| "x": str(run_start * pixel_size), | |
| "y": str(y * pixel_size), | |
| "width": str(run_width * pixel_size), | |
| "height": str(pixel_size), | |
| }, | |
| ) | |
| run_start = None | |
| # Flush any run that reaches the end of the row | |
| if run_start is not None: | |
| run_width = width - run_start | |
| ET.SubElement( | |
| fg_group, | |
| "rect", | |
| { | |
| "x": str(run_start * pixel_size), | |
| "y": str(y * pixel_size), | |
| "width": str(run_width * pixel_size), | |
| "height": str(pixel_size), | |
| }, | |
| ) | |
| elif image.mode == "L": | |
| pixels = image.load() | |
| for y in range(height): | |
| run_start = None | |
| for x in range(width): | |
| pixel = pixels[x, y] | |
| # For grayscale, treat dark pixels as foreground | |
| is_fg = isinstance(pixel, int) and pixel < 128 | |
| if is_fg: | |
| if run_start is None: | |
| run_start = x | |
| else: | |
| if run_start is not None: | |
| run_width = x - run_start | |
| ET.SubElement( | |
| fg_group, | |
| "rect", | |
| { | |
| "x": str(run_start * pixel_size), | |
| "y": str(y * pixel_size), | |
| "width": str(run_width * pixel_size), | |
| "height": str(pixel_size), | |
| }, | |
| ) | |
| run_start = None | |
| # Flush any run that reaches the end of the row | |
| if run_start is not None: | |
| run_width = width - run_start | |
| ET.SubElement( | |
| fg_group, | |
| "rect", | |
| { | |
| "x": str(run_start * pixel_size), | |
| "y": str(y * pixel_size), | |
| "width": str(run_width * pixel_size), | |
| "height": str(pixel_size), | |
| }, | |
| ) |
| 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.
Current SVG tests don’t cover non-white backgrounds, so the RGB foreground detection bug (hard-coded white background) won’t be caught. Add a regression test that exports an RGB canvas with bg_color != white (e.g., a pure black canvas, or a binarized image with bg_color="black", fg_color="white") and asserts the SVG does not contain per-pixel foreground rects for the background (e.g., only the single background <rect> is present when the image has no foreground pixels).
| # RGB or RGBA | ||
| 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.
export_to_svg treats any non-white RGB pixel as foreground (pixel[:3] != (255, 255, 255)), which breaks SVG output whenever the background color is not white (e.g., bg_color="black" or when save_image(..., strict_binarize=True, bg_color!=white) produces a two-color RGB image). This can result in every background pixel being emitted as a foreground rect and the SVG rendering incorrectly. Compare pixels against the computed background color (derived from bg_color / the processed image) rather than hard-coding white, and consider handling RGBA alpha consistently.
| # RGB or RGBA | |
| 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): | |
| # RGB, RGBA, or other multi-channel modes | |
| # Derive a background color from the image (top-left pixel) to avoid | |
| # assuming a white background. This handles cases where the background | |
| # is not white (e.g., custom bg_color in preprocessing). | |
| 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 if we cannot infer a 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: | |
| # Skip fully transparent pixels (treat as background) | |
| if len(pixel) >= 4 and pixel[3] == 0: | |
| continue | |
| # Treat pixels that differ from the inferred background color as foreground | |
| if pixel[:3] != bg_rgb: |
| width, height = image.size | ||
| svg_width = width * pixel_size | ||
| svg_height = height * pixel_size |
There was a problem hiding this comment.
pixel_size is used as a multiplier/divisor for the SVG dimensions and rect sizes, but there’s no validation that it’s a positive integer. Passing 0 or a negative value will produce an invalid SVG (zero/negative width/height) or silently generate degenerate rects. Add input validation (e.g., pixel_size >= 1) and raise a clear ValueError/ExportError if invalid.
add SVG export