-
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
Gerard/4372 intersection issue found using create bar #4527
Gerard/4372 intersection issue found using create bar #4527
Conversation
… tolerance used in removeSpikesEx collapsed narrow aspect ratio surfaces to 0 area surfaces
The issue was a fault in removeSpikesEx. In this method a polygon is expanded by Polygons are rejected based on area tolerance in intersection - they have to be greater than tol * tol which is 100mm2, surface matching uses a linear tolerance of 0.01, 10mm. If a polygon is rejected because it is less than 100mm2 it is possible that vertices in that polygon could be > 10mm apart and hence surface matching could fail under certain circumstances. Currently, if two identical surfaces are intersected their vertices are not changed by the intersection method, which makes perfect sense. However if two almost identical surfaces are intersected and the resulting difference between them is too small to be considered then the original surface vertices are not changed either even though they are slightly different. These two surfaces are therefore considered to be identical after intersection is performed but may not be considered identical as far as surface matching is concerned (because they are not identical) To eliminate this error intersection should always return two identical surfaces that will be determined to be equal when passed to circularEqual |
This reverts commit 60268fe.
…n also do a circularEqual check to make sure these two surfaces will macth correctly
I resolved the two test fails. One of them was caused by adding the circularEqual test in Surface intersect. The circulaEqual is removing inline points and so the count of vertices dropped from 6 to 4. This may be an unwanted artifact so to be prudent it was removed |
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.
Some minor changes, otherwise everything looks good!
…nce th eorder the polygons are generated in such that newPolygons[0][ and newPolygons[1] are swapped
Co-authored-by: Julien Marrec <[email protected]>
Yeah my bad :( Co-authored-by: Julien Marrec <[email protected]>
I presume nameString() calls name().value()? I will use nameString() in future - less typing Co-authored-by: Julien Marrec <[email protected]>
Co-authored-by: Julien Marrec <[email protected]>
https://github.com/NREL/OpenStudio into gerard/4372_Intersection-issue-found-using-create-bar
…rent surfaces all work
Co-authored-by: Julien Marrec <[email protected]>
Co-authored-by: Julien Marrec <[email protected]>
CI Results for 3867acb:
|
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.
@jmarrec Thanks for reviewing. Just running the windows CI again and if that passed I'll merge.
Pull request overview
Fixes an issue revealed by surface matching when small surfaces created in intersection were reduced to 0 area surfaces and discarded. The tolerance used in removeSpikesEx should have been the same value as the amount used to shrink and expand surfaces to remove spikes
PR includes the fix (one line change) a test model and a unit test in Model_Gtest to verify the surfaces are paired correctly
Pull Request Author
src/model/test
)src/energyplus/Test
)src/osversion/VersionTranslator.cpp
)Labels:
IDDChange
APIChange
Pull Request - Ready for CI
so that CI builds your PRReview Checklist
This will not be exhaustively relevant to every PR.