Fix array bounds error for interzone windows and fix convexity of mirrored surfaces #10498

mjwitte · 2024-05-06T22:07:34Z

Pull request overview

Fixes idf file causing segfault and mirrors of nonconvex shading surfaces are classified as convex #10490
Fixes array bounds error with interzone (interspace) windows when the number of zones is < the number of enclosures. This issue was the root cause of a release build crash with the defect file.
Fixes MakeMirrorSurface to properly set IsConvex for the mirrored surface. When a mirrored surface is created, the original surface has already set IsConvex for itself. The former MakeMirrorSurface copied a list of specific attributes to the new surface, but did not copy IsConvex so it was always true. This resulted in a non-convex surface being classified as convex which then failed debug assert in Surface2D for mirrored nonconvex shading surfaces with 20 or more vertices (that's a mouthful). The defect file tripped on this assert in a debug build, masking the arrays bounds issue.
Streamlines MakeMirrorSurface.
Add some new constants to avoid using literal 1.0e-6 and the like.
Do some housekeeping in Vectors.cc

Defects Fixed

Revised defect file (which runs faster): 10490.idf.txt
The defect file crashed with a release build and failed an assert with a debug build revealing two separate issues.
The defect file now runs to completion with both release and debug builds.

Diffs

There are no diffs. There was an expectation that diffs might show up from fixing the mirrored surface convexity. This fix only impacts the mirrored side of a non-convex shading surface which will only impact the results if the mirrored side casts shadows toward the building and if the non-convex shape is such that the Polygon Clipping Algorithm does not handle it correctly.

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.

…indowsAndSurfs20PlusVertices

mjwitte · 2024-05-08T17:51:27Z

Added some unit tests for surfaces with >=20 vertices. Here's a plot of the vertices for the defect file surface that fails the original assert. Looks like it may not be a valid surface in the first place. Clearly it should not be classified as Convex.

Here's the surface object shown above

  Shading:Building:Detailed,
    aim108660,               !- Name
    ,                        !- Transmittance Schedule Name
    ,                        !- Number of Vertices
    4.5047023,14.8653133,35.35,  !- X,Y,Z ==> Vertex 1 {m}
    6.5689151,13.4862441,35.35,  !- X,Y,Z ==> Vertex 2 {m}
    6.1242243,12.8206238,35.35,  !- X,Y,Z ==> Vertex 3 {m}
    7.8836902,11.6451513,35.35,  !- X,Y,Z ==> Vertex 4 {m}
    8.2156112,12.1419759,35.35,  !- X,Y,Z ==> Vertex 5 {m}
    8.7993282,11.7520035,35.35,  !- X,Y,Z ==> Vertex 6 {m}
    8.9659831,12.0014552,35.35,  !- X,Y,Z ==> Vertex 7 {m}
    9.2241656,11.8289674,35.35,  !- X,Y,Z ==> Vertex 8 {m}
    9.4352618,12.1449395,35.35,  !- X,Y,Z ==> Vertex 9 {m}
    8.5933623,12.7073998,35.35,  !- X,Y,Z ==> Vertex 10 {m}
    9.6032909,14.219077,35.35,  !- X,Y,Z ==> Vertex 11 {m}
    9.5550636,14.251297,35.35,  !- X,Y,Z ==> Vertex 12 {m}
    8.5268029,12.71218,35.35,  !- X,Y,Z ==> Vertex 13 {m}
    9.1313075,12.3083197,35.35,  !- X,Y,Z ==> Vertex 14 {m}
    8.7902205,11.7977752,35.35,  !- X,Y,Z ==> Vertex 15 {m}
    8.1857159,12.2016356,35.35,  !- X,Y,Z ==> Vertex 16 {m}
    7.8676828,11.7255986,35.35,  !- X,Y,Z ==> Vertex 17 {m}
    6.2046715,12.8366312,35.35,  !- X,Y,Z ==> Vertex 18 {m}
    6.6740828,13.5392534,35.35,  !- X,Y,Z ==> Vertex 19 {m}
    4.646872,14.8936022,35.35,  !- X,Y,Z ==> Vertex 20 {m}
    5.8684524,16.7220831,35.35,  !- X,Y,Z ==> Vertex 21 {m}
    7.8474358,15.3999543,35.35,  !- X,Y,Z ==> Vertex 22 {m}
    7.8796558,15.4481816,35.35,  !- X,Y,Z ==> Vertex 23 {m}
    5.815443,16.8272508,35.35;  !- X,Y,Z ==> Vertex 24 {m}

mjwitte · 2024-05-09T13:39:12Z

Noticed this comment in SurfaceGeometry::MakeMirrorSurface:

EnergyPlus/src/EnergyPlus/SurfaceGeometry.cc

Lines 9758 to 9761 in 4d39dff

    
           // doesn't work when Vertex are pointers  SurfaceTmp(SurfNum+1)=SurfaceTmp(SurfNum) 
        
           state.dataSurfaceGeometry->SurfaceTmp(SurfNum + 1).Name = state.dataSurfaceGeometry->SurfaceTmp(SurfNum).Name; 
        
           state.dataSurfaceGeometry->SurfaceTmp(SurfNum + 1).Construction = state.dataSurfaceGeometry->SurfaceTmp(SurfNum).Construction; 
        
           state.dataSurfaceGeometry->SurfaceTmp(SurfNum + 1).ConstructionStoredInputValue =

But vertices haven't been POINTER since this commit many many moons ago.

So let's try SurfaceTmp(SurfNum+1)=SurfaceTmp(SurfNum) and see what happens . . .

mjwitte

Code walkthrough.

mjwitte · 2024-05-10T14:27:42Z

src/EnergyPlus/DataGlobalConstants.hh

+    Real64 constexpr OneThousandth = 1.0e-3; // Used as a tolerance in various places
+    Real64 constexpr OneMillionth = 1.0e-6;  // Used as a tolerance in various places

+    Real64 constexpr DistTooSmall(1.e-4);      // Geometric tolerance


Added some new constants that are used as tolerances in various places in surface geometry and shadowing calculations. DistToolSmall was moved up from a local declaration in Vectors.cc - decided to keep the same name.

mjwitte · 2024-05-10T14:28:39Z

src/EnergyPlus/DataSurfaces.cc

@@ -88,15 +87,6 @@ using namespace WindowManager;

 Array1D_string const cExtBoundCondition({-6, 0}, {"KivaFoundation", "FCGround", "OSCM", "OSC", "OSC", "Ground", "ExternalEnvironment"});

-// Parameters to indicate surface classes


Removed some comments that were left behind at some point when the parameters were moved to an enum.

mjwitte · 2024-05-10T14:32:23Z

src/EnergyPlus/DataSurfaces.cc

-            assert((shapeCat == ShapeCat::Nonconvex) || (crossEdges.size() == 2));
+            assert((shapeCat == ShapeCat::Nonconvex) || (crossEdges.size() == 2u));


This is the assert that was failing. Initially, I thought it was incorrect and should be >=2, but in the end the failing surface was ShapeCat:Convex here when it should have been Nonconvex.

mjwitte · 2024-05-10T14:35:32Z

src/EnergyPlus/SurfaceGeometry.cc

+        auto &origSurface = state.dataSurfaceGeometry->SurfaceTmp(SurfNum);
+        auto &newSurface = state.dataSurfaceGeometry->SurfaceTmp(SurfNum + 1);
+        newSurface = origSurface;


In MakeMirrorSurface just copy the entire original surface into the new surface instead of trying to copy each pertinent field individually. Then go about modifying name, vertices and other things. See #10498 (comment).

mjwitte · 2024-05-10T14:36:28Z

src/EnergyPlus/Vectors.cc

@@ -110,16 +106,6 @@ Vector const XUnit(1.0, 0.0, 0.0);
 Vector const YUnit(0.0, 1.0, 0.0);
 Vector const ZUnit(0.0, 0.0, 1.0);

-// DERIVED TYPE DEFINITIONS


Some housekeeping throughout Vectors.cc.

mjwitte · 2024-05-10T14:45:35Z

tst/EnergyPlus/unit/DataSurfaces.unit.cc

@@ -244,6 +244,154 @@ TEST_F(EnergyPlusFixture, SurfaceTest_Surface2D)
    }
 }

