-
Notifications
You must be signed in to change notification settings - Fork 195
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
Geometry improvements #4221
Conversation
…hat takes two Polygons and is not affected by holes. UNit test to test this out (Polygon3d_Join)
…nd show fixed with using Polygon3ed
…nd returns Polygon3d list as result
…sider using them?
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 I also tagged as |
Unix has -Wall -Werror and is throwing at least here:
|
/// +----------------------------+ | ||
/// | Polygon A | | ||
/// | | | ||
/// | +---------+ | | ||
/// | | | | | ||
/// | | | | | ||
/// | | | | | ||
/// |--------+---------+---------| | ||
/// | | | ||
/// | Polygon B | | ||
/// | | | ||
/// | | | ||
/// +----------------------------+ |
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.
👍
…ror=sign-compare]
/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);
^~~~~~~~ |
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 |
…ets tho building on unix and macos
@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
(Your PR doesn't affect their status yet, they still fail) |
src/utilities/geometry/Polygon.cpp
Outdated
Vector3d Polygon3d::newellVector() { | ||
OptionalVector3d v = openstudio::getNewallVector(points); |
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.
One of these two is a typo
src/utilities/geometry/Polygon.cpp
Outdated
Polygon3d::Polygon3d(Point3dVector outerPath) { | ||
for (auto p : outerPath) { | ||
outerPath.push_back(p); |
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.
You're actually pushing to the argument you pass, not the private member.
src/utilities/geometry/Polygon.cpp
Outdated
|
||
|
||
void Polygon3d::setOuterPath(Point3dVector outerPath) { | ||
outerPath = outerPath; |
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.
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.
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.
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?
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.
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)
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.
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
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.
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)
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.
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:
- Round the input data first, so instead of 4.572000000000027, 4.571999157764836 use 4.572, 4.572
- 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.
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.
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
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.
@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
withsize_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.
- This is easy to fix (I'll replace
- 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)
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. |
@ggartside CI failures are due to scheduled brown outs at conan - https://status.bintray.com/. The bintray is something we are aware of and are actively moving away. |
CI Results for debd914:
|
@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 ) |
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.