Skip to content

Changing to using BHoM join instead of RhinoJoin when converting polycurves to avoid Rhino Crash #221

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

Conversation

IsakNaslundBh
Copy link
Contributor

@IsakNaslundBh IsakNaslundBh commented Apr 11, 2022

NOTE: Depends on

Issues addressed by this PR

Closes #220

Using BHoM Join instead of Rhino join to check if the curve can be coverted.

This is to avoid full blown crashes of Rhino from happening that can for some cases be triggered by the Rhino.Join method that was used.

Test files

https://burohappold.sharepoint.com/:f:/r/sites/BHoM/02_Current/12_Scripts/02_Pull%20Request/BHoM/Rhinoceros_Toolkit/%23221-AvoidCallingRhinoJoinToStopRhinoCrashWhenConvertingPolycurves?csf=1&web=1&e=UMB25Z

Changelog

Additional comments

@IsakNaslundBh IsakNaslundBh added the type:bug Error or unexpected behaviour label Apr 11, 2022
@IsakNaslundBh IsakNaslundBh self-assigned this Apr 11, 2022
Copy link
Contributor

@FraserGreenroyd FraserGreenroyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not yet tested this Pull Request, only performed a code review.

My request for changes relate to the spelling on the comments and request for a date.

My other comments are for discussion, happy for them to be shot down as 'invalid' and resolved if appropriate.

@FraserGreenroyd
Copy link
Contributor

@BHoMBot check compliance

@bhombot-ci
Copy link

bhombot-ci bot commented Apr 22, 2022

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

  • code-compliance
  • documentation-compliance
  • project-compliance
  • branch-compliance
  • dataset-compliance
  • copyright-compliance

There are 1 requests in the queue ahead of you.

@FraserGreenroyd
Copy link
Contributor

@BHoMBot check required

@bhombot-ci
Copy link

bhombot-ci bot commented Apr 22, 2022

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

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

@bhombot-ci
Copy link

bhombot-ci bot commented Apr 22, 2022

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
Copy link

bhombot-ci bot commented Apr 22, 2022

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.

Copy link
Contributor

@FraserGreenroyd FraserGreenroyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks fine, test script looked ok (didn't dig into it too deep). Happy to merge this to alpha for further testing by alpha users before we go to the next beta.

@FraserGreenroyd
Copy link
Contributor

@BHoMBot check ready-to-merge

@bhombot-ci
Copy link

bhombot-ci bot commented Apr 22, 2022

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

  • ready-to-merge

@bhombot-ci
Copy link

bhombot-ci bot commented Apr 22, 2022

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 ready-to-merge.

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

@FraserGreenroyd
Copy link
Contributor

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

@bhombot-ci
Copy link

bhombot-ci bot commented Apr 22, 2022

@FraserGreenroyd I have now provided a passing check on reference 6129722289 as requested.

@FraserGreenroyd FraserGreenroyd merged commit a2e027e into main Apr 22, 2022
@FraserGreenroyd FraserGreenroyd deleted the Rhinoceros_Toolkit-#220-AvoidCallingRhinoJoinToStopRhinoCrashWhenConvertingPolycurves branch April 22, 2022 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Error or unexpected behaviour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Calling Rhino.Join from convert to Polycurve sometimes leads to a full Rhino crash
2 participants