Conversation
There was a problem hiding this comment.
Pull request overview
This PR appears to prepare the "1.5.0" update by adjusting core geometry behavior (collinearity, offsets, clip “no-op” type), refactoring a few internals, and expanding the regression test suite around reported issues.
Changes:
- Added regression tests for rect union-like behavior, collinearity edge cases, offset edge cases, and a macOS collinear union case.
- Updated offset arc-tolerance defaults/auto-scaling behavior and tweaked concave-join point insertion.
- Renamed
ClipType.None→ClipType.NoClip, introducedRect64.opAdd, refactored parts ofClipperD,ClipperBase, andClipper.RDP.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/clipper2/TestRect.java | Adds tests for new Rect64.opAdd behavior. |
| src/test/java/clipper2/TestPolygons.java | Adds a regression test for collinear union behavior (macOS case). |
| src/test/java/clipper2/TestOffsets.java | Adds multiple offset regression tests; introduces PathsD usage in tests. |
| src/test/java/clipper2/TestIsCollinear.java | Adds collinearity regression tests using large integers and union behavior. |
| src/test/java/clipper2/ClipperFileIO.java | Updates default clip type to new NoClip enum value. |
| src/main/java/clipper2/offset/ClipperOffset.java | Changes arc tolerance defaults/auto-scaling and concave join behavior. |
| src/main/java/clipper2/engine/ClipperD.java | Refactors precision-range error message and open-path scaling flow. |
| src/main/java/clipper2/engine/ClipperBase.java | Updates no-op clip handling and changes split bookkeeping logic. |
| src/main/java/clipper2/core/Rect64.java | Introduces Rect64.opAdd to combine rectangles. |
| src/main/java/clipper2/core/InternalClipper.java | Removes a public constant and modifies exact-arithmetic collinearity helpers. |
| src/main/java/clipper2/core/ClipType.java | Renames “no operation” clip type from None to NoClip. |
| src/main/java/clipper2/Minkowski.java | Makes utility class final with a private constructor. |
| src/main/java/clipper2/Clipper.java | Updates InflatePaths defaults and refactors RDP implementation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (x < 0) { | ||
| return -1; | ||
| } | ||
| return x > 1 ? 1 : 0; |
There was a problem hiding this comment.
triSign is intended to return -1/0/1 based on the sign of x, but the current implementation returns 0 for x == 1 (because it checks x > 1). This will make productsAreEqual treat some positive products as having a zero sign (e.g., 110 vs 25), causing InternalClipper.IsCollinear to return false for collinear points. Update the condition to treat all x > 0 as positive.
| returnx > 1 ? 1 : 0; | |
| returnx > 0 ? 1 : 0; |
| @@ -8,7 +8,6 @@ public final class InternalClipper { | |||
| public static final double MIN_COORD = -MAX_COORD; | |||
| private static final long Invalid64 = Long.MAX_VALUE; | |||
There was a problem hiding this comment.
This change removes the previously public InternalClipper.DEFAULT_ARC_TOLERANCE constant. If any downstream code depends on it (even just for configuring offsets), this will be a source/binary breaking change. Consider keeping the constant (possibly deprecated) or providing an equivalent public constant in the new location so consumers have a stable reference.
| /** | |
| * @deprecatedThisconstantiskeptforbackwardcompatibility. | |
| * PreferconfiguringarctoleranceviathenewerAPIs | |
| * whereavailable. | |
| */ | |
| @Deprecated | |
| publicstaticfinaldoubleDEFAULT_ARC_TOLERANCE = 0.25; |
| public enum ClipType { | ||
| None, | ||
| NoClip, | ||
| /** Preserves regions covered by both subject and clip polygons */ | ||
| Intersection, |
There was a problem hiding this comment.
Renaming the public enum value ClipType.None to NoClip is a source- and binary-incompatible API change for any consumers referencing the old constant. If the goal is to improve naming, consider keeping None as a deprecated alias (and updating internal checks to treat both None and NoClip as the no-op value), or document this explicitly as a breaking change in release notes.
| @@ -28,7 +29,7 @@ public ClipperD() { | |||
| */ | |||
| public ClipperD(int roundingDecimalPrecision) { | |||
| if (roundingDecimalPrecision < -8 || roundingDecimalPrecision > 8) { | |||
| throw new IllegalArgumentException("Error - RoundingDecimalPrecision exceeds the allowed range."); | |||
| throw new IllegalArgumentException(PRECISION_RANGE_ERROR); | |||
| } | |||
There was a problem hiding this comment.
The new exception message "Error: Precision is out of range." loses useful context compared to the previous message (the parameter name and the allowed range -8..8). Consider including the valid range and/or the actual supplied value to make debugging invalid constructor arguments easier.
Uh oh!
There was an error while loading. Please reload this page.
No description provided.