Orientation compared to Clipper 1

[philstopford]

This comes from #83 and I'm still rather bothered by the change. It seems to break expectations compared to Clipper 1 and I'm struggling to retain that old behavior.

My first expectation is that all outers have a consistent representation coming out of Clipper. It might not be my first choice of representation, but a lot of code was built on the observed behavior from Clipper 1, for better or worse, and having some mode that gives outers the same orientation, with cutters being the opposite orientation, would be very helpful for migration; up until recently I was using Clipper 2 without any issues; the recent changes have broken a lot in my local libraries and, staring at the problem, I am struggling to see how to start repairing it all if there's no migration option. I don't dismiss the reason for the change at all, but I really don't understand it. I saw no problems with the prior implementation for my use cases.

I've tried the preprocessor REVERSE_ORIENTATION, which provided no relief, and the property ReverseSolution doesn't exist for offset. Offsetting as well as clipping seems to have changed, which makes the situation more awkward again : I don't always run the offset through a Clipper instance, for example, when doing clean-up :

Paths64 source = new() {
 ClipperFunc.MakePath(new [] {
-200000,-200000, 
200000,-200000, 
200000,200000, 
-200000,200000, 
-200000,-200000
}), 
 ClipperFunc.MakePath(new [] {
-100000,-100000, 
-100000,100000, 
100000,100000, 
100000,-100000, 
-100000,-100000
})
); 

I feed this through an offset to remove any slivers and gaps. There are some constants below, but in the library code, these values are changeable based on need :

        Paths cGeometry = new();

        ClipperOffset co = new();
        co.AddPaths(source, joinType, EndType.Polygon);
        cGeometry = co.Execute(customSizing);
        co.Clear();
        co.AddPaths(cGeometry.ToList(), joinType, EndType.Polygon);
        cGeometry.Clear();
        cGeometry = co.Execute(-customSizing); // Size back to original dimensions

In the old code, this yielded :

cGeometry = {List<List<Point64>>} Count = 2
 [0] = {List<Point64>} Count = 4
  [0] = {Point64} 200000,-200000,0 
  [1] = {Point64} -200000,-200000,0 
  [2] = {Point64} -200000,200000,0 
  [3] = {Point64} 200000,200000,0 
 [1] = {List<Point64>} Count = 4
  [0] = {Point64} 100000,-100000,0 
  [1] = {Point64} 100000,100000,0 
  [2] = {Point64} -100000,100000,0 
  [3] = {Point64} -100000,-100000,0 

In the new code, though, I get :

cGeometry = {List<List<Point64>>} Count = 2
 [0] = {List<Point64>} Count = 5
  [0] = {Point64} -200000,-200000,0 
  [1] = {Point64} 200000,-200000,0 
  [2] = {Point64} 200000,200000,0 
  [3] = {Point64} -200000,200000,0 
  [4] = {Point64} -200000,-200000,0 
 [1] = {List<Point64>} Count = 5
  [0] = {Point64} -100000,-100000,0 
  [1] = {Point64} -100000,100000,0 
  [2] = {Point64} 100000,100000,0 
  [3] = {Point64} 100000,-100000,0 
  [4] = {Point64} -100000,-100000,0 

Even with REVERSE_ORIENTATION specified, I get a different result than I was looking for.:

cGeometry = {List<List<Point64>>} Count = 2
 [0] = {List<Point64>} Count = 4
  [0] = {Point64} 200000,-200000,0 
  [1] = {Point64} 200000,200000,0 
  [2] = {Point64} -200000,200000,0 
  [3] = {Point64} -200000,-200000,0 
 [1] = {List<Point64>} Count = 4
  [0] = {Point64} -100000,100000,0 
  [1] = {Point64} 100000,100000,0 
  [2] = {Point64} 100000,-100000,0 
  [3] = {Point64} -100000,-100000,0 

I am contemplating further abusing the Z callback to allow me to track original point order through the computation, which may also help me workaround the orientation changes elsewhere in Clipper, so that I would forcibly respin things back into compliance, but am not sure whether I would win in all cases due to the need to track origin polygon index in various scenarios.

[philstopford]

TLDR version : results now are seemingly reversed from what went before. Offset does not allow for reverse solution to be set.

[AngusJohnson]

results now are seemingly reversed from what went before.

That may be true, and may be reversed yet once I have another look and am convinced that current implementation breaks convention.

Offset does not allow for reverse solution to be set.

Fair point, and easly fixed by adding in a ReverseSolution property to ClipperOffset too.

[philstopford]

I had a second discussion open for open path clipping; that seems to have gone MIA.

[philstopford]

I should also note that I provide all the background so that the request has context behind it. Not to intentionally bore or frustrate, so the TLDR may be unduly terse/concise.

