Fix #4837 - Geometry improvements - Detect incorrectly oriented surfaces in spaces, non convex spaces #4843
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jmarrec
added
component - Utilities Geometry
component - Model
Pull Request - Ready for CI
jmarrec
commented
Mar 29, 2023
| @@ -223,6 +223,8 @@ namespace model { | |||
| /// Returns any SurfacePropertyConvectionCoefficients associated with this surface, does not return SurfacePropertyConvectionCoefficientsMultipleSurface. | |||
| std::vector<SurfacePropertyConvectionCoefficients> surfacePropertyConvectionCoefficients() const; | |||
|
|
|||
| bool isConvex() const; | |||
There was a problem hiding this comment.
New user-facing method: bool PlanarSurface::isConvex() const
Comment on lines
+644
to
+661
| // Find all surfaces where the outwardNormal does not point towards the outside of the Space | ||
| std::vector<Surface> findSurfacesWithIncorrectOrientation() const; | ||
|
|
||
| // Checks the outwardNormal of every surface points towards the outside of the Space | ||
| bool areAllSurfacesCorrectlyOriented() const; | ||
|
|
||
| // Returns true if the orientation of any surface has been changed | ||
| bool fixSurfacesWithIncorrectOrientation(); | ||
|
|
||
| /** This method will check the floorPrint of the space, if that can't be computed, it's not convex. If it can, it checks whether that resulting | ||
| * Surface3d is convex. Note: having a floorPrint that's convex isn't sufficied to deem a space to be convex, the walls could still be non | ||
| * convex, but it should suit most typical applications */ | ||
| bool isConvex() const; | ||
|
|
||
| /** Checks if every Surface is convex, returns the Concave ones. | ||
| * Note: having a non convex surface does not necesarilly mean that the Space is not convex | ||
| * eg: a box with a wall that is split into two L s would return false, while the Space is actually still convex. */ | ||
| std::vector<Surface> findNonConvexSurfaces() const; |
There was a problem hiding this comment.
New user-facing methods in Space.
Comment on lines
+106
to
+107
| static boost::optional<Space> fromFloorPrint(const std::vector<Point3d>& floorPrint, double floorHeight, Model& model, | ||
| const std::string& spaceName = ""); |
There was a problem hiding this comment.
New optional spaceName param here, allow naming the Space plus the surfaces. Very useful for testing purposes in particular.
Comment on lines
+40
to
+54
| // Convenience functions for testing. All them returns point in Counterclockwise order, so if creating a floor, reverse the vertices | ||
|
|
||
| UTILITIES_API std::vector<Point3d> convexRegularPolygon(const Point3d& center, size_t num_sides, double side_with = 1.0); | ||
|
|
||
| UTILITIES_API std::vector<Point3d> hShapedPolygon(const Point3d& center, double total_length = 10.0); | ||
|
|
||
| UTILITIES_API std::vector<Point3d> uShapedPolygon(const Point3d& center, double total_length = 10.0); | ||
|
|
||
| UTILITIES_API std::vector<Point3d> tShapedPolygon(const Point3d& center, double total_length = 10.0); | ||
|
|
||
| UTILITIES_API std::vector<Point3d> lShapedPolygon(const Point3d& center, double total_length = 10.0); | ||
|
|
||
| UTILITIES_API std::vector<Point3d> squaredPolygon(const Point3d& center, double side_with = 10.0); | ||
|
|
||
| UTILITIES_API std::vector<Point3d> rectangularPolygon(const Point3d& center, double length_x = 20.0, double length_y = 10.0); |
There was a problem hiding this comment.
New convenience functions for creating standard shapes.
Comment on lines
+54
to
+59
| // Copy and move operators are implicitly declared | ||
| // Point3d(const Point3d& other) = default; | ||
| // Point3d(Point3d&& other) = default; | ||
| // Point3d& operator=(const Point3d&) = default; | ||
| // Point3d& operator=(Point3d&&) = default; | ||
| // ~Point3d() noexcept = default; |
There was a problem hiding this comment.
Throughout the entire Geometry subfolder, I have made sure to enable move semantics (most of the time that means just deleting the explicitly-defaulted copy constructor)
Comment on lines
167
to
201
| bool Surface3d::isConvex() const { | ||
|
|
||
| // const auto& a = edges.front().start(); | ||
| // const auto& b = edges.front().end(); | ||
| // const auto& c = edges.at(1).end(); | ||
|
|
||
| auto ab = edges.front().asVector(); | ||
| auto bc = edges[1].asVector(); | ||
| auto outwardNormal = ab.cross(bc); | ||
| outwardNormal.normalize(); | ||
|
|
||
| for (auto it = std::next(edges.begin()); it != edges.end(); ++it) { | ||
| auto itnext = std::next(it); | ||
| if (itnext == std::end(edges)) { | ||
| itnext = std::begin(edges); | ||
| } | ||
| const Point3d& a = it->start(); | ||
| const Point3d& b = it->end(); | ||
| const Point3d& c = itnext->end(); | ||
|
|
||
| auto ab = b - a; | ||
| auto ac = c - a; | ||
|
|
||
| auto triangleNormal = ab.cross(ac); | ||
| auto d = outwardNormal.dot(triangleNormal); | ||
| if (d < 0) { | ||
| return false; | ||
| } | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Convexity of a Surface3d: for each triangle of 3 points, we compute the triangleNormal by taking the cross product of AB x AC. We then take the dot product with the initial outwardNormal. If the dot product is negative, this means the triangleNomal is pointing in the opposite direction and we therefore have a non convex / concave surface.
Comment on lines
+212
to
+222
| // We call the volume calculation... if this ends up being negative, we're in the case where ALL surfaces are in the wrong orientation | ||
| if (!m_hasAnySurfaceWithIncorrectOrientation) { | ||
| m_polyhedronVolume = calcPolyhedronVolume(); | ||
| if (m_polyhedronVolume < 0) { | ||
| LOG(Error, "It seems that ALL surfaces are reversed"); | ||
| m_hasAnySurfaceWithIncorrectOrientation = true; | ||
| m_isCompletelyInsideOut = true; |
There was a problem hiding this comment.
Catch the case where ALL surfaces are inside out.... If we initially didn't find any orientation issues, but the Polyhedron's volume is negative, then it is inside out.
Comment on lines
246
to
278
| void Polyhedron::performEdgeMatching() { | ||
|
|
||
| m_hasAnySurfaceWithIncorrectOrientation = false; | ||
|
|
||
| for (size_t i = 0; i < m_surfaces.size(); ++i) { | ||
| for (size_t j = 0; j < m_surfaces.size(); ++j) { | ||
| if (i == j) { | ||
| continue; | ||
| } | ||
| Surface3dEdge thisSurface3dEdge(*it, *itnext, surface, surfNum); | ||
| auto itFound = std::find(uniqueSurface3dEdges.begin(), uniqueSurface3dEdges.end(), thisSurface3dEdge); | ||
| if (itFound == uniqueSurface3dEdges.end()) { | ||
| LOG(Debug, "NOT FOUND: " << thisSurface3dEdge); | ||
| uniqueSurface3dEdges.emplace_back(std::move(thisSurface3dEdge)); | ||
| } else { | ||
| LOG(Debug, " FOUND: " << thisSurface3dEdge); | ||
| itFound->appendSurface(surface); | ||
| auto& surface1 = m_surfaces[i]; | ||
| auto& surface2 = m_surfaces[j]; | ||
| for (Surface3dEdge& edge1 : surface1.edges) { | ||
| for (Surface3dEdge& edge2 : surface2.edges) { | ||
| if (edge1 == edge2) { | ||
| if (std::find(edge1.allSurfNums().begin(), edge1.allSurfNums().cend(), edge2.firstSurfNum()) == edge1.allSurfNums().end()) { | ||
| edge1.appendSurface(surface2); | ||
| edge2.appendSurface(surface1); | ||
| if (!edge1.reverseEqual(edge2)) { | ||
| edge1.markConflictedOrientation(); | ||
| edge2.markConflictedOrientation(); | ||
| m_hasAnySurfaceWithIncorrectOrientation = true; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Performing edge matching, in one go we pair the edges (later we can check that edge.count() == 2, any Polyhedron which has edges where count() != 2 is not enclosed), and we also check if that edge matches in reverse order to the other.
In a polyhedron that has a consistent orientation (typically all faces are in counter clockwise order), each edge must be matched by an edge in the opposite direction.
Comment on lines
406
to
461
| std::vector<Surface3d> Polyhedron::findSurfacesWithIncorrectOrientation() const { | ||
|
|
||
| if (!m_hasAnySurfaceWithIncorrectOrientation) { | ||
| return {}; | ||
| } | ||
|
|
||
| if (m_isCompletelyInsideOut) { | ||
| LOG(Error, "It seems that ALL surfaces are reversed"); | ||
| return m_surfaces; | ||
| } | ||
|
|
||
| if (!m_isEnclosedVolume) { | ||
| LOG(Warn, "Polyhedron is not enclosed. Looking surfaces with incorrect orientations can return false negatives."); | ||
| // return {}; | ||
| } | ||
|
|
||
| std::vector<Surface3dEdge> uniqueSurface3dEdges = uniqueEdges(); | ||
|
|
||
| // Remove non-conflicted edges | ||
| uniqueSurface3dEdges.erase( | ||
| std::remove_if(uniqueSurface3dEdges.begin(), uniqueSurface3dEdges.end(), [](const auto& edge) { return !edge.hasConflictedOrientation(); }), | ||
| uniqueSurface3dEdges.end()); | ||
| if (uniqueSurface3dEdges.empty()) { | ||
| return {}; | ||
| } | ||
|
|
||
| std::set<Surface3d> conflictedSurfaces; | ||
| for (auto& edge : uniqueSurface3dEdges) { | ||
| const auto& surfNums = edge.allSurfNums(); | ||
| const size_t sfIndex1 = surfNums.front(); | ||
| const size_t sfIndex2 = surfNums.back(); | ||
|
|
||
| const auto& sf1 = m_surfaces.at(sfIndex1); | ||
| const auto& sf2 = m_surfaces.at(sfIndex2); | ||
|
|
||
| const auto c1 = sf1.ratioOfConflictedEdges(); | ||
| const auto c2 = sf2.ratioOfConflictedEdges(); | ||
|
|
||
| LOG(Trace, fmt::format("'{}' has {:.2f}% conflicted edges ({}/{}), while '{}' has {:.2f}% conflicted edges ({}/{})", sf1.name, c1, | ||
| sf1.numConflictedEdges(), sf1.edges.size(), sf2.name, c2, sf2.numConflictedEdges(), sf2.edges.size())); | ||
| if (c1 > c2) { | ||
| conflictedSurfaces.insert(sf1); | ||
| } else { | ||
| conflictedSurfaces.insert(sf2); | ||
| } | ||
| } | ||
|
|
||
| return {conflictedSurfaces.begin(), conflictedSurfaces.end()}; |
There was a problem hiding this comment.
When determining surfaces with incorrect orientation, we check each conflicted edge.
For each of the two surfaces that share that edge, we compute the ratio of conflicted edges / total number of edges, and we keep the one with the largest ratio as this is probably the one that's in the wrong order.
Comment on lines
+485
to
+503
| double Polyhedron::polyhedronVolume() const { | ||
| if (m_isEnclosedVolume && (m_isCompletelyInsideOut || !m_hasAnySurfaceWithIncorrectOrientation)) { | ||
| return m_polyhedronVolume; | ||
| } | ||
| if (!m_isEnclosedVolume) { | ||
| LOG(Error, "Polyhedron volume calculation for a non-enclosed Polyhedron will return bogus values"); | ||
| } else if (m_hasAnySurfaceWithIncorrectOrientation && !m_isCompletelyInsideOut) { | ||
| LOG(Error, "Polyhedron volume calculation for an enclosed Polyhedron but with incorrectly oriented surfaces will return bogus values"); | ||
| } | ||
| return calcPolyhedronVolume(); | ||
| } |
There was a problem hiding this comment.
Trying to be as loud as possible when calling polyhedronVolume with a condition that will return wrong values.
jmarrec
force-pushed
the
issue-4837
branch
2 times, most recently
from
484c29c
to
4fa5eb1
Compare
jmarrec
added a commit
to openstudiocoalition/OpenStudioApplication
that referenced
this pull request
Mar 31, 2023
macumber
reviewed
Apr 1, 2023
| Surface3dEdgeVector edgesNot2; | ||
| void resetEdgeMatching(); | ||
|
|
||
| bool operator<(const Surface3d& rhs) const; | ||
| }; | ||
|
|
||
| class UTILITIES_API Polyhedron |
There was a problem hiding this comment.
Where did this come from, is this yours @jmarrec ?
There was a problem hiding this comment.
The enclosed checks were initially in E+, I've had to modify that portion last year so after learning about polyhedron geometry I decided to tackle the Space::volume issues that were still there 13 years later.
The work in this PR is original.
jmarrec
commented
Apr 3, 2023
Comment on lines
+663
to
+666
| /// @cond | ||
| void cacheGeometryDiagnostics(); | ||
| void resetCachedGeometryDiagnostics(); | ||
| /// @endcond |
There was a problem hiding this comment.
Hidden methods to cache... I can't very reliably use signals onChange because it depends on the surfaces... so I'm choosing not to, but making this hidden, so the ThreeJSForwardTranslator can use it.
Comment on lines
+319
to
+321
| // If set to false, the 4 diags below aren't used | ||
| bool includeGeometryDiagnostics() const; | ||
| void setIncludeGeometryDiagnostics(bool includeGeometryDiagnostics); |
Comment on lines
+67
to
+70
| // Check if Surface is Convex, and check if its correctly oriented given the containing space, whether the space itself is Convex, Enclosed | ||
| // This is computationally intensive, so it's opt-in | ||
| bool includeGeometryDiagnostics() const; | ||
| void setIncludeGeometryDiagnostics(bool includeGeometryDiagnostics); |
There was a problem hiding this comment.
Optional include of geometry diagnostics though with caching it's not that bad.
before caching:
| Test case | Opt-out | Opt-out | Opt-in | Opt-in |
|---|---|---|---|---|
| Debug | Release | Debug | Release | |
| 7_7_Windows_Complete | 84.63 | 11.78 | 26.76 | 3.68 |
| ParkUnder_Retail_Office_C2 | 120.64 | 18.08 | 15.92 | 2.13 |
After caching, in release:
1/4 Test #2562: ModelFixture.ThreeJSForwardTranslator_RefBldgOutPatientNew2004_Chicago ....................... Passed 1.39 sec
2/4 Test #2563: ModelFixture.ThreeJSForwardTranslator_RefBldgOutPatientNew2004_Chicago_GeometryDiagnostics ... Passed 1.40 sec
3/4 Test #2560: ModelFixture.ThreeJSForwardTranslator_7_7_Windows_Complete ................................... Passed 3.79 sec
4/4 Test #2561: ModelFixture.ThreeJSForwardTranslator_7_7_Windows_Complete_GeometryDiagnostics ............... Passed 3.95 sec
After caching, in debug:
1/4 Test #2562: ModelFixture.ThreeJSForwardTranslator_RefBldgOutPatientNew2004_Chicago ....................... Passed 10.09 sec
2/4 Test #2563: ModelFixture.ThreeJSForwardTranslator_RefBldgOutPatientNew2004_Chicago_GeometryDiagnostics ... Passed 10.19 sec
3/4 Test #2560: ModelFixture.ThreeJSForwardTranslator_7_7_Windows_Complete ................................... Passed 27.17 sec
4/4 Test #2561: ModelFixture.ThreeJSForwardTranslator_7_7_Windows_Complete_GeometryDiagnostics ............... Passed 28.33 sec
| Test case | Release | Release | Debug | Debug |
|---|---|---|---|---|
| No Diag | With Diag | No Diag | With Diag | |
| RefBldgOutPatientNew2004_Chicago | 1.39 | 1.4 | 10.09 | 10.19 |
| 7_7_Windows_Complete | 3.79 | 3.95 | 27.17 | 28.33 |
Comment on lines
+507
to
+513
| std::vector<Space> spaces; | ||
| if (m_includeGeometryDiagnostics) { | ||
| spaces = model.getConcreteModelObjects<Space>(); | ||
| for (auto& space : spaces) { | ||
| space.cacheGeometryDiagnostics(); | ||
| } | ||
| } |
There was a problem hiding this comment.
If including, precompute and cache all space info.
Comment on lines
+539
to
+544
| if (m_includeGeometryDiagnostics) { | ||
| for (auto& space : spaces) { | ||
| space.resetCachedGeometryDiagnostics(); | ||
| } | ||
| } | ||
|
|
jmarrec
added a commit
to openstudiocoalition/OpenStudioApplication
that referenced
this pull request
Apr 5, 2023
jmarrec
added a commit
to openstudiocoalition/OpenStudioApplication
that referenced
this pull request
Apr 5, 2023
jmarrec
force-pushed
the
issue-4837
branch
2 times, most recently
from
0feeac6
to
831493f
Compare
jmarrec
force-pushed
the
issue-4837
branch
2 times, most recently
from
b2bfe68
to
1c45975
Compare
… the same stuff over and over again and clean up the API. Catch the case where it's also completely inside out
…::vector<Surface> findNonConvexSurfaces
…nsive ``` | Test case | Opt-out | Opt-out | Opt-in | Opt-in | |----------------------------|---------|---------|--------|---------| | | Debug | Release | Debug | Release | | 7_7_Windows_Complete | 84.63 | 11.78 | 26.76 | 3.68 | | ParkUnder_Retail_Office_C2 | 120.64 | 18.08 | 15.92 | 2.13 | ```
``` 1/4 Test #2562: ModelFixture.ThreeJSForwardTranslator_RefBldgOutPatientNew2004_Chicago ....................... Passed 1.39 sec 2/4 Test #2563: ModelFixture.ThreeJSForwardTranslator_RefBldgOutPatientNew2004_Chicago_GeometryDiagnostics ... Passed 1.40 sec 3/4 Test #2560: ModelFixture.ThreeJSForwardTranslator_7_7_Windows_Complete ................................... Passed 3.79 sec 4/4 Test #2561: ModelFixture.ThreeJSForwardTranslator_7_7_Windows_Complete_GeometryDiagnostics ............... Passed 3.95 sec ```
|
CI Results for e03d083:
|
jmarrec
added a commit
to openstudiocoalition/OpenStudioApplication
that referenced
this pull request
Aug 28, 2023
jmarrec
added a commit
to openstudiocoalition/OpenStudioApplication
that referenced
this pull request
Aug 28, 2023
jmarrec
added a commit
to openstudiocoalition/OpenStudioApplication
that referenced
this pull request
Aug 29, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull request overview
Pull Request Author
src/model/test)src/energyplus/Test)src/osversion/VersionTranslator.cpp)Labels:
IDDChangeAPIChangePull Request - Ready for CIso that CI builds your PRReview Checklist
This will not be exhaustively relevant to every PR.