Closed pawelbaran closed 6 months ago
Thanks for the review @FraserGreenroyd, all comments responded 👍
@BHoMBot check required
@BHoMBot check compliance @BHoMBot check core @BHoMBot check null-handling
@BHoMBot check compliance @BHoMBot check core @BHoMBot check null-handling
I got an error when splitting a more complex polygon. Here's the test script: TestPolygonSplit.zip
If it's because the cutting line is too short, we probably should make the error say that (or add the useInfinite
input parameter which would be really useful as I wished it existed on the current Split method!).
Thanks @vietle-bh for the review, well spotted! I have raised #3289 to capture the issue, then fixed it in this PR, also included that case in test files linked to this PR.
Finally, I made use of your polygon to stress test the code, with satisfying results 😃
@BHoMBot check required
One more edge case fix pushed + test added to the test files, the code should exactly as it did just more robust, all tests passing 👍
@BHoMBot check required
Thanks @vietle-bh for the review, well spotted! I have raised #3289 to capture the issue, then fixed it in this PR, also included that case in test files linked to this PR.
Finally, I made use of your polygon to stress test the code, with satisfying results 😃
I use the existing Split method intensively in a recursive method for a custom tool. The new Split method in this PR hasn't failed for the last several days, and seems to have resolved https://github.com/BHoM/BHoM_Engine/issues/3270 👍
Another great thing I got from this PR is a way to break down self-intersecting polylines. Thanks @pawelbaran 😍 Out of interest, what did everyone previously use for breaking down self-intersecting polylines?
Another great thing I got from this PR is a way to break down self-intersecting polylines. Thanks @pawelbaran 😍 Out of interest, what did everyone previously use for breaking down self-intersecting polylines?
Guess what I'm really asking is which way is most performant 😋
Another great thing I got from this PR is a way to break down self-intersecting polylines. Thanks @pawelbaran 😍 Out of interest, what did everyone previously use for breaking down self-intersecting polylines?
I love it!!! Self-intersecting polylines has always been one of the most annoying and not fully solved geometrical problems for me. So can't answer your question, but have never thought of such application, this is brilliant ❤️
Great teamwork! By the way, the Split method returns that generic "internal error" message if I feed it the self-intersecting line. It would be great to have it check for self-intersection and give a more specific message.
Great teamwork! By the way, the Split method returns that generic "internal error" message if I feed it the self-intersecting line. It would be great to have it check for self-intersection and give a more specific message.
What do you mean by self-intersecting line? The outline?
What do you mean by self-intersecting line? The outline?
Yes, the "outerRegion" input polyline.
I added more preliminary checks to the Split
method, also included these cases in the test file @vietle-bh - should be all good now.
@FraserGreenroyd FYI I added IsNull
overloads in BHoM_Engine and cleaned up the code a bit over there, could be worth you having a quick look 👍
@BHoMBot check required
@BHoMBot check required
@BHoMBot check copyright-compliance @BHoMBot check dataset-compliance @BHoMBot check unit-tests
@BHoMBot this is a DevOps instruction. I am requesting neutral checks on: unit-tests
@BHoMBot check ready-to-merge
NOTE: Depends on
This PR will only work if merged into https://github.com/BHoM/BHoM_Engine/pull/3280 - let the latter get merged first to then get this one rebased.
Issues addressed by this PR
Closes #3260 Closes #3282 Closes #3284 Closes #3285 Closes #3289 Closes #3270 (@vietle-bh please provide test file)
Test files
On SharePoint
Changelog
Additional comments
@FraserGreenroyd could you please have a look at
Split
as the original overviewer? It should be more robust now, edge cases ironed out @michal-pekacki @adam-sobieski please reviewOutlinesFromLines
- this is for your use mainly @vietle-bh please test this PR against the issue you are having in #3270I would like to skip UTs in this PR and do them all in one batch under https://github.com/BHoM/BHoM_Engine/issues/3277 once @peterjamesnugent is back, how does that sound @FraserGreenroyd?