-
Notifications
You must be signed in to change notification settings - Fork 389
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.
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
Fix #10656 - PolygonClipping: CLIPLINE produces NaN when line to clip is parallel to the maxX of the clip window #10657
Conversation
Use the Liang-Barsky algorithm * Loosely adapted from https://en.wikipedia.org/wiki/Liang%E2%80%93Barsky_algorithm * Rewrote it until performance is optimal I used microbenchmarking: https://github.com/jmarrec/CppBenchmarks/blob/94f0e1594ff17f9833a0547f0725d68c561311a1/bench_clipline.cpp BM_CLIPLINE is the current E+ algorithm. BM_liang_barsky_clipper3 is my final version that I used here, and which is 15% faster than the original while being actually correct. in the edge cases ``` ------------------------------------------------------------------- Benchmark Time CPU Iterations ------------------------------------------------------------------- BM_CLIPLINE 567 ns 566 ns 1231826 BM_Clip2 703 ns 703 ns 991672 BM_liang_barsky_clipper 588 ns 588 ns 1199750 BM_liang_barsky_clipper2 490 ns 490 ns 1430600 BM_liang_barsky_clipper3 482 ns 482 ns 145236 ```
TEST_F(EnergyPlusFixture, CLIPLINE_Throw) | ||
{ | ||
Real64 constexpr minX = 2.0; | ||
Real64 constexpr maxX = 8.0; | ||
Real64 constexpr minY = 3.0; | ||
Real64 constexpr maxY = 6.0; | ||
|
||
Real64 x0 = maxX; | ||
Real64 x1 = maxX; | ||
Real64 y0 = 4.5; | ||
Real64 y1 = 1.0; | ||
bool visible = false; | ||
|
||
EXPECT_NO_THROW(CLIPLINE(x0, x1, y0, y1, maxX, minX, maxY, minY, visible)); | ||
|
||
EXPECT_DOUBLE_EQ(maxX, x0); | ||
EXPECT_DOUBLE_EQ(4.5, y0); | ||
EXPECT_DOUBLE_EQ(maxX, x1); | ||
EXPECT_DOUBLE_EQ(minY, y1); // This is NaN | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simple test, was enough to produce a NaN for #10656
// From 7 to 5 | ||
TestCase{Line{Point{center_x, greater_y}, Point{greater_x, center_y}}, false}, | ||
// From 7 to 8 | ||
TestCase{Line{Point{center_x, greater_y}, Point{greater_x, greater_y}}, false}, | ||
// From 2 to 0 | ||
TestCase{Line{Point{greater_x, below_y}, Point{below_x, below_y}}, false}, | ||
// From 2 to 3 | ||
TestCase{ | ||
Line{Point{greater_x, below_y}, Point{below_x, center_y}}, true, Line{Point{4.2857142857142865, minY}, Point{minX, 3.8000000000000003}}}, | ||
// From 2 to 6 | ||
TestCase{Line{Point{greater_x, below_y}, Point{below_x, greater_y}}, true, Line{Point{7.5, minY}, Point{3.75, maxY}}}, | ||
// From 2 to 1 | ||
TestCase{Line{Point{greater_x, below_y}, Point{center_x, below_y}}, false}, | ||
// From 2 to 4 | ||
TestCase{Line{Point{greater_x, below_y}, Point{center_x, center_y}}, true, Line{Point{7.142857142857143, minY}, Point{center_x, center_y}}}, | ||
// From 2 to 7 | ||
TestCase{Line{Point{greater_x, below_y}, Point{center_x, greater_y}}, true, Line{Point{maxX, 4.2}, Point{6.875, maxY}}}, | ||
// From 2 to 5 | ||
TestCase{Line{Point{greater_x, below_y}, Point{greater_x, center_y}}, false}, | ||
// From 2 to 8 | ||
TestCase{Line{Point{greater_x, below_y}, Point{greater_x, greater_y}}, false}, | ||
// From 5 to 0 | ||
TestCase{Line{Point{greater_x, center_y}, Point{below_x, below_y}}, true, Line{Point{maxX, 3.8}, Point{5.714285714285714, minY}}}, | ||
// From 5 to 3 | ||
TestCase{Line{Point{greater_x, center_y}, Point{below_x, center_y}}, true, Line{Point{maxX, center_y}, Point{minX, center_y}}}, | ||
// From 5 to 6 | ||
TestCase{Line{Point{greater_x, center_y}, Point{below_x, greater_y}}, true, Line{Point{maxX, 5.4}, Point{6.666666666666667, maxY}}}, | ||
// From 5 to 1 | ||
TestCase{ | ||
Line{Point{greater_x, center_y}, Point{center_x, below_y}}, true, Line{Point{maxX, 3.0999999999999996}, Point{7.857142857142858, minY}}}, | ||
// From 5 to 4 | ||
TestCase{Line{Point{greater_x, center_y}, Point{center_x, center_y}}, true, Line{Point{maxX, center_y}, Point{center_x, center_y}}}, | ||
// From 5 to 7 | ||
TestCase{Line{Point{greater_x, center_y}, Point{center_x, greater_y}}, false}, | ||
// From 5 to 2 | ||
TestCase{Line{Point{greater_x, center_y}, Point{greater_x, below_y}}, false}, | ||
// From 5 to 8 | ||
TestCase{Line{Point{greater_x, center_y}, Point{greater_x, greater_y}}, false}, | ||
// From 8 to 0 | ||
TestCase{Line{Point{greater_x, greater_y}, Point{below_x, below_y}}, true, Line{Point{6.25, maxY}, Point{2.5, minY}}}, | ||
// From 8 to 3 | ||
TestCase{Line{Point{greater_x, greater_y}, Point{below_x, center_y}}, true, Line{Point{3.333333333333334, maxY}, Point{minX, 5.4}}}, | ||
// From 8 to 6 | ||
TestCase{Line{Point{greater_x, greater_y}, Point{below_x, greater_y}}, false}, | ||
// From 8 to 1 | ||
TestCase{Line{Point{greater_x, greater_y}, Point{center_x, below_y}}, true, Line{Point{maxX, 5.8}, Point{6.25, minY}}}, | ||
// From 8 to 4 | ||
TestCase{Line{Point{greater_x, greater_y}, Point{center_x, center_y}}, true, Line{Point{6.666666666666667, maxY}, Point{center_x, center_y}}}, | ||
// From 8 to 7 | ||
TestCase{Line{Point{greater_x, greater_y}, Point{center_x, greater_y}}, false}, | ||
// From 8 to 2 | ||
TestCase{Line{Point{greater_x, greater_y}, Point{greater_x, below_y}}, false}, | ||
// From 8 to 5 | ||
TestCase{Line{Point{greater_x, greater_y}, Point{greater_x, center_y}}, false}, | ||
}}; | ||
|
||
size_t i = 0; | ||
for (const auto &t : test_cases) { | ||
++i; | ||
std::string const msg = | ||
fmt::format("test_case {}: From ({}, {}) to ({}, {})", i, t.line_ori.p0.x, t.line_ori.p0.y, t.line_ori.p1.x, t.line_ori.p1.y); | ||
SCOPED_TRACE(msg); | ||
testclipline(t); | ||
} | ||
|
||
constexpr std::array<TestCase, 24> boundary_lines{ | ||
TestCase{Line{Point{minX, below_y}, Point{minX, center_y}}, true, Line{Point{minX, minY}, Point{minX, center_y}}}, | ||
TestCase{Line{Point{minX, below_y}, Point{minX, greater_y}}, true, Line{Point{minX, minY}, Point{minX, maxY}}}, | ||
TestCase{Line{Point{minX, center_y}, Point{minX, below_y}}, true, Line{Point{minX, center_y}, Point{minX, minY}}}, | ||
TestCase{Line{Point{minX, center_y}, Point{minX, greater_y}}, true, Line{Point{minX, center_y}, Point{minX, maxY}}}, | ||
TestCase{Line{Point{minX, greater_y}, Point{minX, below_y}}, true, Line{Point{minX, maxY}, Point{minX, minY}}}, | ||
TestCase{Line{Point{minX, greater_y}, Point{minX, center_y}}, true, Line{Point{minX, maxY}, Point{minX, center_y}}}, | ||
TestCase{Line{Point{maxX, below_y}, Point{maxX, center_y}}, true, Line{Point{maxX, minY}, Point{maxX, center_y}}}, | ||
TestCase{Line{Point{maxX, below_y}, Point{maxX, greater_y}}, true, Line{Point{maxX, minY}, Point{maxX, maxY}}}, | ||
TestCase{Line{Point{maxX, center_y}, Point{maxX, below_y}}, true, Line{Point{maxX, center_y}, Point{maxX, minY}}}, | ||
TestCase{Line{Point{maxX, center_y}, Point{maxX, greater_y}}, true, Line{Point{maxX, center_y}, Point{maxX, maxY}}}, | ||
TestCase{Line{Point{maxX, greater_y}, Point{maxX, below_y}}, true, Line{Point{maxX, maxY}, Point{maxX, minY}}}, | ||
TestCase{Line{Point{maxX, greater_y}, Point{maxX, center_y}}, true, Line{Point{maxX, maxY}, Point{maxX, center_y}}}, | ||
TestCase{Line{Point{below_x, minY}, Point{center_x, minY}}, true, Line{Point{minX, minY}, Point{center_x, minY}}}, | ||
TestCase{Line{Point{below_x, minY}, Point{greater_x, minY}}, true, Line{Point{minX, minY}, Point{maxX, minY}}}, | ||
TestCase{Line{Point{center_x, minY}, Point{below_x, minY}}, true, Line{Point{center_x, minY}, Point{minX, minY}}}, | ||
TestCase{Line{Point{center_x, minY}, Point{greater_x, minY}}, true, Line{Point{center_x, minY}, Point{maxX, minY}}}, | ||
TestCase{Line{Point{greater_x, minY}, Point{below_x, minY}}, true, Line{Point{maxX, minY}, Point{minX, minY}}}, | ||
TestCase{Line{Point{greater_x, minY}, Point{center_x, minY}}, true, Line{Point{maxX, minY}, Point{center_x, minY}}}, | ||
TestCase{Line{Point{below_x, maxY}, Point{center_x, maxY}}, true, Line{Point{minX, maxY}, Point{center_x, maxY}}}, | ||
TestCase{Line{Point{below_x, maxY}, Point{greater_x, maxY}}, true, Line{Point{minX, maxY}, Point{maxX, maxY}}}, | ||
TestCase{Line{Point{center_x, maxY}, Point{below_x, maxY}}, true, Line{Point{center_x, maxY}, Point{minX, maxY}}}, | ||
TestCase{Line{Point{center_x, maxY}, Point{greater_x, maxY}}, true, Line{Point{center_x, maxY}, Point{maxX, maxY}}}, | ||
TestCase{Line{Point{greater_x, maxY}, Point{below_x, maxY}}, true, Line{Point{maxX, maxY}, Point{minX, maxY}}}, | ||
TestCase{Line{Point{greater_x, maxY}, Point{center_x, maxY}}, true, Line{Point{maxX, maxY}, Point{center_x, maxY}}}, | ||
}; | ||
i = 0; | ||
for (const auto &t : boundary_lines) { | ||
++i; | ||
std::string const msg = | ||
fmt::format("Boundary Line {}: From ({}, {}) to ({}, {})", i, t.line_ori.p0.x, t.line_ori.p0.y, t.line_ori.p1.x, t.line_ori.p1.y); | ||
SCOPED_TRACE(msg); | ||
testclipline(t); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bunch more testing to ensure whatever algorithm I ended up with was actually correct.
I test from from one zone to another
// We break the plane into 9 parts, where region (4) is the clipping region
//
// 6 │ 7 │ 8
// ───────────
// 3 │ 4 │ 5
// ───────────
// 0 │ 1 │ 2
And I also test on boundary lines, where the line is parallel (and on) the boundary of the clip window
@@ -136,6 +136,8 @@ namespace SolarShading { | |||
|
|||
void CLIP(EnergyPlusData &state, int const NVT, Array1D<Real64> &XVT, Array1D<Real64> &YVT, Array1D<Real64> &ZVT); | |||
|
|||
void CLIPLINE(Real64 &x0, Real64 &x1, Real64 &y0, Real64 &y1, Real64 maxX, Real64 minX, Real64 maxY, Real64 minY, bool &visible); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I expose it so I can test it. I'm also handling the rev inside the routine, makes no sense to leave the caller to deal with it
Note: in the regular usage, we do nothing is visible is false, and we pass copies into the CLIPLINE method anyways, so there's no point reversing it back
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll mark it approved but actually I will remove the SCOPED_TRACE if that actually adds outputs during the test runs.
@@ -3868,162 +3869,99 @@ inline bool d_eq(Real64 a, Real64 b) | |||
return std::abs(a - b) < 2.0; | |||
} | |||
|
|||
void CLIPLINE(Real64 &x1, Real64 &x2, Real64 &y1, Real64 &y2, Real64 maxX, Real64 minX, Real64 maxY, Real64 minY, bool &visible, bool &rev) | |||
void CLIPLINE(Real64 &x0, Real64 &x1, Real64 &y0, Real64 &y1, Real64 maxX, Real64 minX, Real64 maxY, Real64 minY, bool &visible) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fantastic. So much cleaner!
++i; | ||
std::string const msg = | ||
fmt::format("test_case {}: From ({}, {}) to ({}, {})", i, t.line_ori.p0.x, t.line_ori.p0.y, t.line_ori.p1.x, t.line_ori.p1.y); | ||
SCOPED_TRACE(msg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this always emit the current position? I will see it locally if so, just asking to remind myself to look I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SCOPED_TRACE will emit everytime another assertion fails. So it emits nothing if the test passes, but if it something goes wrong in the lambda with the EXPECT statements, you know which iteration of the loop triggered it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, that's great. I thought it was just emitting info by calling the macro itself. I now see it is just registering the extra data to report in case of a failure. This is great, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, let's go for it.
++i; | ||
std::string const msg = | ||
fmt::format("test_case {}: From ({}, {}) to ({}, {})", i, t.line_ori.p0.x, t.line_ori.p0.y, t.line_ori.p1.x, t.line_ori.p1.y); | ||
SCOPED_TRACE(msg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, that's great. I thought it was just emitting info by calling the macro itself. I now see it is just registering the extra data to report in case of a failure. This is great, thanks.
Pull request overview
Use the Liang-Barsky algorithm
I used microbenchmarking: https://github.com/jmarrec/CppBenchmarks/blob/94f0e1594ff17f9833a0547f0725d68c561311a1/bench_clipline.cpp
BM_CLIPLINE is the current E+ algorithm. BM_liang_barsky_clipper3 is my final version that I used here, and which is 15% faster than the original while being actually correct. in the edge cases
Pull Request Author
Add to this list or remove from it as applicable. This is a simple templated set of guidelines.
Reviewer
This will not be exhaustively relevant to every PR.