-
Notifications
You must be signed in to change notification settings - Fork 14
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_Engine: Do not check zero length segments in IsSelfIntersecting #2410
Geometry_Engine: Do not check zero length segments in IsSelfIntersecting #2410
Conversation
@BHoMBot check unit tests |
@IsakNaslundBh sorry, I didn't understand. |
@BHoMBot Check Unit Tests |
@IsakNaslundBh sorry, I didn't understand. |
@BHoMBot check unit-tests |
@IsakNaslundBh to confirm, |
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.
Given that these changes are now known not to be the cause of the failing unit tests check.
I think this is a good solution.
We should add a unit test for this method - which should not be based on the profile creates but direct calls from all the Curve inputs.
Other methods that call IsSelfIntersecting
directly are:
Normal, Centroid, Offset
All of which have unit tests and these are passing.
OffsetVariable
does not yet have a unit test but I cannot see a problem.
Would also be good to get thoughts from others on other methods that may be indirectly calling IsSelfIntersecting
and possible impact.
In setting up a batch of unit tests for
Should the Polyline method also remove the short(zero) length segments? Also a possible confusion for users is that in Rhino those Curves with zero length segments are marked as Invalid Curves yet they can be handled as valid curves in the BHoM. |
…ting' of https://github.com/BHoM/BHoM_Engine into Geometry_Engine-#2409-AvoidShortCurvesForIsSelfIntersecting
Unit test creation script here |
@BHoMBot check unit-tests |
@rolyhudson to confirm, |
@BHoMBot check unit-tests |
@rolyhudson to confirm, |
@BHoMBot check compliance |
@IsakNaslundBh to confirm, |
@BHoMBot check installer |
@IsakNaslundBh to confirm, |
@BHoMBot check core |
@IsakNaslundBh to confirm, |
@BHoMBot check versioning |
@IsakNaslundBh to confirm, |
@BHoMBot check serialisation |
@IsakNaslundBh to confirm, |
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.
Happy with this now.
Please be advised that the check with reference 2177645870 has more than 50 annotations of notes. API limitations restrict annotations to 50. You may need to rerun this check to obtain the next set when you make changes. At the time of reporting this check, there are 196 additional annotations waiting, made up of 196 errors and 0 warnings. |
@BHoMBot check core |
@FraserGreenroyd to confirm, |
Issues addressed by this PR
Closes #2409
This seem to fix the urgent issue in #2393 , but leaving that issue open as a review of the profile creates will be needed to avoid any zero-length segments to be created in the first place.
Test files
https://burohappold.sharepoint.com/:f:/s/BHoM/EqqGnXiRrSVPkYN2v1QyjoIB51GZm6ShUdu-f_E62Hn-YA?e=X5oILq
Changelog
Additional comments
@LMarkowski @pawelbaran would be good if this could be checked quite thoroughly, to ensure this is not causing any issues elsewhere, and to make sure I have not missed any critical reason why this would not be ok!