Geometry_Engine: Fix bug where SortAlongCurve for a Polyline returns duplicate points

[peterjamesnugent]

Issues addressed by this PR

Closes #3104

Test files

https://burohappold.sharepoint.com/:u:/r/sites/BHoM/02_Current/12_Scripts/01_Issue/BHoM/BHoM_Engine/Geometry_Engine/%233104-SortAlongCurve.gh?csf=1&web=1&e=zkOUFi

Changelog

Fixed a bug where a closed Poyline was returning more Points than were being input.

Additional comments

UnitTests only include Ellipse so I have added the Polyline tests.

[peterjamesnugent]

@BHoMBot check required

[bhombot-ci]

@peterjamesnugent 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

[peterjamesnugent]

@BHoMBot check unit-tests

[bhombot-ci]

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

• check unit-tests

[pawelbaran]

Happy to approve the changes because they fix the issue stated. However, I have looked into the code and ended up with a few question marks such as:

• points that do not lie on the curve get culled without even a warning (I would argue they should be sorted based on the closest point on curve, while a warning about being skipped sounds like a bare minimum)
• IsOnCurve is called multiple times, while it could be done only once (pair each point with the closest curve and work with each of such sets independently)

Of course this is just a side comment, out of scope of this PR. But curious about your opinion on the above points @peterjamesnugent

[peterjamesnugent]

@pawelbaran agree, if we are culling we need to make that clear in the description. Given that you can use IsOnCurve prior to using SortAlongCurve, I think we should project points and then sort - then we just need to use IsOnCurve when we go through Polyline.SubParts.

[pawelbaran]

Project the points, but still sort the original input, right? So that the original input points get sorted based on their correspondent points on curve, would that make sense or am I overcomplicating?

[peterjamesnugent]

Yes exactly.

[pawelbaran]

#3108 raised 👍

Meanwhile, happy to get this PR merged as soon as the checks pass.

[FraserGreenroyd]

My comment was addressed, happy to approve for merge.

[FraserGreenroyd]

@BHoMBot check compliance
@BHoMBot check required

[bhombot-ci]

@FraserGreenroyd 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 code-compliance
• check documentation-compliance
• check project-compliance
• check core
• check null-handling
• check serialisation
• check versioning
• check installer

There are 34 requests in the queue ahead of you.

[bhombot-ci]

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]

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.

[FraserGreenroyd]

@BHoMBot check ready-to-merge

[bhombot-ci]

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

• check ready-to-merge

There are 16 requests in the queue ahead of you.

[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.

[bhombot-ci]

FAO: @FraserGreenroyd
@FraserGreenroyd is seeking dispensation on this Pull Request to skip a required check. Please can you provide authorisation for the check to be skipped, or provide assistance as appropriate.

The check they wish to have dispensation on is unit-tests.

If you are providing dispensation on this occasion, please reply with:

@BHoMBot this is a DevOps instruction. I am authorising dispensation to be granted on check ref. 15102708366