From a32a151da035ec796895f6022e3c7688b93944a2 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 22 Sep 2026 20:26:50 +0000 Subject: [PATCH] Clamp padding to the canvas the artwork actually gets [patch] Arguments.Validate checks padding against --size, but sizing is downscale-only, so the canvas is min(trimmedSquareSize, size). Artwork that trims smaller than --size therefore got a canvas the validated padding did not fit on. --size 100 --padding 45 validates, and then artwork trimming to 80 square asks for 90 pixels of padding on an 80 pixel canvas, driving the content size to 80 - 90 = -10. Resize threw on the non-positive dimension. ProcessDirectory's broad per-file catch turned that into a silently skipped file, a Failed count and exit code 2, on a batch the user had configured exactly as the help text and README documented. EffectivePadding now clamps to (canvasSize - 1) / 2, which leaves at least one pixel of content on any canvas. Clamping is preferred over reporting the file as failed because the requested padding is not wrong, only unsatisfiable on that input, and a directory of assorted icon sizes would otherwise fail exactly the small ones. Padding that already fits is applied unchanged, so this engages only where the tool used to throw. The help text, README and CLAUDE.md all stated the old constraint as if --size were the bound, so all three are corrected rather than left to imply the check is complete. Fixes #110 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XH3MikiXVCii3abMojvibs --- CLAUDE.md | 14 ++++++- IconHelper.Test/ProcessDirectoryTests.cs | 27 ++++++++++++++ IconHelper.Test/ProcessImageTests.cs | 47 ++++++++++++++++++++++++ IconHelper/Arguments.cs | 2 +- IconHelper/IconHelper.cs | 28 +++++++++++++- README.md | 8 +++- 6 files changed, 121 insertions(+), 5 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 2b0421e..82a60eb 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -100,7 +100,9 @@ those before changing the pixel maths. Three details in particular: Sizing is deliberately downscale-only: `finalSize = Math.Min(trimmedSquareSize, args.Size)`. Padding is applied by shrinking the *content* (`finalSize - padding * 2`) and padding back out, so the output -canvas is always `finalSize` square regardless of padding. +canvas is always `finalSize` square regardless of padding. The padding is first clamped against +`finalSize` by `EffectivePadding`, because `Validate` can only check it against `args.Size` and the +two differ whenever the artwork downscales. See the fixed bugs below. Files containing `.new.png` in their name are skipped, so re-running over an output directory is safe. @@ -168,6 +170,16 @@ Both are covered by regression tests. Do not reintroduce them. detects the inverted bounds and emits an empty square instead. Pinned by `ProcessImageTests.ProducesATransparentSquareWhenTheArtworkHasNoOpaquePixels` and `ProcessDirectoryTests.AFullyTransparentFileDoesNotStopTheBatch`. +- **Padding validated against the wrong size.** `Validate` checks `padding < size / 2`, but the + canvas is `min(trimmedSquareSize, size)`. Artwork trimming smaller than `--size` therefore got a + canvas the validated padding did not fit on, driving the content size to zero or negative, and + `Resize` threw. The broad per-file catch turned that into a silently failed file and exit code 2 + on a batch the user had configured exactly as documented. `EffectivePadding` now clamps to + `(finalSize - 1) / 2`. Pinned by + `ProcessImageTests.PaddingValidAgainstTheRequestedSizeStillWorksOnSmallerArtwork`, + `PaddingTooLargeForTheCanvasStillLeavesVisibleContent`, + `PaddingThatFitsIsAppliedExactlyAndNotClamped` and + `ProcessDirectoryTests.SmallArtworkWithValidatedPaddingDoesNotFailTheBatch`. ## Testing diff --git a/IconHelper.Test/ProcessDirectoryTests.cs b/IconHelper.Test/ProcessDirectoryTests.cs index 3c9e834..67fd630 100644 --- a/IconHelper.Test/ProcessDirectoryTests.cs +++ b/IconHelper.Test/ProcessDirectoryTests.cs @@ -272,4 +272,31 @@ public void WritesEightBitRgbaPngFiles() Assert.AreEqual(PngBitDepth.Bit8, png.BitDepth); Assert.AreEqual(PngColorType.RgbWithAlpha, png.ColorType); } + + [TestMethod] + public void SmallArtworkWithValidatedPaddingDoesNotFailTheBatch() + { + // The whole failure scenario end to end, at the layer where it actually cost the user + // something. `--size 100 --padding 45` passes Validate, but WritePng's artwork covers only + // the middle half of its canvas, so a 160 pixel file trims to 80 and the padding did not fit + // the effective canvas. Resize threw, the broad per-file catch swallowed it, and the file was + // counted as failed, taking an otherwise correct run to exit code 2. + using TempDirectory temp = new(); + string input = temp.Combine("in"); + string output = temp.Combine("out"); + Directory.CreateDirectory(input); + WritePng(Path.Combine(input, "small.png"), 160); + + Arguments args = ArgumentsFor(input, output); + args.Size = 100; + args.Padding = 45; + + Assert.IsTrue(args.Validate(out _), "Precondition: these are the documented, valid arguments."); + + BatchResult result = IconHelper.ProcessDirectory(args, NamedColors.White); + + Assert.AreEqual(0, result.Failed, "Arguments that validate must not fail a file."); + Assert.AreEqual(1, result.Written); + Assert.AreEqual(IconHelper.ExitSuccess, IconHelper.ExitCodeFor(result)); + } } diff --git a/IconHelper.Test/ProcessImageTests.cs b/IconHelper.Test/ProcessImageTests.cs index b3f00b6..c74152c 100644 --- a/IconHelper.Test/ProcessImageTests.cs +++ b/IconHelper.Test/ProcessImageTests.cs @@ -315,6 +315,53 @@ public void PaddingLeavesTheBorderTransparent() Assert.AreEqual(255, image[32, 32].A, "The centre of the artwork should remain opaque."); } + [TestMethod] + public void PaddingValidAgainstTheRequestedSizeStillWorksOnSmallerArtwork() + { + // Regression test. Arguments.Validate only checks padding against --size, but the canvas + // actually used is min(trimmedSquareSize, size) because sizing is downscale-only. Artwork + // that trims smaller than --size therefore gets a canvas the validated padding does not fit + // on: here 45 < 100/2 validates, but the real canvas is 80, so the content size used to come + // out as 80 - 90 = -10 and Resize threw. + using Image image = TestImages.Blank(200, 200); + TestImages.FillRect(image, 10, 10, 80, 80, OpaqueWhite); + + IconHelper.ProcessImage(image, NamedColors.White, 100, 45); + + Assert.AreEqual(80, image.Width, "The downscale-only canvas is unchanged by the padding clamp."); + Assert.AreEqual(80, image.Height); + } + + [TestMethod] + public void PaddingTooLargeForTheCanvasStillLeavesVisibleContent() + { + // The clamp has to leave at least one pixel of content: a canvas padded to nothing would be + // a silently blank icon, which is no better than the throw it replaces. + using Image image = TestImages.Blank(200, 200); + TestImages.FillRect(image, 10, 10, 80, 80, OpaqueWhite); + + IconHelper.ProcessImage(image, NamedColors.White, 100, 45); + + Assert.AreEqual(255, image[40, 40].A, "The centre of the canvas should still carry artwork."); + Assert.AreEqual(0, image[0, 0].A, "The clamped padding should still inset the artwork."); + } + + [TestMethod] + public void PaddingThatFitsIsAppliedExactlyAndNotClamped() + { + // The guard against the clamp reaching inputs that were always valid. 8 fits on the 80 pixel + // canvas, so the content must be inset by exactly 8 per side and no more: pixel 7 is padding + // and pixel 8 is the first row of artwork. + using Image image = TestImages.Blank(200, 200); + TestImages.FillRect(image, 10, 10, 80, 80, OpaqueWhite); + + IconHelper.ProcessImage(image, NamedColors.White, 100, 8); + + Assert.AreEqual(80, image.Width); + Assert.AreEqual(0, image[7, 40].A, "The last padding column should be transparent."); + Assert.AreEqual(255, image[8, 40].A, "The artwork should start exactly at the padding offset."); + } + [TestMethod] public void PreservesTransparencyOfTheSourceArtwork() { diff --git a/IconHelper/Arguments.cs b/IconHelper/Arguments.cs index 604dd8e..fc7022c 100644 --- a/IconHelper/Arguments.cs +++ b/IconHelper/Arguments.cs @@ -24,7 +24,7 @@ internal sealed class Arguments [Option('s', "size", Required = false, HelpText = "The maximum size of the icon. Defaults to 128.")] public int Size { get; set; } = 128; - [Option('p', "padding", Required = false, HelpText = "The number of pixels per size to pad the output image. Must be < (size / 2). Will not change the output size. Defaults to 0.")] + [Option('p', "padding", Required = false, HelpText = "The number of pixels per side to pad the output image. Must be < (size / 2), and is clamped further on artwork that downscales to less than size. Will not change the output size. Defaults to 0.")] public int Padding { get; set; } = 0; internal bool Validate(out Collection errors) diff --git a/IconHelper/IconHelper.cs b/IconHelper/IconHelper.cs index 7241310..09a4e7f 100644 --- a/IconHelper/IconHelper.cs +++ b/IconHelper/IconHelper.cs @@ -350,7 +350,8 @@ private static PixelBounds FlattenToCoverageAndMeasureBounds( /// /// Crops to the artwork, squares it off, and scales it down to at most /// pixels, insetting the content by per side - /// without changing the final canvas size. + /// without changing the final canvas size. The padding is clamped to what the canvas can carry, + /// see . /// private static void CropSquareAndResize(Image image, PixelBounds bounds, int size, int padding) { @@ -358,7 +359,7 @@ private static void CropSquareAndResize(Image image, PixelBounds bounds, // We intentionally only shrink the image and not grow it int finalSize = Math.Min(newSize, size); - int finalContentSize = finalSize - (padding * 2); + int finalContentSize = finalSize - (EffectivePadding(finalSize, padding) * 2); Rgba32 paddingColor = Rgba32.ParseHex("00000000"); image.Mutate(x => x @@ -373,4 +374,27 @@ private static void CropSquareAndResize(Image image, PixelBounds bounds, .Resize(finalContentSize, finalContentSize) .Pad(finalSize, finalSize, paddingColor)); } + + /// + /// The padding actually applied to a canvas of pixels, clamped so + /// that at least one pixel of content survives. + /// + /// + /// + /// checks the requested padding against --size, but + /// sizing is downscale-only, so the canvas is min(trimmedSquareSize, size). Artwork that + /// trims smaller than --size gets a canvas the validated padding need not fit on: + /// --size 100 --padding 45 validates, and then artwork trimming to 80 square asks for 90 + /// pixels of padding on an 80 pixel canvas. That drove the content size to zero or negative and + /// Resize threw, costing that file in a batch the user had configured correctly. + /// + /// + /// Clamping is preferred over reporting the file as failed because the requested padding is not + /// wrong, it is only unsatisfiable on this particular input, and a batch of assorted icon sizes + /// would otherwise fail exactly the small ones. Every padding that already fits is applied + /// unchanged, so this only engages where the tool used to throw. + /// + /// + private static int EffectivePadding(int canvasSize, int padding) + => Math.Min(padding, (canvasSize - 1) / 2); } diff --git a/README.md b/README.md index d439b33..b186266 100644 --- a/README.md +++ b/README.md @@ -247,6 +247,12 @@ the artwork leaves it alone. `finalSize - padding * 2` and then padded back out to `finalSize`, so the output is always `finalSize` square whatever the padding. +Because the canvas is `min(squareSize, --size)` rather than `--size`, the padding is measured against +that effective canvas and **clamped to `(finalSize - 1) / 2`** so at least one pixel of content +survives. This only engages on artwork that trims smaller than `--size`: `--size 100 --padding 45` +passes validation, but artwork trimming to 80 square would otherwise ask for 90 pixels of padding on +an 80 pixel canvas. Padding that already fits the effective canvas is applied exactly as requested. + The result is written as an 8-bit RGBA PNG, with the encoder set to clear the colour channels of fully transparent pixels so no invisible colour data is carried into the file. @@ -267,7 +273,7 @@ already contains its own output will not reprocess those files. | `-o` | `--output` | Yes | n/a | Path to the directory where modified files are written | | `-c` | `--color` | No | `#FFFFFF` | The colour to tint the icon with, as hex or a known name | | `-s` | `--size` | No | `128` | The maximum size, in pixels, of the output icon | -| `-p` | `--padding` | No | `0` | Pixels of padding per side. Must be less than `size / 2`. Does not change the output dimensions | +| `-p` | `--padding` | No | `0` | Pixels of padding per side. Must be less than `size / 2`, and is clamped further on artwork that downscales to less than `size`. Does not change the output dimensions | ## Exit Codes