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 39 forks source link

X or Y negative-positive-negative causes ArgumentOutOfRangeException in Draw(). #170

Closed ControlDesign closed 3 years ago

ControlDesign commented 3 years ago

Negative, then positive, then negative X or Y causes DrawPathExtensions.Draw to throw ArgumentOutOfRangeException: Specified argument was out of the range of valid values. (Parameter 'edgeIdx').

To recreate use my 3 points and invoke Draw() as follows:

                    points = new List<PointF>(new PointF[] {
                        new PointF(170.92926f, 0f),
                        new PointF(166.381f, 4.548294f),
                        new PointF(180.50574f, 4.548294f),
                        new PointF(175.7349f, 9.319138f),
                        new PointF(147.33237f, 0f),
                        new PointF(137.5681f, -9.764252f),
                        new PointF(118.26215f, 0f),
                        new PointF(113.71387f, 4.548294f),
                        new PointF(175.76965f, 0f),
                        new PointF(132.60944f, -43.160202f)});

                    //Distill it down a little bit.
                    //Negative Y, then positive, then negative throws exception
                    points = new List<PointF>(new PointF[] {
                        //new PointF(170.92926f, 0f),
                        //new PointF(166.381f, 4.548294f),
                        //new PointF(180.50574f, 4.548294f),
                        //new PointF(175.7349f, 9.319138f),
                        //new PointF(147.33237f, 0f),
                        new PointF(110f, -10f),
                        new PointF(120f, 20f),
                        //new PointF(113.71387f, 4.548294f),
                        //new PointF(175.76965f, 45.01f),
                        new PointF(130f, -30f)
                        });

                    IPath open_simple_path = new PathBuilder().AddLines(points.ToArray()).Build();

                    //SixLabors.ImageSharp.Drawing.Processing.DrawPathExtensions.Draw(
                    //imageContext, Color.AliceBlue, grbx.pathc_global_skeleton[bone].Item2 * f_scale_fac, pathc_local2);
                    try
                    {
                        if (points.Count() >= 2 && bool_non_negative)
                            SixLabors.ImageSharp.Drawing.Processing.DrawPathExtensions.Draw(
                        imageContext, Color.AliceBlue, 3, open_simple_path);
                    } catch (Exception e2)
                    {
                        rtb1.Clear();
                        rtb1.Font = new System.Drawing.Font("Consolas", 10f);
                        rtb1.AppendText(e2.Message + "\n");

                        //autocode the point array that caused the exception
                        rtb1.AppendText($"points = new List<PointF>(new PointF[] {{\n");                                
                        for (int i = 0; i < points.Count(); i++)
                        {
                            rtb1.AppendText($"new PointF({points[i].X}f, {points[i].Y}f)");
                            if (i < points.Count() - 1)
                                rtb1.AppendText(",\n");
                        }
                        rtb1.AppendText($"\n}});\n");
                    }
brianpopow commented 3 years ago

do I understand it correctly that the following should trigger the issue?

var points = new List<PointF>(new[]
{
    new PointF(110f, -10f),
    new PointF(120f, 20f),
    new PointF(130f, -30f)
});

IPath path = new PathBuilder().AddLines(points.ToArray()).Build();
Draw here triggers the exeption...

I would like to keep the reproduction as minimal as possible.

ControlDesign commented 3 years ago

Yes thanks.

JimBobSquarePants commented 3 years ago

Can confirm that the minimal repro is sufficient.

[Fact]
public void MixedPositiveNegativePointsShouldNotThrowException()
{
    List<PointF> points = new(new[]
    {
        new PointF(110f, -10f),
        new PointF(120f, 20f),
        new PointF(130f, -30f)
    });

    IPath path = new PathBuilder().AddLines(points.ToArray()).Build();

    using var image = new Image<Rgba32>(200, 200, Color.Black);
    image.Mutate(x => x.Draw(Color.AliceBlue, 3, path));
}

An exception is thrown at line 56 in ActiveEdgeList.LeaveEdge(int edgeIdx) where we explicitly throw if an edge index is not found within the active edge list. I've created a branch js/issue170 with a test containing the relevant repro code to investigate the issue.

Removing that explicit throw allowed me to render the following but I'm not sure what the implications of doing so would be. @antonfirsov I'll likely need your guidance here.

170

EDIT. Looks like we end up doing the inverse of what we should,

Here's the equivalent in System.Drawing.

List<PointF> points = new(new[]
{
    new PointF(110f, -10f),
    new PointF(120f, 20f),
    new PointF(130f, -30f)
});

var path = new GraphicsPath();
path.AddLines(points.ToArray());

using var image = new Bitmap(200, 200);
using var graphics = Graphics.FromImage(image);
graphics.Clear(Color.Black);    
graphics.DrawPath(new System.Drawing.Pen(Color.AliceBlue,3),path);  

image

antonfirsov commented 3 years ago

I will look into this within a few days or a week maximum. The use case is very interesting and inspiring!

JimBobSquarePants commented 3 years ago

Thanks @antonfirsov

JimBobSquarePants commented 3 years ago

Hi @ControlDesign

We've just pushed a fix for the issue to our MyGet feed. The version number you require is 1.0.0-beta13.14.

https://www.myget.org/feed/sixlabors/package/nuget/SixLabors.ImageSharp.Drawing/1.0.0-beta13.14

All packages within that feed are subject to an identical testing and validation process as our NuGet so you can safely consume it. We're currently working on a release candidate for our next release.

ControlDesign commented 3 years ago

Not having any exception hick-ups while panning and zooming, it's nice. Circuit boards look good, all those tiny lines are antialiasing really good. RC means I could publish some packages and not worry literals changing around?

gerbtool1 .

JimBobSquarePants commented 3 years ago

RC means I could publish some packages and not worry literals changing around?

Exactly. We’ll lock the API in preparation for the major version release. Btw, those diagrams are amazing! It’s so cool seeing someone being so creative with our libraries.

HarelM commented 3 years ago

I think I just stumbled across this issue as well when using this library to draw thumbnail of routes in my server. It wouldn't be trivial for me to add a Nuget source so if you happen to release this to Nuget soon please let me know. I'm currently using version beta12.