[AngusJohnson]

I had a second discussion open for open path clipping; that seems to have gone MIA.

Phil, please lets try and sort this out one step at a time.

[Edit: I've deleted some comments I made because I misunderstood the tone of Phil's posts. And sorry Phil for that misunderstanding 😢.]

I will look at the orientation of open paths again and compare this with Clipper1. However, the current changes to orientation really haven't changed the clipping process, just whether the results of clipping will or won't be reversed. But I will take this more slowly now.

[philstopford]

I'm sorry - I think my tone is misconstrued. I'm just trying to figure out what the right answer is, not intending to blame or annoy. The detail is there to back-up my observations and explain why I'm lost in the weeds, and why the change is impacting me. I suspect it might be that I need to change something on my side, but with the changes as they are I don't understand what knob to turn :D

TLDR again : I'm frustrated that I can't figure this out on my own, certainly not with you and your efforts, and I'm trying to provide enough detail that people smarter than me can point me in the right direction

[rs0xFFFF]

Difficult topics should first be allowed to soften for a while. A solution will be found later.

[AngusJohnson]

Hi again Phil.

You've probably noticed that I've just made major changes to how the library handles orientation.
However, I haven't yet investigated how this affects open path clipping. That's what I intend to do next 😁.

Clipping solutions of closed paths should now always be Positive except when Clipper's ReverseSolution property is set true.
Conversely, solutions from offsetting (inflating) closed paths should retain the orientation of the input paths

[AngusJohnson]

Hi again Phil!
I've just uploaded a new revision that has fixed several very significant bugs in open path clipping, so 🤞. 😜
I've also amended the code so that in open paths, vertex order should now be preserved 🎂🍾.
Looking at what happens in Clipper1, as I suspected there appears to be no defined vertex order for open paths there.
IOW, in Clipper1 open path solutions are sometimes reversed.

[AngusJohnson]

but perhaps I was simply lucky

Here's the console output of a very simple test I ran earlier (code attached),
though asterisks have been added for emphasis 😁 ...
Edit: I notice that it's only horizontal lines that are reversed.

Clipper1 test
8 open subjects:
200,200 300,200
200,200 300,100
200,200 200,100
200,200 100,100
200,200 100,200
200,200 100,300
200,200 200,300
200,200 300,300
clip:
0,0 400,0 400,400 0,400

Operation: Intersection, NonZero:

Solution:
200,200 300,300
200,200 200,300
200,200 100,300
100,200 200,200   ******
200,200 100,100
200,200 200,100
200,200 300,100
300,200 200,200   ******

Clipper1LinesTest.zip

Either way, are you ready for me to kick the tires on the library again,

Yes, but please make sure you take your boots off first 😜.

[philstopford]

I'm seeing changes in results and am working on the detailed review. I'll need a few days, I think, to put the pieces together and figure out what's going on. Stay tuned.

[AngusJohnson]

If there are any reversed open paths (in clipping solutions), I will now consider that a 🐛.

[philstopford]

Just adding some test review here, for my notes and the record.... The 'expected output' label comes from the May 27 or so Clipper 2 code, with the exact same test.

Also note that I had to set this line to get consistent orientation in other cases with that previous version of the library - this line is in effect for all testing with the current code :

public const bool DEFAULT_ORIENTATION_IS_REVERSED = true;

I don't think this bool is being recognized in the offsetting code, though. Searching, I don't see reference to it and the results below suggest it has no effect here.

    public static void edgeOffsetTest()
    {
        Path64 edge = ClipperFunc.MakePath(new []
        {
            -100000, 99500,
            -100000, 200500
        });

        ClipperOffset co = new();
        co.AddPath(edge, JoinType.Miter, EndType.Square);
        Paths64 p = co.Execute(500);
        
        /* Expected output
        p = {List<List<Point64>>} Count = 1
         [0] = {List<Point64>} Count = 4
          [0] = {Point64} -100250,99250,0 
          [1] = {Point64} -100250,200750,0 
          [2] = {Point64} -99750,200750,0 
          [3] = {Point64} -99750,99250,0 
           */

    }

The current library gives me :

p = {List<List<Point64>>} Count = 1
 [0] = {List<Point64>} Count = 4
  [0] = {Point64} -99750,200750,0 
  [1] = {Point64} -100250,200750,0 
  [2] = {Point64} -100250,99250,0 
  [3] = {Point64} -99750,99250,0 

which is the reverse orientation of the prior library.

