Skip to content

Commit 1766c4b

Browse files
committed
Changes from Review
* some cosmetic changes * Added Aggressive Inlining and removed the need for func (lessIf)
1 parent bd14671 commit 1766c4b

File tree

4 files changed

+50
-70
lines changed

4 files changed

+50
-70
lines changed

src/PolygonClipper/PolygonClipper.cs

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -140,11 +140,6 @@ public Polygon Run()
140140
for (int i = 0; i < clipping.ContourCount; i++)
141141
{
142142
Contour contour = clipping[i];
143-
bool exterior = operation != BooleanOperation.Difference;
144-
if (exterior)
145-
{
146-
contourId++;
147-
}
148143

149144
for (int j = 0; j < contour.VertexCount - 1; j++)
150145
{
@@ -696,7 +691,6 @@ private static void DivideSegment(
696691
// Prevent from corner case 1
697692
if (p.X == le.Point.X && p.Y < le.Point.Y)
698693
{
699-
// TODO: enabling this line makes a single test issue76.geojson fail.
700694
// The files are different in the two reference repositories but both fail.
701695
p = new Vertex(p.X.NextAfter(double.PositiveInfinity), p.Y);
702696
}
@@ -1029,7 +1023,7 @@ private static int NextPos(
10291023
{
10301024
// Entire group is already processed?
10311025
found = false;
1032-
return Int32.MinValue;
1026+
return int.MinValue;
10331027
}
10341028

10351029
if (!processed[pos])

src/PolygonClipper/SegmentComparer.cs

Lines changed: 17 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
using System;
55
using System.Collections;
66
using System.Collections.Generic;
7+
using System.Runtime.CompilerServices;
78

89
namespace PolygonClipper;
910

@@ -34,19 +35,19 @@ public int Compare(SweepEvent? x, SweepEvent? y)
3435
}
3536

3637
SweepEvent perhapsInversedX, perhapsInversedY;
37-
Func<bool, int> lessIf;
38+
bool inversed = false;
3839

3940
if (x.IsBefore(y))
4041
{
4142
perhapsInversedX = x;
4243
perhapsInversedY = y;
43-
lessIf = LessIf;
44+
inversed = false;
4445
}
4546
else
4647
{
4748
perhapsInversedX = y;
4849
perhapsInversedY = x;
49-
lessIf = LessIfInversed;
50+
inversed = true;
5051
}
5152

5253
// Check if the segments are collinear by comparing their signed areas
@@ -59,26 +60,26 @@ public int Compare(SweepEvent? x, SweepEvent? y)
5960
// If they share their left endpoint, use the right endpoint to sort
6061
if (perhapsInversedX.Point == perhapsInversedY.Point)
6162
{
62-
return lessIf(perhapsInversedX.Below(perhapsInversedY.OtherEvent.Point));
63+
return LessIf(perhapsInversedX.Below(perhapsInversedY.OtherEvent.Point), inversed);
6364
}
6465

6566
// Different left endpoints: use the y-coordinate to sort if x-coordinates are the same
6667
if (perhapsInversedX.Point.X == perhapsInversedY.Point.X)
6768
{
68-
return lessIf(perhapsInversedX.Point.Y < perhapsInversedY.Point.Y);
69+
return LessIf(perhapsInversedX.Point.Y < perhapsInversedY.Point.Y, inversed);
6970
}
7071

7172
// If `x` and `y` lie on the same side of the reference segment,
7273
// no intersection check is necessary.
7374
if ((area1 > 0) == (area2 > 0))
7475
{
75-
return lessIf(area1 > 0);
76+
return LessIf(area1 > 0, inversed);
7677
}
7778

7879
// If `x` lies on the reference segment, compare based on `y`.
7980
if (area1 == 0)
8081
{
81-
return lessIf(area2 > 0);
82+
return LessIf(area2 > 0, inversed);
8283
}
8384

8485
// Form segments from the events.
@@ -91,17 +92,17 @@ public int Compare(SweepEvent? x, SweepEvent? y)
9192
if (interResult == 0)
9293
{
9394
// No unique intersection found: decide based on area1.
94-
return lessIf(area1 > 0);
95+
return LessIf(area1 > 0, inversed);
9596
}
9697
else if (interResult == 1)
9798
{
9899
// Unique intersection found.
99100
if (pi0 == y.Point)
100101
{
101-
return lessIf(area2 > 0);
102+
return LessIf(area2 > 0, inversed);
102103
}
103104

104-
return lessIf(area1 > 0);
105+
return LessIf(area1 > 0, inversed);
105106
}
106107

107108
// If interResult is neither 0 nor 1, fall through to collinear logic.
@@ -114,16 +115,16 @@ public int Compare(SweepEvent? x, SweepEvent? y)
114115
if (perhapsInversedX.Point == perhapsInversedY.Point)
115116
{
116117
// When left endpoints are identical, order by contour id.
117-
return lessIf(perhapsInversedX.ContourId < perhapsInversedY.ContourId);
118+
return LessIf(perhapsInversedX.ContourId < perhapsInversedY.ContourId, inversed);
118119
}
119120

120121
// If left endpoints differ, the Rust version simply returns "less" (i.e. the one inserted earlier).
121122
// Here we mimic that by always returning -1.
122-
return lessIf(true);
123+
return LessIf(true, inversed);
123124
}
124125

125126
// Segments are collinear but belong to different polygons.
126-
return lessIf(perhapsInversedX.PolygonType == PolygonType.Subject);
127+
return LessIf(perhapsInversedX.PolygonType == PolygonType.Subject, inversed);
127128
}
128129

129130
/// <inheritdoc/>
@@ -152,14 +153,8 @@ public int Compare(object? x, object? y)
152153
/// Returns -1 if the condition is true, 1 if false.
153154
/// </summary>
154155
/// <param name="condition">The boolean condition to evaluate.</param>
156+
/// <param name="inversed">Should the result be inversed.</param>
155157
/// <returns>-1 if condition is true, 1 if false.</returns>
156-
public static int LessIf(bool condition) => condition ? -1 : 1;
157-
158-
/// <summary>
159-
/// Converts a boolean comparison result to an inversed ordering value.
160-
/// Returns 1 if the condition is true, -1 if false.
161-
/// </summary>
162-
/// <param name="condition">The boolean condition to evaluate.</param>
163-
/// <returns>1 if condition is true, -1 if false.</returns>
164-
public static int LessIfInversed(bool condition) => condition ? 1 : -1;
158+
[MethodImpl(MethodImplOptions.AggressiveInlining)]
159+
private static int LessIf(bool condition, bool inversed = false) => condition ^ inversed ? -1 : 1;
165160
}

