Geometry_Engine: Split bugs fixed and OutlinesFromLines method added

[pawelbaran]

NOTE: Depends on

This PR will only work if merged into #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 review OutlinesFromLines - this is for your use mainly
@vietle-bh please test this PR against the issue you are having in #3270

I would like to skip UTs in this PR and do them all in one batch under #3277 once @peterjamesnugent is back, how does that sound @FraserGreenroyd?

[FraserGreenroyd]

Ok so I've reviewed the code and run a test and here be some feedback.

Comments on the code have been left at their relevant locations, I don't have any overarching comments regarding the algorithms of themselves at this time so no additional comments there.

On the test however, unfortunately, what's in this PR doesn't work for the original implementation of Split 😄

These are 2 models which split happily under the old code.

However on this branch I get this helpful error message 😉 (see code comment for my comments on the UX of this error message 😋 ).

So on the basis of the test alone I'm afraid I need to request changes cause it no longer works but did previously 😄 I will upload my model to the linked folder as well for others.

[pawelbaran]

Thanks for the review @FraserGreenroyd, all comments responded 👍

[pawelbaran]

@BHoMBot check required

[bhombot-ci]

@pawelbaran to confirm, the following actions are now queued:

• check code-compliance
• check documentation-compliance
• check project-compliance
• check core
• check null-handling
• check serialisation
• check versioning
• check installer

[vietle-bh]

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?

[vietle-bh]

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 😋

[pawelbaran]

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 ❤️

[vietle-bh]

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.

[pawelbaran]

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?

[vietle-bh]

What do you mean by self-intersecting line? The outline?

Yes, the "outerRegion" input polyline.

[pawelbaran]

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 👍

[michal-pekacki]

Tested OutlinesFromLines and it works as it should, happy to approve 👍

[pawelbaran]

@BHoMBot check required

[bhombot-ci]

@pawelbaran to confirm, the following actions are now queued:

• check code-compliance
• check documentation-compliance
• check project-compliance
• check core
• check null-handling
• check serialisation
• check versioning
• check installer

[pawelbaran]

@BHoMBot check required

[bhombot-ci]

@pawelbaran to confirm, the following actions are now queued:

• check code-compliance
• check documentation-compliance
• check project-compliance
• check core
• check null-handling
• check serialisation
• check versioning
• check installer

There are 1 requests in the queue ahead of you.

[bhombot-ci]

The check installer has already been run previously and recorded as a successful check. This check has not been run again at this time.

[michal-pekacki]

Happy to re-approve 👍

[pawelbaran]

@BHoMBot check copyright-compliance
@BHoMBot check dataset-compliance
@BHoMBot check unit-tests

[bhombot-ci]

@pawelbaran to confirm, the following actions are now queued:

• check copyright-compliance
• check dataset-compliance
• check unit-tests

[FraserGreenroyd]

@BHoMBot this is a DevOps instruction. I am requesting neutral checks on: unit-tests

[bhombot-ci]

@FraserGreenroyd I have provided neutral checks to the checks requested. These checks will need to be run properly to obtain full results.

[FraserGreenroyd]

@BHoMBot check ready-to-merge

[bhombot-ci]

@FraserGreenroyd to confirm, the following actions are now queued:

• check ready-to-merge