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.

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

Geometry improvements #4221

Merged
merged 90 commits into from
Apr 13, 2021
Merged

Geometry improvements #4221

merged 90 commits into from
Apr 13, 2021

Conversation

ggartside
Copy link
Collaborator

@ggartside ggartside commented Feb 23, 2021

Pull request overview

Adds Polygon3d class to geometry utility, adds new join and joinAll methods to intersection. Existing join and joinAll methods are not affected.

Adds unit tests in Intersection to test new join and joinAll methods, and unit tests go Geometry to test basic functions of the pOlygon3d class such as area and normal calculations

This will fix #1614 - joinAll fails on cases with inner loop in result

Review Checklist

This will not be exhaustively relevant to every PR.

  • Perform a Code Review on GitHub
  • Code Style, strip trailing whitespace, etc.
  • All related changes have been implemented: model changes, model tests, FT changes, FT tests, VersionTranslation, OS App
  • Labeling is ok
  • If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
  • If feature, test running new feature, try creative ways to break it
  • CI status: all green or justified

@jmarrec
Copy link
Collaborator

jmarrec commented Feb 25, 2021

I just dropped by and saw clang-format/cppcheck being unhappy so I fixed that. I see you tried to clang-format @ggartside already, but you probably have an older version of clang-format or something.

I commented some unused variables in tests to please cppcheck in ea25e0e, but it would be better to actually use these variables in the test probably, like adding an EXPECT_EQ(xx, perimeter), I'll let you decide.

I also tagged as Pull Request - Ready for CI, I'm looking forward to test results (to see if these fixes the Space_GTest degenerate tests etc (which I locally disabled today as it's been annoying me for a while to see them fail))

@jmarrec jmarrec added component - Utilities Geometry Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. labels Feb 25, 2021
@jmarrec
Copy link
Collaborator

jmarrec commented Feb 25, 2021

Unix has -Wall -Werror and is throwing at least here:

[  4%] Generating ruby_OpenStudioGBXML_wrap.cxx

/srv/jenkins/openstudio/git/incremental/OpenStudio/src/utilities/geometry/Polygon.cpp: In member function ‘double openstudio::Polygon3d::getPerimeter()’:

