Skip to content
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

Fix #10103 - CheckConvexity removes colinear vertices but stops when surface is determined non-convex, causing vertex size mismatch fatal error #10114

Merged
merged 3 commits into from
Jul 19, 2023
Merged

Fix #10103 - CheckConvexity removes colinear vertices but stops when surface is determined non-convex, causing vertex size mismatch fatal error #10114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c780970
Add a test for #10103
jmarrec Jul 10, 2023
20cb122
Fix #10103 - CheckConvexity removes colinear vertices but stops when …
jmarrec Jul 10, 2023
7918039
custom_check in unit test
jmarrec Jul 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 25 additions & 22 deletions src/EnergyPlus/SurfaceGeometry.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15788,30 +15788,33 @@ namespace SurfaceGeometry {
}

if (SignFlag != PrevSignFlag) {
if (state.dataHeatBal->SolarDistribution != DataHeatBalance::Shadowing::Minimal && surfaceTmp.ExtSolar) {
if (state.dataGlobal->DisplayExtraWarnings) {
ShowWarningError(state,
format("CheckConvexity: Zone=\"{}\", Surface=\"{}\" is non-convex.",
state.dataHeatBal->Zone(surfaceTmp.Zone).Name,
surfaceTmp.Name));
int Np1 = n + 1;
if (Np1 > NSides) {
Np1 -= NSides;
}
int Np2 = n + 2;
if (Np2 > NSides) {
Np2 -= NSides;
}
ShowContinueError(state, format("...vertex {} to vertex {} to vertex {}", n, Np1, Np2));
ShowContinueError(state, format("...vertex {}=[{:.2R},{:.2R},{:.2R}]", n, X(n), Y(n), Z(n)));
ShowContinueError(state, format("...vertex {}=[{:.2R},{:.2R},{:.2R}]", Np1, X(n + 1), Y(n + 1), Z(n + 1)));
ShowContinueError(state, format("...vertex {}=[{:.2R},{:.2R},{:.2R}]", Np2, X(n + 2), Y(n + 2), Z(n + 2)));
// ShowContinueError(state, format("...theta angle=[{:.6R}]", Theta));
// ShowContinueError(state, format("...last theta angle=[{:.6R}]", LastTheta));
}
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.",
Comment on lines +15791 to +15796
Copy link
Contributor Author

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)

state.dataHeatBal->Zone(surfaceTmp.Zone).Name,
surfaceTmp.Name));
int Np1 = n + 1;
if (Np1 > NSides) {
Np1 -= NSides;
}
int Np2 = n + 2;
if (Np2 > NSides) {
Np2 -= NSides;
}
ShowContinueError(state, format("...vertex {} to vertex {} to vertex {}", n, Np1, Np2));
ShowContinueError(state, format("...vertex {}=[{:.2R},{:.2R},{:.2R}]", n, X(n), Y(n), Z(n)));
ShowContinueError(state, format("...vertex {}=[{:.2R},{:.2R},{:.2R}]", Np1, X(n + 1), Y(n + 1), Z(n + 1)));
ShowContinueError(state, format("...vertex {}=[{:.2R},{:.2R},{:.2R}]", Np2, X(n + 2), Y(n + 2), Z(n + 2)));
// ShowContinueError(state, format("...theta angle=[{:.6R}]", Theta));
// ShowContinueError(state, format("...last theta angle=[{:.6R}]", LastTheta));
}
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;
Comment on lines 15814 to +15817
Copy link
Contributor Author

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.

}
PrevSignFlag = SignFlag;
LastTheta = Theta;
Expand Down
80 changes: 80 additions & 0 deletions tst/EnergyPlus/unit/SurfaceGeometry.unit.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3274,6 +3274,86 @@ TEST_F(EnergyPlusFixture, SurfaceGeometry_CheckConvexityTest_ASHRAE901_Hospital_
EXPECT_FALSE(surface.IsConvex);
}

TEST_F(EnergyPlusFixture, SurfaceGeometry_CheckConvexity_ColinearStability)
{
// 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
Comment on lines +3279 to +3295
Copy link
Contributor Author

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.

Copy link
Contributor Author

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhh this makes sense and is a phenomenal demonstration of the problem and test of the routine. Thanks @jmarrec !

@mjwitte this looks good, but feel free to take a look before this merges.


const std::vector<Vector> floorVertices = {
{30.0, 0., 0.},
{20.0, 0., 0.},
{10.0, 0., 0.},
{0.0, 0., 0.},
{0.0, 20., 0.},
{20.0, 20., 0.},
{20.0, 40., 0.},
{30.0, 40., 0.},
{40.0, 40., 0.},
{40.0, 0., 0.},
};
const int nVertices = static_cast<int>(floorVertices.size());

state->dataSurface->TotSurfaces = 2;
constexpr int floorSurfNum = 1;
constexpr int ceilingSurfNum = 2;
state->dataSurface->MaxVerticesPerSurface = nVertices;
state->dataSurfaceGeometry->SurfaceTmp.allocate(state->dataSurface->TotSurfaces);

auto &floorSurface = state->dataSurfaceGeometry->SurfaceTmp(floorSurfNum);
auto &ceilingSurface = state->dataSurfaceGeometry->SurfaceTmp(ceilingSurfNum);
{
floorSurface.Azimuth = 0.0;
floorSurface.Tilt = 0.0;
floorSurface.Sides = nVertices;
floorSurface.GrossArea = 100.0;
floorSurface.Name = "Floor";
floorSurface.Vertex.allocate(nVertices);

floorSurface.Vertex = floorVertices;

CheckConvexity(*state, floorSurfNum, floorSurface.Sides);

EXPECT_EQ(6, floorSurface.Sides);
EXPECT_FALSE(floorSurface.IsConvex);
}

{
auto ceilingVertices = floorVertices;
std::reverse(ceilingVertices.begin(), ceilingVertices.end());

ceilingSurface.Azimuth = 0.0;
ceilingSurface.Tilt = 0.0;
ceilingSurface.Sides = nVertices;
ceilingSurface.GrossArea = 100.0;
ceilingSurface.Name = "Ceiling";
ceilingSurface.Vertex.allocate(nVertices);

ceilingSurface.Vertex = ceilingVertices;

CheckConvexity(*state, ceilingSurfNum, ceilingSurface.Sides);

EXPECT_EQ(6, ceilingSurface.Sides);
EXPECT_FALSE(ceilingSurface.IsConvex);
}

EXPECT_EQ(floorSurface.Sides, ceilingSurface.Sides);
Copy link
Contributor Author

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

}

TEST_F(EnergyPlusFixture, InitialAssociateWindowShadingControlFenestration_test)
{
state->dataSurface->TotWinShadingControl = 3;
Expand Down