-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Geometry_Engine: Add shortest path algorithm #3165
Conversation
@BHoMBot check compliance |
@albinber to confirm, the following actions are now queued:
There are 22 requests in the queue ahead of you. |
@albinber fix requested for copyright headers. The errors with the copyright headers on the CS ( I will apply the fixes to every case detailed on the checks tab. If you want to perform the fixes in a different manner please resolve this manually and rerun the check. Each CS ( If you are happy for me to go ahead and perform this action, please reply with:
|
@BHoMBot fix copyright headers ref. 16412106428 |
@albinber I have queued up your request to fix copyright headers. There are 0 requests in the queue ahead of you. |
@albinber I am now going to fix the copyright compliance in accordance with the annotations previously made. |
@albinber to confirm I have now resolved the copyright compliance and pushed a commit to this Pull Request. |
@BHoMBot check compliance |
@albinber to confirm, the following actions are now queued:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welcome to BHoM @albinber - great to have you here! A great first pull request - some small changes to the code structure (my favourite, blank lines 😉 ) and where we put the null
check to protect the while
loop.
Otherwise, code structure looks good and in line with @jamesramsden-bh original issue/pseudo code and works well in Rhino so great job 😄
Co-authored-by: Fraser Greenroyd <[email protected]>
Co-authored-by: Fraser Greenroyd <[email protected]>
Co-authored-by: Fraser Greenroyd <[email protected]>
Co-authored-by: Fraser Greenroyd <[email protected]>
Co-authored-by: Fraser Greenroyd <[email protected]>
Co-authored-by: Fraser Greenroyd <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks good now, thanks @albinber - great work 😄
@albinber to confirm, the following actions are now queued:
|
The check |
The check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @albinber, glad to see more faces contributing! 🚀 🚀 🚀
Starting from apologies: I do not want to spoil the party but... are we sure we are OK with modifying (i.e. emptying) the input collection? That should not happen from what I reckon - can be fixed easily by calling the following in line 42:
List<Point> ringPoints = points.ToList();
Would also be good to add checks against points
being null or empty as well. But besides that, good stuff, can't wait for more! 👍
@albinber to confirm, the following actions are now queued:
There are 33 requests in the queue ahead of you. |
The check |
The check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes make sense in line with comments, good to go.
@BHoMBot check ready-to-merge |
@FraserGreenroyd to confirm, the following actions are now queued:
|
@BHoMBot this is a DevOps instruction. I am requesting neutral checks on: unit-tests |
@FraserGreenroyd I have provided neutral checks to the checks requested. These checks will need to be run properly to obtain full results. |
NOTE: Depends on
Issues addressed by this PR
Closes #2826
Test files
Changelog
Additional comments