SixLabors / ImageSharp.Drawing

:pen: Extensions to ImageSharp containing a cross-platform 2D polygon manipulation API and drawing operations.
https://sixlabors.com/products/imagesharp-drawing/
Other
285 stars 38 forks source link

PolygonOffsetter throws ClipperException when DrawLine is called with 2 equal PointF values. #299

Closed woutware closed 1 year ago

woutware commented 1 year ago

Prerequisites

Description

PolygonOffsetter throws ClipperException when DrawLine is called with 2 equal PointF values.

Note that the DrawLineExtensions.DrawLines call did not throw this exception in version 1.0.0-beta14. It would be better if the behavior was the same, so I don't have to check all the points for equality before calling DrawLine. Either that, or provide the requirements for the input points, are they allowed to be equal? Are they allowed to be within 1e-8 of eachother, etc.

Steps to Reproduce

using System;

using SixLabors.ImageSharp.PixelFormats;
using SixLabors.ImageSharp;
using SixLabors.ImageSharp.Processing;
using SixLabors.ImageSharp.Drawing.Processing;

namespace ConsoleApp {
    public static class SixLaborsBug {

        public static void DrawLineBug() {
            Image<Bgra32> bitmap = new Image<Bgra32>(Configuration.Default, 100, 100, new Bgra32(0, 0, 0));
            PointF p = new PointF(4, 2);

            // Throws SixLabors.ImageSharp.Drawing.Shapes.PolygonClipper.ClipperException: 'An error occurred while attempting to clip the polygon. Check input for invalid entries.'
            bitmap.Mutate(
                context => {
                    context.DrawLine(Color.Aqua, 1f, p, p);
                }
            );
        }
    }
}

System Configuration

Windows 10, 64-bits.

JimBobSquarePants commented 1 year ago

Yeah, that's reasonable.

The exception is actually thrown in the polygon offsetter which is a port of Clipper2 offsetter (Changed from Clipper1 in the beta).

There are changes there that meant an empty solution is returned from the clipper which we were capturing and throwing for. I can change that to simply return the original outline which in this case would result in an empty render.

woutware commented 1 year ago

Reasonable is my middle name!

For my case, and maybe the general case, it's better that it would just draw a single pixel. ImageSharp.Drawing version beta14 also used to draw a pixel, so for me it's better if the output remains the same. Also, the reason for feeding 2 identical points to DrawLine is this: imagine a large (vector) drawing, and now you're zooming out a lot, so line segments become very small. Or you just don't have a lots of pixels allocated for the output, such that small line segments now suddenly will occupy the space of just a single pixel. Outputting no pixel at all would be against expectations.

So my first preference would be, retain the old behavior, which is output a pixel. Otherwise, it would be good for there to be an option to configure the DrawLine behavior for this edge case. But as default behavior I would still take the output a single pixel approach (if only for backwards compatibility).

antonfirsov commented 1 year ago

The shape to draw isn't really one pixel, it should depend on the line thickness and the EndCapStyle. When EndCapStyle == Round, we can draw a circle, in other cases we don't have a Reasonable option, but we can pick an arbitrary one or do an empty render.

woutware commented 1 year ago

Ah yes, I did not take into account the line thickness there. But I think the old beta14 behavior was reasonable here. When I adjust the sample program pen width to 20, beta14 will draw a 20 pixel wide rectangle:

using System;

using SixLabors.ImageSharp.PixelFormats;
using SixLabors.ImageSharp;
using SixLabors.ImageSharp.Processing;
using SixLabors.ImageSharp.Drawing.Processing;

namespace ConsoleApp3 {
    public static class SixLaborsBug {

        public static void DrawLineBug() {
            Image<Bgra32> bitmap = new Image<Bgra32>(Configuration.Default, 100, 100, new Bgra32(0, 0, 0));
            PointF p = new PointF(40, 30);

            // Throws SixLabors.ImageSharp.Drawing.Shapes.PolygonClipper.ClipperException: 'An error occurred while attempting to clip the polygon. Check input for invalid entries.'
            bitmap.Mutate(
                context => {
                    context.DrawLines(Color.Aqua, 20f, p, p);
                }
            );
            bitmap.SaveAsPng(@"C:\support\image.png");
        }
    }
}

I have attached the output png.

image

JimBobSquarePants commented 1 year ago

Clipper actually has handling for paths that deduplicate to a single point (including endcap handling) however the implementation is actually buggy! I've fixed that and notified upstream.

Here's various output examples following my PR changes. (Alias/Antialias at 1px and 5px)

DrawLinesInvalidPoints_Rgba32_T(1) DrawLinesInvalidPoints_Rgba32_T(1)_NoAntialias DrawLinesInvalidPoints_Rgba32_T(5) DrawLinesInvalidPoints_Rgba32_T(5)_NoAntialias

woutware commented 1 year ago

Excellent work Jim, output looks good.

You might even want to document the DrawLine method to define the expected behavior for edge cases like this so users know what to exect. And I guess it's also useful for implementers to have a definition to work off from.

JimBobSquarePants commented 1 year ago

v2.0.1 has been released with this fix.

woutware commented 1 year ago

Thanks Jim for the quick release, output looks again good and backwards compatible with beta14.