+TEST_F(EnergyPlusFixture, SurfaceTest_Surface2D_bigVertices)


2 new unit tests added related to the failed assert in Surface2D. The first is a convex surface, the second is nonconvex. In hindsight, this isn't focusing on the root problems.

mjwitte added 2 commits May 6, 2024 13:09

Fix interior window array size

17c808c

Fix Surface2D assert

7461b39

mjwitte added the Defect Includes code to repair a defect in EnergyPlus label May 6, 2024

mjwitte marked this pull request as draft May 6, 2024 22:09

mjwitte added this to the EnergyPlus 24.2 milestone May 8, 2024

mjwitte added 2 commits May 8, 2024 12:32

Merge remote-tracking branch 'remotes/origin/develop' into Interzonew…

c40c608

…indowsAndSurfs20PlusVertices

Initial unit tests

bcf2046

mjwitte added 3 commits May 8, 2024 18:05

Revert Surface2D assert

259849e

Revise unit tests

2a93689

Fix convexity for mirrored surfaces

85662bb

mjwitte added 4 commits May 9, 2024 09:28

Streamline MakeMirrorSurface

28ec9eb

Add some constants

b5d41cf

Use new constants

c4fd8dc

Use new constants and housekeeping

57e23ee

mjwitte changed the title ~~Fix array bounds error for interzone windows when number of zones < enclosures~~ Fix array bounds error for interzone windows and fix convexity of mirrored surfaces May 10, 2024

