Geometry_Enigne: Full refactor of polyline offset

IsakNaslundBh commented 3 months ago

NOTE: Depends on

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.

Addition of MultiOffset method that is able to generate a series of offsets for a single curve
Make Polyline offset call this new more powerful method
Significant amount of edgecases handled
Several optimisations put in place by caching of data, stored into new internal classes stored in the objects folder
Complete change to methodology for figuring out locations of new nodes etc.

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.

IsakNaslundBh commented 3 months ago

@BHoMBot check compliance

bhombot-ci[bot] commented 3 months ago

@IsakNaslundBh to confirm, the following actions are now queued: - check `code-compliance` - check `documentation-compliance` - check `project-compliance` - check `branch-compliance` - check `dataset-compliance` - check `copyright-compliance` There are 14 requests in the queue ahead of you.

bhombot-ci[bot] commented 3 months ago

@IsakNaslundBh to confirm, the following actions are now queued: - check `code-compliance` - check `documentation-compliance` - check `project-compliance` - check `branch-compliance` - check `dataset-compliance` - check `copyright-compliance` There are 4 requests in the queue ahead of you.

IsakNaslundBh commented 1 month ago

@BHoMBot check compliance

bhombot-ci[bot] commented 1 month ago

@IsakNaslundBh to confirm, the following actions are now queued: - check `code-compliance` - check `documentation-compliance` - check `project-compliance` - check `branch-compliance` - check `dataset-compliance` - check `copyright-compliance` There are 51 requests in the queue ahead of you.

IsakNaslundBh commented 1 month ago

After chat with @pawelbaran have fixed the comments and agreed on the following:

Revert back the method signature to what it was before, and only use the OffsetOptions for the MultiOffset method
Kept the return type for the main Offset method as a single curve. For multi curve return the MultiOffset method is to be used. Can be discussed if the return type should be changed, but that requires alignment with the POlyCurve offset method as well alignment with methods calling this method.

IsakNaslundBh commented 1 month ago

@BHoMBot check compliance

bhombot-ci[bot] commented 1 month ago

@IsakNaslundBh to confirm, the following actions are now queued: - check `code-compliance` - check `documentation-compliance` - check `project-compliance` - check `branch-compliance` - check `dataset-compliance` - check `copyright-compliance`

IsakNaslundBh commented 1 month ago

@BHoMBot check unit-tests

bhombot-ci[bot] commented 1 month ago

@IsakNaslundBh to confirm, the following actions are now queued: - check `unit-tests`

IsakNaslundBh commented 1 month ago

@BHoMBot check compliance @BHoMBot check unit-tests @BHoMBot check required

bhombot-ci[bot] commented 1 month ago

@IsakNaslundBh to confirm, the following actions are now queued: - check `code-compliance` - check `documentation-compliance` - check `project-compliance` - check `branch-compliance` - check `dataset-compliance` - check `copyright-compliance` - check `unit-tests` - check `code-compliance` - check `documentation-compliance` - check `project-compliance` - check `core` - check `null-handling` - check `serialisation` - check `versioning` - check `installer`

bhombot-ci[bot] commented 1 month ago

The check `code-compliance` has already been run previously and recorded as a successful check. This check has not been run again at this time.

bhombot-ci[bot] commented 1 month ago

The check `documentation-compliance` has already been run previously and recorded as a successful check. This check has not been run again at this time.

pawelbaran commented 1 month ago

@BHoMBot check ready-to-merge

bhombot-ci[bot] commented 1 month ago

@pawelbaran to confirm, the following actions are now queued: - check `ready-to-merge`

BHoM / BHoM_Engine