/srv/jenkins/openstudio/git/incremental/OpenStudio/src/utilities/geometry/Polygon.cpp:113:22: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]

   for (long i = 0; i < points.size(); i++) {

                    ~~^~~~~~~~~~~~~~~

src/utilities/geometry/Polygon.cpp Outdated Show resolved Hide resolved
src/utilities/geometry/Polygon.cpp Outdated Show resolved Hide resolved
src/utilities/geometry/Polygon.hpp Outdated Show resolved Hide resolved
Comment on lines +1850 to +1862
/// +----------------------------+
/// | Polygon A |
/// | |
/// | +---------+ |
/// | | | |
/// | | | |
/// | | | |
/// |--------+---------+---------|
/// | |
/// | Polygon B |
/// | |
/// | |
/// +----------------------------+
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@jmarrec
Copy link
Collaborator

jmarrec commented Feb 26, 2021

/srv/jenkins/openstudio/git/incremental/OpenStudio/src/utilities/geometry/Test/Geometry_GTest.cpp: In member function ‘virtual void GeometryFixture_Polygon_Basic_Test::TestBody()’:
/srv/jenkins/openstudio/git/incremental/OpenStudio/src/utilities/geometry/Test/Geometry_GTest.cpp:1130:24: error: cannot bind non-const lvalue reference of type ‘openstudio::Point3d&’ to an rvalue of type ‘openstudio::Point3d’
   testPolygon.addPoint(Point3d(0, 0, 0));
                        ^~~~~~~~~~~~~~~~
In file included from /srv/jenkins/openstudio/git/incremental/OpenStudio/src/utilities/geometry/Test/Geometry_GTest.cpp:36:0:
/srv/jenkins/openstudio/git/incremental/OpenStudio/src/utilities/geometry/Test/../Polygon.hpp:52:8: note:   initializing argument 1 of ‘void openstudio::Polygon3d::addPoint(openstudio::Point3d&)’
   void addPoint(Point3d& point);
        ^~~~~~~~

/srv/jenkins/openstudio/git/incremental/OpenStudio/src/utilities/geometry/Test/Geometry_GTest.cpp:1131:24: error: cannot bind non-const lvalue reference of type ‘openstudio::Point3d&’ to an rvalue of type ‘openstudio::Point3d’
   testPolygon.addPoint(Point3d(100, 0, 0));
                        ^~~~~~~~~~~~~~~~~~
In file included from /srv/jenkins/openstudio/git/incremental/OpenStudio/src/utilities/geometry/Test/Geometry_GTest.cpp:36:0:
/srv/jenkins/openstudio/git/incremental/OpenStudio/src/utilities/geometry/Test/../Polygon.hpp:52:8: note:   initializing argument 1 of ‘void openstudio::Polygon3d::addPoint(openstudio::Point3d&)’
   void addPoint(Point3d& point);
        ^~~~~~~~

/srv/jenkins/openstudio/git/incremental/OpenStudio/src/utilities/geometry/Test/Geometry_GTest.cpp:1132:24: error: cannot bind non-const lvalue reference of type ‘openstudio::Point3d&’ to an rvalue of type ‘openstudio::Point3d’
   testPolygon.addPoint(Point3d(100, 100, 0));
                        ^~~~~~~~~~~~~~~~~~~~
In file included from /srv/jenkins/openstudio/git/incremental/OpenStudio/src/utilities/geometry/Test/Geometry_GTest.cpp:36:0:
/srv/jenkins/openstudio/git/incremental/OpenStudio/src/utilities/geometry/Test/../Polygon.hpp:52:8: note:   initializing argument 1 of ‘void openstudio::Polygon3d::addPoint(openstudio::Point3d&)’
   void addPoint(Point3d& point);
        ^~~~~~~~

@ggartside
Copy link
Collaborator Author

I just dropped by and saw clang-format/cppcheck being unhappy so I fixed that. I see you tried to clang-format @ggartside already, but you probably have an older version of clang-format or something.

I commented some unused variables in tests to please cppcheck in ea25e0e, but it would be better to actually use these variables in the test probably, like adding an EXPECT_EQ(xx, perimeter), I'll let you decide.

I also tagged as Pull Request - Ready for CI, I'm looking forward to test results (to see if these fixes the Space_GTest degenerate tests etc (which I locally disabled today as it's been annoying me for a while to see them fail))

Can you tell me which tests these are and I'll look out for them - we are mainly addressing a limited number of outstanding issues and those may include the space tests or not, I don't know at this stage

@jmarrec
Copy link
Collaborator

jmarrec commented Mar 9, 2021

@ggartside I opened a PR to this branch with proposed changes, that should fix your build issues. #4238. I'll let you review and determine whether they make sense or not.


The failing geometry tests I was talking about can be run via ctest or Products/openstudio_model_tests at the root of the build folder:

ModelFixture.Space_intersectSurfaces_degenerate1
ModelFixture.Space_intersectSurfaces_degenerate2
ModelFixture.Space_intersectSurfaces_degenerate3
ModelFixture.ThreeJSReverseTranslator_FloorplanJS_SimAUD_Paper

