Skip to content

clipper take 2 #323

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
magnesj wants to merge 5 commits into
base: dev
Choose a base branch
from
Open

clipper take 2 #323

magnesj wants to merge 5 commits into from

Conversation

@magnesj
Copy link
Owner

@magnesj magnesj commented Sep 2, 2025 •
edited by qodo-code-review bot
Loading

User description

Failing unit test, but testing with resinsight seems ok

PR Type

Enhancement

Description

  • Migrates from deprecated Clipper library to Clipper2

  • Updates polygon clipping operations with new API

  • Removes local Clipper dependency, uses vcpkg instead

  • Adds Z-coordinate handling limitations for some functions

Diagram Walkthrough

flowchart LR
  A["Old Clipper Library"] --> B["Clipper2 Library"]
  B --> C["Updated API Calls"]
  B --> D["vcpkg Dependency"]
  C --> E["Z-coordinate Limitations"]
Loading

File Walkthrough
Relevant files
Enhancement
1 files
RigCellGeometryTools.cpp
Update polygon clipping to Clipper2 API                                   
+72/-128
Tests
1 files
RigCellGeometryTools-Test.cpp
Update unit tests for Clipper2 compatibility                         
+32/-68 
Dependencies
2 files
CMakeLists.txt
Replace local Clipper with vcpkg Clipper2                               
+4/-7     
vcpkg.json
Add clipper2 dependency to vcpkg manifest                               
+1/-0     
Configuration changes
1 files
CMakeLists.txt
Link Clipper2 library to application                                         
+1/-0     
Additional files
4 files
autoformat_pep8.txt +0/-6     
CMakeLists.txt +0/-21   
clipper.cpp +0/-4629
clipper.hpp +0/-406 

magnesj and others added 3 commits September 2, 2025 08:41
@magnesj
Updates .gitignore and removes obsolete file
ab26d49
@magnesj
Migrates to Clipper2 library
501bea2 
Replaces the deprecated Clipper library with the actively maintained Clipper2 library.

This change improves code maintainability and takes advantage of the new features and performance enhancements offered by Clipper2. Includes changes to data types and function calls to align with the Clipper2 API.

Removes the local copy of the Clipper library and uses vcpkg to manage the dependency.

Note: Z coordinate handling for some functions is not yet fully supported in Clipper2 and require manual implementations.
@magnesj @github-actions
Fixes by cmake-format
67f8af2
@qodo-code-review
Copy link

qodo-code-review bot commented Sep 2, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Z-Handling Regression

The migration removes Z propagation/interpolation in clipping operations (e.g., polygon/line clipping). Functions now default Z to 0.0 or leave TODOs, which may change behavior and break callers or tests relying on Z interpolation (INTERPOLATE_LINE_Z / USE_HUGEVAL). Verify all call sites and update tests or reintroduce manual Z handling.

 
cvf::Vec3d fromClipperPoint( const Clipper2Lib::Point64& clipPoint, double zValue = 0.0 )
{
    return cvf::Vec3d( clipPoint.x / clipperConversionFactor, clipPoint.y / clipperConversionFactor, zValue );
}

//--------------------------------------------------------------------------------------------------
///
//--------------------------------------------------------------------------------------------------
std::vector<std::vector<cvf::Vec3d>>
    RigCellGeometryTools::intersectionWithPolygons( const std::vector<cvf::Vec3d>&              polygon1,
                                                    const std::vector<std::vector<cvf::Vec3d>>& polygonToIntersectWith )
{
    std::vector<std::vector<cvf::Vec3d>> clippedPolygons;

    // Convert to clipper2 format
    Clipper2Lib::Paths64 subject, clip;

    {
        Clipper2Lib::Path64 polygon1path;
        for ( const cvf::Vec3d& v : polygon1 )
        {
            polygon1path.push_back( toClipperPoint( v ) );
        }
        subject.push_back( polygon1path );
    }

    for ( const auto& path : polygonToIntersectWith )
    {
        Clipper2Lib::Path64 polygon2path;
        for ( const auto& v : path )
        {
            polygon2path.push_back( toClipperPoint( v ) );
        }
        clip.push_back( polygon2path );
    }

    Clipper2Lib::Paths64 solution = Clipper2Lib::Intersect( subject, clip, Clipper2Lib::FillRule::EvenOdd );

    // Convert back to std::vector<std::vector<cvf::Vec3d> >
    for ( const Clipper2Lib::Path64& pathInSol : solution )
    {
        std::vector<cvf::Vec3d> clippedPolygon;
        for ( const Clipper2Lib::Point64& IntPosition : pathInSol )
        {
            // Z value is lost in 2D clipping, set to 0
            clippedPolygon.push_back( fromClipperPoint( IntPosition, 0.0 ) );
        }
        clippedPolygons.push_back( clippedPolygon );
    }

    return clippedPolygons;
}

