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