Geometry_Enigne: Full refactor of polyline offset #3394
Conversation
… of adjecent parallel segments to its own method and remove method with multiple lists
…edgecases handled
…ion attribute Also some additional descriptions
Method growing in complexity, and wanting to enable more powerful features leads to better to migrate the method out to separate method, and for the Offset method to call this method instead.
First gets out the distance to the segment planes, then order by distance, then sort based on required offset, then check until a segment that is actually intersected is found
|
@BHoMBot check compliance |
|
@IsakNaslundBh to confirm, the following actions are now queued:
There are 51 requests in the queue ahead of you. |
|
After chat with @pawelbaran have fixed the comments and agreed on the following:
|
|
@BHoMBot check compliance |
|
@IsakNaslundBh to confirm, the following actions are now queued:
|
There was a problem hiding this comment.
I have gone through the code again and explored the test script - all looks good, I think the PR is ready to merge from the code perspective 👍
@IsakNaslundBh what do you think about adding UTs based on the test script you provided? That would be a cherry on top
|
@BHoMBot check unit-tests |
|
@IsakNaslundBh to confirm, the following actions are now queued:
|
|
@IsakNaslundBh to confirm, the following actions are now queued:
|
|
The check |
|
The check |
There was a problem hiding this comment.
Happy to finally approve, thank you @IsakNaslundBh for this intellectual adventure! 😃
|
@BHoMBot check ready-to-merge |
|
@pawelbaran to confirm, the following actions are now queued:
|
IsakNaslundBh
deleted the
Geometry_Enigne-#3235-RefactorPolylineOffset
branch
NOTE: Depends on
BHoM/BHoM#1632
Issues addressed by this PR
Closes #3235
also should resolve all issues for polylines in #1333. Leaving that issue open though, as PolyCurves are not handled by this PR.
Full refactor of Polyline offset.
Test files
On SharePoint
Changelog
Additional comments
Started this off as a little side project, that ended up as a quite massive PR, as I went through and thought of more and more edge cases to be handled. Think this now, thoguh, should be really quite stable as well as quick and able to handle many many weird and wonderful edge cases.
The main offset method could for some cases return ever so slightly different results compared to before, but I am pretty confident that what it doing now is more sane and reliable, and gives better control over the behaviour.
Logic I have put in place (by default) is for the offset curve once a self-intersection occurs, is to keep the part with largest area for a closed curve, and always keep the start and end bits for an open curve. Could ofc be changed to longest length or something like that instead, but area, and start and end felt the most sensible to me. as a starting point.
A lot of the code gets a wee bit complex in places, and there is quite a lot of vector maths going on that I have tried to comment as much as possible. Happy to have a chat through it though.