-
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: implement GeometryHash #2929
Conversation
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.
Hey @alelom - looking good!
See comments below regarding PolySurface
Co-authored-by: Al Fisher <al.fisher@burohappold.com>
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.
A few additional comments
Co-authored-by: Isak Näslund <isak.naslund@burohappold.com>
Co-authored-by: Isak Näslund <isak.naslund@burohappold.com>
Co-authored-by: Isak Näslund <isak.naslund@burohappold.com>
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.
Happy with this now. Have made a few tests:
- Different line representations (line, polyline, nurbscurve with two ctrlPts) as well as Polycurves consisting of the above all coming out the same
- AS above for polylines working fine
- Moving Complex NurbsSurface and checking the hash, works fine
- Moving a Panel and checking the result
- UT covers most base cases.
Code looks sensible and have had quite deep discussion about the implementation for most types.
Approved
@IsakNaslundBh to confirm, the following actions are now queued:
There are 19 requests in the queue ahead of you. |
@BHoMBot check required |
@IsakNaslundBh to confirm, the following actions are now queued:
There are 6 requests in the queue ahead of you. |
The check |
The check |
@BHoMBot check required |
@BHoMBot check required |
@IsakNaslundBh to confirm, the following actions are now queued:
|
The check |
@IsakNaslundBh 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.
Null handling correctly added and checked for.
Re-approving.
Null-handling failure is due to this PR being behind main, where another method has been fixed and merged in. Will dispensate as this is the only failure. Will also dispensate code-compliance as agreed with @FraserGreenroyd in thread above. |
@BHoMBot check ready-to-merge |
@IsakNaslundBh to confirm, the following actions are now queued:
There are 6 requests in the queue ahead of you. |
FAO: @FraserGreenroyd The check they wish to have dispensation on is null-handling. If you are providing dispensation on this occasion, please reply with:
|
FAO: @FraserGreenroyd The check they wish to have dispensation on is code-compliance. 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. 9549259727 |
@IsakNaslundBh I have now provided a passing check on reference |
@BHoMBot this is a DevOps instruction. I am authorising dispensation to be granted on check ref. 9549222836 |
@IsakNaslundBh I have now provided a passing check on reference |
Issues addressed by this PR
Closes #2928
Test files
https://burohappold.sharepoint.com/:f:/s/BHoM/EjL3UGZ1dPNJtyv8RSTZsLYBktwHrwDwRjxsNqh6aJPQrw?e=EDxHzY
Changelog
Additional comments