(Your PR doesn't affect their status yet, they still fail)

Comment on lines 75 to 76
Vector3d Polygon3d::newellVector() {
OptionalVector3d v = openstudio::getNewallVector(points);
Copy link
Collaborator

Choose a reason for hiding this comment

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

One of these two is a typo

Comment on lines 38 to 40
Polygon3d::Polygon3d(Point3dVector outerPath) {
for (auto p : outerPath) {
outerPath.push_back(p);
Copy link
Collaborator

Choose a reason for hiding this comment

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

You're actually pushing to the argument you pass, not the private member.



void Polygon3d::setOuterPath(Point3dVector outerPath) {
outerPath = outerPath;
Copy link
Collaborator

@jmarrec jmarrec Mar 9, 2021

Choose a reason for hiding this comment

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

You're trying to assign to the argument passed not the private member

Same as

void func(T arg) {
   arg = arg
}

Clang throws on it.

/Users/jenkins/git/OpenStudioIncremental/develop/src/utilities/geometry/Polygon.cpp:60:13: error: explicitly assigning value of variable of type 'openstudio::Point3dVector' (aka 'vector<openstudio::Point3d>') to itself [-Werror,-Wself-assign-overloaded]
  outerPath = outerPath;

#4238 should fix that issue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok that was bad!

The visual studio compiler didn't complain though no doubt there are warnings somewhere, clang though, I'm not familiar with clang, is this something I can run locally as part of the build?

Copy link
Collaborator

Choose a reason for hiding this comment

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

clang is LLVM's compiler. Typically found on mac, as Xcode is apple-clang (a fork of clang). You could run that on Windows but I would't bother, that's why we have CI in place to build on MSVC (Windows), Clang (Mac) and GCC (Ubuntu)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Two of those degenerate space/surface tests are fixed with the latest check in, the one not fixed, 2, has a couple of very small surfaces that are not eliminated but make the combined roof areas outside tolerance

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm still seeing all of them fail on Ubuntu 18.04 with the tip of this branch 5f7742b

The following tests FAILED:

	2146 - ModelFixture.Space_intersectSurfaces_degenerate1 (Failed)
	2147 - ModelFixture.Space_intersectSurfaces_degenerate2 (Failed)
	2148 - ModelFixture.Space_intersectSurfaces_degenerate3 (Failed)
	2284 - ModelFixture.ThreeJSReverseTranslator_FloorplanJS_SimAUD_Paper (Failed)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That would be because I haven't pushed the changes yet, sorry to confuse you, I'm not much of a git user so our terminology may differ a little.

Also you should be aware that the contract terms with NREL are for us to resolve the 10 geometry utility issues that are assigned to me, not to get these tests working but if the bug fixes I make also fix these issues than all the better but that is not my main focus.

Anyway I ran that test locally and it still fails with one error:
Initial area of other surface 'Face 188' 522.58 does not equal post intersection area 522.579
It looks like a tolerance issue to me, the area tolerances are usually set to 0.0001 m2 so they are very tight.

In this instance the initial area of 'Face 188' is 522.5795999999999
the post intersection area is 522.579131498
A difference of about 4mm2 (assuming units are m)

The post intersection area is calculated from the common area of the intersection plus the remaining areas of the original polygons. We are reliant on the numerical accuracy of the boost polygon boolean operation and also how accurate the area calculation method is. There's nothing we can do to change this, however there are other things we can do.

Accuracy may be improved with the polygon boolean operations by rounding the numbers first.

Additionally when converting to boost data types the input vertices are modified such that if any of the vertices are slightly different to a canonical set of vertices but within the 0.01 tolerance then those vertices are modified so they are identical to the canonical vertex. This is a good strategy for polygon booleans, I use the same strategy myself, but it means that the area comparison is being done between an UNMODIFIED set of vertices and a potentially MODIFIED set of vertices which means there is a high likelihood that the areas won't match, whether the difference is within tolerance or not is entirely dependent on the original geometry. To remove this error the area calculations should be done between the MODIFIED vertices input to the boolean operation and the MODIFIED vertices that result from the boolean operation.

In this case the polygon that has the error, the second polygon, has vertices that are slightly different to the first polygon and therefore its vertices are modified and therefore its area has changed.

My recommendations to fix this would be:

  1. Round the input data first, so instead of 4.572000000000027, 4.571999157764836 use 4.572, 4.572
  2. Relax the tolerance of the area calculations to take into account the numerical variations OR use the modified vertices for the area calculation

This worksheet contains the data for the particular intersection that has failed, it will help you better understand the geometries involved.

ThreeJSReverseTranslator_FloorplanJS_SimAUD_Paper.xlsx

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Julien, do you have any idea what this syntax error might be?

[src/utilities/geometry/Intersection.cpp:82]:(warning),[constStatement],Found suspicious operator '>'
[src/utilities/geometry/Intersection.cpp:83]:(warning),[constStatement],Found suspicious operator '>'
[src/utilities/geometry/Intersection.cpp:84]:(warning),[constStatement],Found suspicious operator '>'
[src/utilities/geometry/Intersection.cpp:1295]:(warning),[constStatement],Found suspicious operator '>'

The lines in the file are
BoostMultiPolygon resultExpand;
BoostMultiPolygon resultShrink;
BoostMultiPolygon result;

And BoostMultiPolygon is defined as

typedef boost::geometry::model::multi_polygon BoostMultiPolygon;

BoostMultiPolygon is also used in Space.cpp

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ggartside Thanks for the excel sheet and the explanation. I've got to admit I get slightly lost with this geometry stuff as it don't think about it often and it requires me to concentrate really hard and take everything step by step. From your description and the sheet, I think what you wrote makes total sense and it does seem to be a combination of 1.) Comparing two slightly different things and 2.) very low tolerance (the units are indeed in meters, so 4mm2 isn't a huge deal).


These warnings Found suspicious operator '>' are issued by cppcheck, and cppcheck especially given that we pass the --inconclusive flag can throw false positive (and has some parse errors sometimes).

I was going to revert your commit 8b5e98a which I think is unnecessary but I see you reverted the changes in 212aea6 already. I will fix the cppcheck warnings with an inline suppression.


But as it stands two things:

  • The build fails due to warnings treated as error in the Space_GTest.cpp you added (and a few other places)
    • This is easy to fix (I'll replace int i = 0 with size_t i = 0)
    • But you seem to be using OSM test file(s) that you didn't include in source control. I included an example of how to properly register test OSMs in CMakeLists.txt and how to use them in the last commit. Please adjust that accordingly.
  • Your changes created some new test failures too, in 8b5e98a these are:
The following tests FAILED:
	417 - GeometryFixture.Polygon3d_Join (Failed)
	418 - GeometryFixture.Polygon3d_JoinAll_1614 (Failed)
	419 - GeometryFixture.Polygon3d_JoinAllPolygons_1614 (Failed)
	420 - GeometryFixture.JoinAll_2527 (Failed)
	2210 - ModelFixture.Surface_Intersect_DifferentHeight_PartialOverlap (Failed)
	2211 - ModelFixture.Surface_Intersect_DifferentHeight_ShareOneEdge_PartialOverlap (Failed)

@ggartside
Copy link
Collaborator Author

Nice job, I like the Polygon class and especially all the tests! I left some comments. My hope is that one day we could port Topolys to C++ and add to OpenStudio. Maybe consider that for future work? I would be happy for DA to do the port if it is useful

Thanks! I see the polygon class as away to do the surface intersection and matching without having to immediately triangulate to remove holes. If you defer triangulation to the end then in a lot of cases you will end up with a simpler set of surfaces. Some times the holes will be removed by subsequent intersections so it isn't event necessary to remove them. If you look at page 4 of the attached document you'll see what I mean. And also on page 5 you'll see a bunch of off angle edges when most surfaces are oriented vertically or horizontally, those extra edges are the result of triangulation.

And finally I think you can get a big performance gain by matching when you intersect I I think you are aware of this because there is a comment int he code about this). When you intersect two surfaces and get the common are between them you have by definition already found the matching pair of surfaces so there is no need to have to 're-find' them later

I will have to take a look at Topolys to get a better understanding of it.

Phase 3 - Prototyping.docx

@tijcolem
Copy link
Collaborator

@ggartside CI failures are due to scheduled brown outs at conan - https://status.bintray.com/.
Should be back up later today and I will re-run for the latest commits.

The bintray is something we are aware of and are actively moving away.

@tijcolem tijcolem merged commit f2930bb into develop Apr 13, 2021
@tijcolem tijcolem deleted the Geometry_Improvements branch April 13, 2021 19:02
@tijcolem tijcolem restored the Geometry_Improvements branch April 20, 2021 19:09
This was referenced Apr 20, 2021
@jmarrec
Copy link
Collaborator

jmarrec commented Apr 29, 2021

@ggartside I modified your Original post above to actually write Fix #1614 (this has the effect of automatically closing the associated issue when this PR is merged). I noticed two issues you may be fixing as well:

Fix #1398 - Native Support to calculate building perimeter

Fix #1586 - joinAllMethod seems be to order dependent

Please add them to your original post ahead if it does fix them.

@ggartside Ping on this one please. I think #1398 at least got fixed by this PR. I would appreciate if you would scan the list of open issues assigned to you and inform whether you think this PR fixed them (https://github.com/NREL/OpenStudio/issues?q=assignee%3Aggartside+is%3Aopen) Thanks!

(cc @tijcolem )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component - Utilities Geometry Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

joinAll fails on cases with inner loop in result
7 participants