//--------------------------------------------------------------------------------------------------
///
//--------------------------------------------------------------------------------------------------
std::vector<std::vector<cvf::Vec3d>> RigCellGeometryTools::intersectionWithPolygon( const std::vector<cvf::Vec3d>& polygon1,
                                                                                    const std::vector<cvf::Vec3d>& polygon2 )
{
    return intersectionWithPolygons( polygon1, { polygon2 } );
}

//--------------------------------------------------------------------------------------------------
///
//--------------------------------------------------------------------------------------------------
std::vector<std::vector<cvf::Vec3d>> RigCellGeometryTools::subtractPolygons( const std::vector<cvf::Vec3d>& sourcePolygon,
                                                                             const std::vector<std::vector<cvf::Vec3d>>& polygonsToSubtract )
{
    Clipper2Lib::Paths64 subject, clip;

    {
        // Convert to clipper2 format
        Clipper2Lib::Path64 polygon1path;
        for ( const auto& v : sourcePolygon )
        {
            polygon1path.push_back( toClipperPoint( v ) );
        }
        subject.push_back( polygon1path );
    }

    for ( const auto& path : polygonsToSubtract )
    {
        Clipper2Lib::Path64 polygon2path;
        for ( const auto& v : path )
        {
            polygon2path.push_back( toClipperPoint( v ) );
        }
        clip.push_back( polygon2path );
    }

    Clipper2Lib::Paths64 solution = Clipper2Lib::Difference( subject, clip, Clipper2Lib::FillRule::EvenOdd );

    std::vector<std::vector<cvf::Vec3d>> clippedPolygons;

    // Convert back to std::vector<std::vector<cvf::Vec3d> >
    for ( const Clipper2Lib::Path64& pathInSol : solution )
    {
        std::vector<cvf::Vec3d> clippedPolygon;
        for ( const Clipper2Lib::Point64& IntPosition : pathInSol )
        {
            clippedPolygon.push_back( fromClipperPoint( IntPosition ) );
        }

        clippedPolygons.push_back( clippedPolygon );
    }

    return clippedPolygons;
}

std::vector<std::vector<cvf::Vec3d>> RigCellGeometryTools::subtractPolygon( const std::vector<cvf::Vec3d>& sourcePolygon,
                                                                            const std::vector<cvf::Vec3d>& polygonToSubtract )
{
    return subtractPolygons( sourcePolygon, { polygonToSubtract } );
}

//--------------------------------------------------------------------------------------------------
/// Note for cppcheck : First four parameter cannot be const to match the signature of the receiver
//--------------------------------------------------------------------------------------------------
// Note: Clipper2 doesn't support ZFillFunction like Clipper1
// Z interpolation needs to be implemented manually if needed
void fillInterpolatedSubjectZ( Clipper2Lib::Point64& e1bot,
                               Clipper2Lib::Point64& e1top,
                               Clipper2Lib::Point64& e2bot,
                               Clipper2Lib::Point64& e2top,
                               Clipper2Lib::Point64& pt )
{
    // Clipper2 Point64 doesn't have Z coordinate by default
    // This function is kept for compatibility but doesn't work without Z support
}

//--------------------------------------------------------------------------------------------------
///
//--------------------------------------------------------------------------------------------------
void fillUndefinedZ( Clipper2Lib::Point64& e1bot,
                     Clipper2Lib::Point64& e1top,
                     Clipper2Lib::Point64& e2bot,
                     Clipper2Lib::Point64& e2top,
                     Clipper2Lib::Point64& pt )
{
    // Clipper2 Point64 doesn't have Z coordinate by default
    // This function is kept for compatibility but doesn't work without Z support
}