tests/PolygonClipper.Tests/GenericTestCases.cs

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -16,12 +16,9 @@ namespace PolygonClipper.Tests;
1616

1717
public class GenericTestCases
1818
{
19-
private readonly ITestOutputHelper _testOutputHelper;
19+
private readonly ITestOutputHelper testOutputHelper;
2020

21-
public GenericTestCases(ITestOutputHelper testOutputHelper)
22-
{
23-
_testOutputHelper = testOutputHelper;
24-
}
21+
public GenericTestCases(ITestOutputHelper testOutputHelper) => this.testOutputHelper = testOutputHelper;
2522

2623
public static IEnumerable<object[]> GetTestCases()
2724
=> TestData.Generic.GetFileNames().Select(x => new object[] { x });
@@ -73,7 +70,7 @@ public void GenericTestCase(string testCaseFile)
7370
for (int i = 0; i < expected.ContourCount; i++)
7471
{
7572
// We don't test for holes here as the reference tests do not do so.
76-
this._testOutputHelper.WriteLine($"Current Countour {i}");
73+
this.testOutputHelper.WriteLine($"Current Countour {i}");
7774

7875
Assert.Equal(expected[i].VertexCount, actual[i].VertexCount);
7976
for (int j = 0; j < expected[i].VertexCount; j++)
@@ -153,7 +150,8 @@ private static List<ExpectedResult> ExtractExpectedResults(List<Feature> feature
153150
{
154151
return new ExpectedResult
155152
{
156-
Operation = operation, Coordinates = ConvertToPolygon(feature.Geometry as GeoPolygon)
153+
Operation = operation,
154+
Coordinates = ConvertToPolygon(feature.Geometry as GeoPolygon)
157155
};
158156
}
159157

tests/PolygonClipper.Tests/SegmentComparerTests.cs

Lines changed: 27 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ public void NotCollinear_EventsOrderInSweepLine()
5454
Assert.False(se2.Below(se1.Point));
5555
Assert.True(se2.Above(se1.Point));
5656

57-
AssertOrder(se1,se2, Ordering.Less);
57+
AssertOrder(se1,se2, true);
5858

5959
Assert.Equal(1, eventComparer.Compare(se3, se4));
6060
Assert.False(se4.Above(se3.Point));
@@ -126,49 +126,47 @@ public void TShapedCases()
126126
// /\
127127
var (se1, _) = this.MakeSimple(0, 0.0, 0.0, 1.0, 1.0, true);
128128
var (se2, _) = this.MakeSimple(0, 0.5, 0.5, 1.0, 0.0, true);
129-
this.AssertOrder(se1, se2, Ordering.Greater);
129+
this.AssertOrder(se1, se2, false);
130130

131131
// shape: \/
132132
// \
133133
(se1, _) = this.MakeSimple(0, 0.0, 1.0, 1.0, 0.0, true);
134134
(se2, _) = this.MakeSimple(0, 0.5, 0.5, 1.0, 1.0, true);
135-
this.AssertOrder(se1, se2, Ordering.Less);
135+
this.AssertOrder(se1, se2, true);
136136

137137
// shape: T
138138
(se1, _) = this.MakeSimple(0, 0.0, 1.0, 1.0, 1.0, true);
139139
(se2, _) = this.MakeSimple(0, 0.5, 0.0, 0.5, 1.0, true);
140-
this.AssertOrder(se1, se2, Ordering.Greater);
140+
this.AssertOrder(se1, se2, false);
141141

142142
// shape: T upside down
143143
(se1, _) = this.MakeSimple(0, 0.0, 0.0, 1.0, 0.0, true);
144144
(se2, _) = this.MakeSimple(0, 0.5, 0.0, 0.5, 1.0, true);
145-
this.AssertOrder(se1, se2, Ordering.Less);
145+
this.AssertOrder(se1, se2, true);
146146
}
147147

148148
[Fact]
149149
public void VerticalSegment()
150150
{
151-
// Vertikales Referenzsegment bei x = 0, von y = -1 bis +1
151+
// Vertical Referencesegment at x = 0, from y = -1 to +1
152152
var (se1, _) = this.MakeSimple(0, 0.0, -1.0, 0.0, 1.0, true);
153153

154-
// "above" Fälle
155-
this.AssertOrder(se1, this.MakeSimple(0, -1.0, 1.0, 0.0, 1.0, true).se1, Ordering.Less);
156-
this.AssertOrder(se1, this.MakeSimple(0, 0.0, 1.0, 1.0, 1.0, true).se1, Ordering.Less);
157-
this.AssertOrder(se1, this.MakeSimple(0, -1.0, 2.0, 0.0, 2.0, true).se1, Ordering.Less);
158-
this.AssertOrder(se1, this.MakeSimple(0, 0.0, 2.0, 1.0, 2.0, true).se1, Ordering.Less);
159-
this.AssertOrder(se1, this.MakeSimple(0, 0.0, 1.0, 0.0, 2.0, true).se1, Ordering.Less);
160-
161-
// "below" Fälle
162-
this.AssertOrder(se1, this.MakeSimple(0, -1.0, -1.0, 0.0, -1.0, true).se1, Ordering.Greater);
163-
this.AssertOrder(se1, this.MakeSimple(0, 0.0, -1.0, 1.0, -1.0, true).se1, Ordering.Greater);
164-
this.AssertOrder(se1, this.MakeSimple(0, -1.0, -2.0, 0.0, -2.0, true).se1, Ordering.Greater);
165-
this.AssertOrder(se1, this.MakeSimple(0, 0.0, -2.0, 1.0, -2.0, true).se1, Ordering.Greater);
166-
this.AssertOrder(se1, this.MakeSimple(0, 0.0, -2.0, 0.0, -1.0, true).se1, Ordering.Greater);
167-
168-
// Überlappungen
169-
this.AssertOrder(se1, this.MakeSimple(0, 0.0, -0.5, 0.0, 0.5, true).se1, Ordering.Less);
170-
// Der folgende Fall ist im Rust-Test auskommentiert, da die Ordnung nicht anti-symmetrisch ist.
171-
// this.AssertOrder(se1, this.MakeSimple(0, 0.0, -1.0, 0.0, 0.0, true).se1, Ordering.Less);
154+
// "above" Cases
155+
this.AssertOrder(se1, this.MakeSimple(0, -1.0, 1.0, 0.0, 1.0, true).se1, true);
156+
this.AssertOrder(se1, this.MakeSimple(0, 0.0, 1.0, 1.0, 1.0, true).se1, true);
157+
this.AssertOrder(se1, this.MakeSimple(0, -1.0, 2.0, 0.0, 2.0, true).se1, true);
158+
this.AssertOrder(se1, this.MakeSimple(0, 0.0, 2.0, 1.0, 2.0, true).se1, true);
159+
this.AssertOrder(se1, this.MakeSimple(0, 0.0, 1.0, 0.0, 2.0, true).se1, true);
160+
161+
// "below" Cases
162+
this.AssertOrder(se1, this.MakeSimple(0, -1.0, -1.0, 0.0, -1.0, true).se1, false);
163+
this.AssertOrder(se1, this.MakeSimple(0, 0.0, -1.0, 1.0, -1.0, true).se1, false);
164+
this.AssertOrder(se1, this.MakeSimple(0, -1.0, -2.0, 0.0, -2.0, true).se1, false);
165+
this.AssertOrder(se1, this.MakeSimple(0, 0.0, -2.0, 1.0, -2.0, true).se1, false);
166+
this.AssertOrder(se1, this.MakeSimple(0, 0.0, -2.0, 0.0, -1.0, true).se1, false);
167+
168+
// Overlapping Cases
169+
this.AssertOrder(se1, this.MakeSimple(0, 0.0, -0.5, 0.0, 0.5, true).se1, true);
172170
}
173171

174172
private (SweepEvent se1, SweepEvent se2) MakeSimple(int contourId, double x1, double y1, double x2, double y2,
@@ -187,16 +185,11 @@ public void VerticalSegment()
187185
return (se1, se2);
188186
}
189187

190-
private void AssertOrder(SweepEvent se1, SweepEvent se2, Ordering order)
188+
private void AssertOrder(SweepEvent se1, SweepEvent se2, bool less)
191189
{
192-
Ordering inverseOrder = order == Ordering.Less ? Ordering.Greater : Ordering.Less;
193-
Assert.Equal((int)order, this.segmentComparer.Compare(se1, se2));
194-
Assert.Equal((int)inverseOrder, this.segmentComparer.Compare(se2, se1));
195-
}
196-
197-
public enum Ordering
198-
{
199-
Less = -1,
200-
Greater = 1,
190+
int order = less ? -1 : 1;
191+
int inverseOrder = less ? 1 : -1;
192+
Assert.Equal(order, this.segmentComparer.Compare(se1, se2));
193+
Assert.Equal(inverseOrder, this.segmentComparer.Compare(se2, se1));
201194
}
202195
}

0 commit comments

Comments
 (0)