-
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 #10103 - CheckConvexity removes colinear vertices but stops when surface is determined non-convex, causing vertex size mismatch fatal error #10114
Conversation
``` [ RUN ] EnergyPlusFixture.SurfaceGeometry_CheckConvexity_ColinearStability /home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/SurfaceGeometry.unit.cc:3331: Failure Expected equality of these values: 6 floorSurface.Sides Which is: 8 /home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/SurfaceGeometry.unit.cc:3350: Failure Expected equality of these values: 6 ceilingSurface.Sides Which is: 9 /home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/SurfaceGeometry.unit.cc:3354: Failure Expected equality of these values: floorSurface.Sides Which is: 8 ceilingSurface.Sides Which is: 9 ```
…surface is determined non-convex, causing vertex size mismatch fatal error
if (state.dataGlobal->DisplayExtraWarnings && surfaceTmp.ExtSolar && | ||
(state.dataHeatBal->SolarDistribution != DataHeatBalance::Shadowing::Minimal) && | ||
// Warn only once | ||
surfaceTmp.IsConvex) { | ||
ShowWarningError(state, | ||
format("CheckConvexity: Zone=\"{}\", Surface=\"{}\" is non-convex.", |
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.
When warning, do it only once (when surfaceTmp.IsConvex, before that's set to false)
surfaceTmp.IsConvex = false; | ||
break; | ||
// #10103 - We do not want to break early, because we do want to consistently remove colinear vertices | ||
// to avoid potential vertex size mismatch fatal errors | ||
// break; |
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.
And do NOT break, so we ensure we do drop all colinear vertices and do not end up with a vertex size mistmatch.
// Test for #10103 - Regardless of the order of the vertices, we should drop colinear vertices consistently to avoid a fatal error due to vertex | ||
// side mismatch | ||
// | ||
// y | ||
// ▲ 7 8 9 | ||
// │ ┌─────x────┐ | ||
// │ │ │ | ||
// │ │ │ | ||
// │ │ │ | ||
// │5 │ │ | ||
// ├──────────┘ │ | ||
// │ 6 │ | ||
// │ │ | ||
// │ │ | ||
// │ │10 | ||
// └────x─────x─────x────┴─────► | ||
// 4 3 2 1 x |
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.
Unit test case. We expect 1, 2, 3, and 8 to be dropped, resulting in 6 vertices in the end.
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.
Before the fix, the floor is checked in the order show above. so it drops 2 and 3, then finds a non convex situation when checking (5, 6, 7) and bails. We end up with 8 vertices.
For the ceiling, the vertices are reversed. So it drops 8 (shown above, 3 in actual order), finds the non convex situation at (7, 6, 5) (shown above, (4, 5, 6 in actual order)) and bails. We end up with 9 vertices.
(I explained the same on #10103 when I reported the defect)
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.
EXPECT_FALSE(ceilingSurface.IsConvex); | ||
} | ||
|
||
EXPECT_EQ(floorSurface.Sides, ceilingSurface.Sides); |
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.
Before fix:
[ RUN ] EnergyPlusFixture.SurfaceGeometry_CheckConvexity_ColinearStability
/home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/SurfaceGeometry.unit.cc:3331: Failure
Expected equality of these values:
6
floorSurface.Sides
Which is: 8
/home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/SurfaceGeometry.unit.cc:3350: Failure
Expected equality of these values:
6
ceilingSurface.Sides
Which is: 9
/home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/SurfaceGeometry.unit.cc:3354: Failure
Expected equality of these values:
floorSurface.Sides
Which is: 8
ceilingSurface.Sides
Which is: 9
I don't think we need to hold this up any longer, this is great. Thanks @jmarrec ! |
Pull request overview
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.