Similar story with this code :

    public static void keyHole_test1()
    {
        Console.WriteLine("Clipper2 Test1");
        Path64 outer = ClipperFunc.MakePath(new []
        {
            -200000, -200000,
            200000, -200000,
            200000, 200000,
            -200000, 200000,
            -200000, -200000
        });

        Path64 inner1 = ClipperFunc.MakePath(new []
        {
            -100000, -100000,
            -100000, 100000,
            100000, 100000,
            100000, -100000,
            -100000, -100000
        });

        Paths64 kHSource = new()
        {
            outer,
            inner1
        };
        
        ClipperOffset co = new();
        co.AddPaths(kHSource, JoinType.Miter, EndType.Polygon);
        // ClipperLib2 specifies full width in offset for open path, unlike version 1
        Paths64 out_ = co.Execute(2*keyhole_sizing);
        
        /* Expected output
        out_ = {List<List<Point64>>} Count = 2
         [0] = {List<Point64>} Count = 4
          [0] = {Point64} 201000,-201000,0 
          [1] = {Point64} -201000,-201000,0 
          [2] = {Point64} -201000,201000,0 
          [3] = {Point64} 201000,201000,0 
         [1] = {List<Point64>} Count = 4
          [0] = {Point64} 99000,-99000,0 
          [1] = {Point64} 99000,99000,0 
          [2] = {Point64} -99000,99000,0 
          [3] = {Point64} -99000,-99000,0 
           */
    }

Current code generates :

out_ = {List<List<Point64>>} Count = 2
 [0] = {List<Point64>} Count = 4
  [0] = {Point64} 201000,201000,0 
  [1] = {Point64} -201000,201000,0 
  [2] = {Point64} -201000,-201000,0 
  [3] = {Point64} 201000,-201000,0 
 [1] = {List<Point64>} Count = 4
  [0] = {Point64} -99000,-99000,0 
  [1] = {Point64} -99000,99000,0 
  [2] = {Point64} 99000,99000,0 
  [3] = {Point64} 99000,-99000,0 

[philstopford]

Open path clipping seems to be working in a manner similar to the 'good' results from before. All my tests look good; on the application side, I'm mostly good. There's a fail there, which I think is due to some previously-needed fragment clean-up that is causing trouble. I'm looking at that now, but I don't think there are library issues contributing at this point.

[AngusJohnson]

public const bool DEFAULT_ORIENTATION_IS_REVERSED = true;
I don't think this bool is being recognized in the offsetting code, though. Searching, I don't see reference to it and the results below suggest it has no effect here.

That's correct. The orientation of paths in offsetting solutions should match orientations in the input paths.
However, with open path inputs that have no orientation, the orientation of paths following offsetting is currently undefined (because I haven't checked this yet). Nevertheless, I strongly suspect these will always be Positive UNLESS ClipperOffset's ReverseSolution property has been set true.

which is the reverse orientation of the prior library.

If the orientation is opposite to what you want, just set ClipperOffset's ReverseSolution property to true.

[philstopford]

Indeed. To avoid missed usage, I just turned the constructor default for the reverse to true instead of false, and that then matches previous behavior. Continuing to run and review.

[philstopford]

I'm still working to mentally map out the relationship between IsPositive in this usage model and the IsClockwise approach from before. I am seeing signs that the code is producing the right overall shape, but the orientation isn't always what I'm expecting and I think it's needing me to adjust my post-process code in places to re-orient the shapes. More in the next day or so.

[AngusJohnson]

I'm still working to mentally map out the relationship between IsPositive in this usage model and the IsClockwise approach from before.

IsPositive() (Clipper2), IsClockwise() (earlier Clipper2) and Orientation() (Clipper1) all call Area() internally and should produce exactly the same results, though for a short time (when I was disoriented 😜) in Clipper2 these results were reversed when DEFAULT_ORIENTATION_IS_REVERSED was true.

      //Clipper1
      public static bool Orientation(Path poly)
      {
          return Area(poly) >= 0;
      }

      //Clipper2
      public static bool IsPositive(PathD poly,
      bool orientation_is_reversed = InternalClipperFunc.DEFAULT_ORIENTATION_IS_REVERSED)
    {
      return Area(poly, orientation_is_reversed) >= 0;
    }

[philstopford]

Alright, so now I'm reasonably confident things are as they should be. I'm getting some weird orientation quirkiness on my side, but I think that's largely from legacy re-spin of contours and I'll work to remove those and clean things up over time.

Nice work, Angus, and sorry the review took a while. Lots of things to poke at this end to see if anything exploded. I've got more to do through the rest of this week, but nothing looks/smells funny at the moment.

[AngusJohnson]

Thanks Phil, though I'm about to upload another revision that will mess things up for C# users. I'm about to rename the Clipper class Clipper64 so I can replace ClipperFunc (which I think is/was really cumbersome) with Clipper. Lots of other stuff too, but nothing that'll affect orientation which I'm hoping is finally sorted out.

[philstopford]

So far, it all seems well-behaved. Great work!