mjwitte commented May 10, 2024

View reviewed changes

mjwitte marked this pull request as ready for review May 10, 2024 14:46

Myoldmopar self-assigned this May 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix array bounds error for interzone windows and fix convexity of mirrored surfaces #10498

Fix array bounds error for interzone windows and fix convexity of mirrored surfaces #10498

mjwitte commented May 6, 2024 •

edited

mjwitte commented May 8, 2024 •

edited

mjwitte commented May 9, 2024

mjwitte left a comment

mjwitte May 10, 2024

mjwitte May 10, 2024

mjwitte May 10, 2024

mjwitte May 10, 2024

mjwitte May 10, 2024

mjwitte May 10, 2024

		@@ -88,15 +87,6 @@ using namespace WindowManager;

		Array1D_string const cExtBoundCondition({-6, 0}, {"KivaFoundation", "FCGround", "OSCM", "OSC", "OSC", "Ground", "ExternalEnvironment"});

		// Parameters to indicate surface classes

		assert((shapeCat == ShapeCat::Nonconvex) \|\| (crossEdges.size() == 2));
		assert((shapeCat == ShapeCat::Nonconvex) \|\| (crossEdges.size() == 2u));

Fix array bounds error for interzone windows and fix convexity of mirrored surfaces #10498

Are you sure you want to change the base?

Fix array bounds error for interzone windows and fix convexity of mirrored surfaces #10498

Conversation

mjwitte commented May 6, 2024 • edited

Pull request overview

Defects Fixed

Diffs

Pull Request Author

Reviewer

mjwitte commented May 8, 2024 • edited

mjwitte commented May 9, 2024

mjwitte left a comment

Choose a reason for hiding this comment

mjwitte May 10, 2024

Choose a reason for hiding this comment

mjwitte May 10, 2024

Choose a reason for hiding this comment

mjwitte May 10, 2024

Choose a reason for hiding this comment

mjwitte May 10, 2024

Choose a reason for hiding this comment

mjwitte May 10, 2024

Choose a reason for hiding this comment

mjwitte May 10, 2024

Choose a reason for hiding this comment

mjwitte commented May 6, 2024 •

edited

mjwitte commented May 8, 2024 •

edited