//--------------------------------------------------------------------------------------------------
/// Assumes x.y plane polygon. Polyline might have a Z, and the returned Z is the polyline Z, interpolated if it is
/// clipped.
//--------------------------------------------------------------------------------------------------
std::vector<std::vector<cvf::Vec3d>> RigCellGeometryTools::clipPolylineByPolygon( const std::vector<cvf::Vec3d>& polyLine,
                                                                                  const std::vector<cvf::Vec3d>& polygon,
                                                                                  ZInterpolationType             interpolType )
{
    std::vector<std::vector<cvf::Vec3d>> clippedPolyline;

    // Adjusting polygon to avoid clipper issue with interpolating z-values when lines crosses though polygon vertecies
    std::vector<cvf::Vec3d> adjustedPolygon = ajustPolygonToAvoidIntersectionsAtVertex( polyLine, polygon );

    // Convert to clipper2 format
    Clipper2Lib::Paths64 subject, clip;

    Clipper2Lib::Path64 polyLinePath;
    for ( const cvf::Vec3d& v : polyLine )
    {
        polyLinePath.push_back( toClipperPoint( v ) );
    }
    subject.push_back( polyLinePath );

    Clipper2Lib::Path64 polygonPath;
    for ( const cvf::Vec3d& v : adjustedPolygon )
    {
        polygonPath.push_back( toClipperPoint( v ) );
    }
    clip.push_back( polygonPath );

    // Note: Clipper2 doesn't have ZFillFunction, so Z interpolation needs to be handled manually
    Clipper2Lib::Paths64 solution = Clipper2Lib::Intersect( subject, clip, Clipper2Lib::FillRule::EvenOdd );

    // TODO: Implement Z interpolation manually for interpolType
    Clipper2Lib::Paths64 solutionPaths = solution;

    // Convert back to std::vector<std::vector<cvf::Vec3d> >
    for ( const Clipper2Lib::Path64& pathInSol : solutionPaths )
    {
        std::vector<cvf::Vec3d> clippedPolygon;
        for ( const Clipper2Lib::Point64& IntPosition : pathInSol )
        {
            // TODO: Implement Z interpolation for interpolType
            clippedPolygon.push_back( fromClipperPoint( IntPosition, 0.0 ) );
        }
Polyline Intersect Semantics

Clipper2 Intersect is used for polylines with closed polygon clips, but open-path handling differs from Clipper1 (PolyTree/OpenPathsFromPolyTree no longer used). Ensure the result preserves open paths and segmentation expected by consumers; otherwise, adapt to Clipper2’s open path API or post-process accordingly.

 
std::vector<std::vector<cvf::Vec3d>> RigCellGeometryTools::clipPolylineByPolygon( const std::vector<cvf::Vec3d>& polyLine,
                                                                                  const std::vector<cvf::Vec3d>& polygon,
                                                                                  ZInterpolationType             interpolType )
{
    std::vector<std::vector<cvf::Vec3d>> clippedPolyline;

    // Adjusting polygon to avoid clipper issue with interpolating z-values when lines crosses though polygon vertecies
    std::vector<cvf::Vec3d> adjustedPolygon = ajustPolygonToAvoidIntersectionsAtVertex( polyLine, polygon );

    // Convert to clipper2 format
    Clipper2Lib::Paths64 subject, clip;

    Clipper2Lib::Path64 polyLinePath;
    for ( const cvf::Vec3d& v : polyLine )
    {
        polyLinePath.push_back( toClipperPoint( v ) );
    }
    subject.push_back( polyLinePath );

    Clipper2Lib::Path64 polygonPath;
    for ( const cvf::Vec3d& v : adjustedPolygon )
    {
        polygonPath.push_back( toClipperPoint( v ) );
    }
    clip.push_back( polygonPath );

    // Note: Clipper2 doesn't have ZFillFunction, so Z interpolation needs to be handled manually
    Clipper2Lib::Paths64 solution = Clipper2Lib::Intersect( subject, clip, Clipper2Lib::FillRule::EvenOdd );

    // TODO: Implement Z interpolation manually for interpolType
    Clipper2Lib::Paths64 solutionPaths = solution;

    // Convert back to std::vector<std::vector<cvf::Vec3d> >
    for ( const Clipper2Lib::Path64& pathInSol : solutionPaths )
    {
        std::vector<cvf::Vec3d> clippedPolygon;
        for ( const Clipper2Lib::Point64& IntPosition : pathInSol )
        {
            // TODO: Implement Z interpolation for interpolType
            clippedPolygon.push_back( fromClipperPoint( IntPosition, 0.0 ) );
        }
        clippedPolyline.push_back( clippedPolygon );
    }
Scaling/Precision

The conversion uses a fixed factor (10000) and int64. With Clipper2, large coordinates or accumulated operations could overflow or lose precision. Confirm dataset ranges and consider using Scale/Unscale utilities or double-precision PathsD in Clipper2 if appropriate.

 
double clipperConversionFactor = 10000; // For transform to clipper int

Clipper2Lib::Point64 toClipperPoint( const cvf::Vec3d& cvfPoint )
{
    int64_t xInt = cvfPoint.x() * clipperConversionFactor;
    int64_t yInt = cvfPoint.y() * clipperConversionFactor;

    return Clipper2Lib::Point64( xInt, yInt );
}

cvf::Vec3d fromClipperPoint( const Clipper2Lib::Point64& clipPoint, double zValue = 0.0 )
{
    return cvf::Vec3d( clipPoint.x / clipperConversionFactor, clipPoint.y / clipperConversionFactor, zValue );
}

@qodo-code-review
Copy link

qodo-code-review bot commented Sep 2, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Restore Z and open-path clipping

The migration dropped Z propagation and open polyline clipping, but several
functions (e.g., clipPolylineByPolygon) and tests rely on Z interpolation and
treating the subject as an open path. Clipper2 supports both via Z-enabled point
types and Z callbacks (or you must manually interpolate Z) and dedicated APIs
for open paths; using the generic Intersect with closed Paths64 will yield
incorrect results. Re-implement these with the correct Clipper2 open-path API
and Z handling (or a robust manual interpolation), so outputs preserve Z
semantics and unit tests pass.

Examples:

ApplicationLibCode/ReservoirDataModel/RigCellGeometryTools.cpp [519-564] 
std::vector<std::vector<cvf::Vec3d>> RigCellGeometryTools::clipPolylineByPolygon( const std::vector<cvf::Vec3d>& polyLine,
                                                                                  const std::vector<cvf::Vec3d>& polygon,
                                                                                  ZInterpolationType             interpolType )
{
    std::vector<std::vector<cvf::Vec3d>> clippedPolyline;

    // Adjusting polygon to avoid clipper issue with interpolating z-values when lines crosses though polygon vertecies
    std::vector<cvf::Vec3d> adjustedPolygon = ajustPolygonToAvoidIntersectionsAtVertex( polyLine, polygon );

    // Convert to clipper2 format

 ... (clipped 36 lines)
ApplicationLibCode/UnitTests/RigCellGeometryTools-Test.cpp [602-653] 
TEST( RigCellGeometryTools, ClipperEdgeTracking )
{
    // If the first edges of the polygons are horizontal, the edge tracking will fail.
    // Encode polygon and edge into Z coordinate of the vertex

    std::vector<cvf::Vec3d> polygon1 = { { 0.0, 0.51, 1002.0 }, { 1.0, 0.5, 1000.0 }, { 0.0, 1.0, 1001.0 } };
    std::vector<cvf::Vec3d> polygon2 = { { 0.5, 0.01, 2002.0 }, { 1.0, 0.0, 2000.0 }, { 0.5, 1.0, 2001.0 } };

    std::vector<std::vector<cvf::Vec3d>> clippedPolygons;


 ... (clipped 42 lines)

Solution Walkthrough:

Before:

std::vector<std::vector<cvf::Vec3d>> RigCellGeometryTools::clipPolylineByPolygon(...)
{
    // ...
    // Convert to clipper2 format, losing Z-coordinates
    Clipper2Lib::Paths64 subject, clip;
    Clipper2Lib::Path64 polyLinePath;
    for (const cvf::Vec3d& v : polyLine)
    {
        polyLinePath.push_back(toClipperPoint(v)); // toClipperPoint drops Z
    }
    subject.push_back(polyLinePath);
    // ... polygonPath is created ...
    clip.push_back(polygonPath);

    // Note: Clipper2 doesn't have ZFillFunction...
    // Incorrectly uses Intersect for a polyline, treating it as a closed polygon.
    Clipper2Lib::Paths64 solution = Clipper2Lib::Intersect(subject, clip, ...);
    
    // TODO: Implement Z interpolation manually for interpolType
    for (const auto& pathInSol : solution) {
        // ...
        clippedPolygon.push_back(fromClipperPoint(IntPosition, 0.0)); // Z is hardcoded to 0.0
    }
    return clippedPolyline;
}

After:

std::vector<std::vector<cvf::Vec3d>> RigCellGeometryTools::clipPolylineByPolygon(...)
{
    // ...
    // Keep Z-values for manual interpolation
    Clipper2Lib::Path64 polyLinePath;
    std::vector<double> polyLineZValues;
    for (const cvf::Vec3d& v : polyLine) {
        polyLinePath.push_back(toClipperPoint(v));
        polyLineZValues.push_back(v.z());
    }
    
    // Use the correct Clipper2 API for open paths
    Clipper2Lib::Clipper64 clpr;
    clpr.AddSubjectOpen(polyLinePath);
    clpr.AddClip(polygonPath);

    Clipper2Lib::Paths64 solutionOpen;
    Clipper2Lib::PolyTree64 solutionClosed;
    clpr.Execute(ClipType::Intersection, FillRule::EvenOdd, solutionClosed, solutionOpen);

    // Convert back, with manual Z interpolation for new vertices
    for (const auto& pathInSol : solutionOpen) {
        // ...
        // double interpolated_z = getInterpolatedZ(...);
        // clippedSegment.push_back(fromClipperPoint(point, interpolated_z));
    }
    return clippedPolyline;
}
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a critical functional regression where Z-coordinate handling and open-path clipping were dropped, which is confirmed by TODO comments and the PR author's note about failing tests.

High
Possible issue
Implement Z coordinate interpolation functionality

The Z interpolation functionality has been lost in the migration from Clipper1
to Clipper2. The functions fillInterpolatedSubjectZ and fillUndefinedZ are now
empty stubs, and Z values are hardcoded to 0.0. This will break existing
functionality that depends on Z coordinate interpolation during clipping
operations.

ApplicationLibCode/ReservoirDataModel/RigCellGeometryTools.cpp [372-375]

 cvf::Vec3d fromClipperPoint( const Clipper2Lib::Point64& clipPoint, double zValue = 0.0 )
 {
     return cvf::Vec3d( clipPoint.x / clipperConversionFactor, clipPoint.y / clipperConversionFactor, zValue );
 }
 
+// TODO: Implement Z interpolation manually to replace Clipper1's ZFillFunction
+// Store original Z values in a separate data structure and interpolate during conversion
+
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a critical functional regression where Z-coordinate interpolation is lost due to the migration to Clipper2, which is a major issue.

High
Handle Z interpolation parameter properly

The clipPolylineByPolygon function ignores the interpolType parameter
completely, which breaks the contract of the function. The Z interpolation logic
needs to be implemented manually since Clipper2 doesn't support the
ZFillFunction callback mechanism.

ApplicationLibCode/ReservoirDataModel/RigCellGeometryTools.cpp [519-549]

 std::vector<std::vector<cvf::Vec3d>> RigCellGeometryTools::clipPolylineByPolygon( const std::vector<cvf::Vec3d>& polyLine,
                                                                                   const std::vector<cvf::Vec3d>& polygon,
                                                                                   ZInterpolationType             interpolType )
 {
     // ...
-    // Note: Clipper2 doesn't have ZFillFunction, so Z interpolation needs to be handled manually
     Clipper2Lib::Paths64 solution = Clipper2Lib::Intersect( subject, clip, Clipper2Lib::FillRule::EvenOdd );
     
-    // TODO: Implement Z interpolation manually for interpolType
-    Clipper2Lib::Paths64 solutionPaths = solution;
+    // Implement Z interpolation based on interpolType
+    for ( const Clipper2Lib::Path64& pathInSol : solution )
+    {
+        std::vector<cvf::Vec3d> clippedPolygon;
+        for ( const Clipper2Lib::Point64& IntPosition : pathInSol )
+        {
+            double zValue = 0.0;
+            if (interpolType == INTERPOLATE_LINE_Z) {
+                // TODO: Implement Z interpolation from original polyLine
+                zValue = interpolateZFromPolyline(IntPosition, polyLine);
+            } else if (interpolType == USE_HUGEVAL) {
+                zValue = HUGE_VAL;
+            }
+            clippedPolygon.push_back( fromClipperPoint( IntPosition, zValue ) );
+        }
+        clippedPolyline.push_back( clippedPolygon );
+    }
 }

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 9

__

Why: The suggestion correctly points out that the interpolType parameter is ignored, breaking the function's contract and causing a functional regression in Z-value handling, which is a critical issue.

High
General
Fix non-functional test callback

The test function clipperEdgeIntersectCallback is now a no-op that only prints a
message, which means the test ClipperEdgeTracking will not function as intended.
This could lead to false test results and mask potential issues with edge
tracking functionality.

ApplicationLibCode/UnitTests/RigCellGeometryTools-Test.cpp [591-600]

 void clipperEdgeIntersectCallback( Clipper2Lib::Point64& e1bot,
                                    Clipper2Lib::Point64& e1top,
                                    Clipper2Lib::Point64& e2bot,
                                    Clipper2Lib::Point64& e2top,
                                    Clipper2Lib::Point64& pt )
 {
-    // Clipper2 Point64 doesn't have Z coordinate by default
-    // This function is kept for compatibility but doesn't work without Z support
-    std::cout << "Edge intersection callback - Z handling not supported in Clipper2" << std::endl;
+    // TODO: Implement alternative edge tracking mechanism for Clipper2
+    // or disable this test until proper implementation is available
+    FAIL() << "Edge intersection callback not implemented for Clipper2";
 }
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a test that no longer performs its intended check due to the API change, and proposes to make it fail explicitly, which is crucial for maintaining test suite integrity.

Medium
  • More

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Review effort 3/5

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@magnesj