-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add view capturing capability #230
Add view capturing capability #230
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.
Really nice and easy to use! It took a minute to understand the distinction between a DimensionView and a ScaleView since this functionality is unified in a standard Rhino command line prompt, but I think I'm good with the approach generally. I did notice however that there are some performance quirks when using one or the other when exporting a sample image 5000x5000 (likely larger than standard outputs, but I noticed similar behaviors with anything over 3K).
This is entirely not in our perview as I see it, but good to simply document here it seems capture methodology could have varying nuances between the different types (dimensioned on the left, and scaled on the right).
I'm happy to provide an approving review when needed, but as noted in the PR and in my comments, we need to discuss our decision to leave Rhino 5 behind in support of these newer features.
f375759
to
fd2ee7c
Compare
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.
Very happy with this implementation!
@BHoMBot check compliance |
@FraserGreenroyd to confirm, the following actions are now queued:
There are 27 requests in the queue ahead of you. |
fd2ee7c
to
928d573
Compare
@BHoMBot check compliance |
@michaelhoehn to confirm, the following actions are now queued:
|
@michaelhoehn 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. 10441140689 |
@michaelhoehn I have queued up your request to fix copyright headers. There are 0 requests in the queue ahead of you. |
@michaelhoehn I'm sorry, but I cannot take that instruction from you. As this action would modify this Pull Request, this instruction can only come via an authorised user, per our Code of Conduct for Bots |
@michaelhoehn fix requested for project compliance. The errors with the CSProject ( I will apply the fixes to every case detailed on the checks tab with the exception of any references to the target framework. I am unable to provide fixes to the Target Framework automatically, these will need to be performed manually. If you want to perform the fixes in a different manner please resolve this manually and rerun the check. If you are happy for me to go ahead and perform this action, please reply with:
|
@BHoMBot fix copyright headers ref. 10441140689 |
@FraserGreenroyd I have queued up your request to fix copyright headers. There are 0 requests in the queue ahead of you. |
@FraserGreenroyd I am now going to fix the copyright compliance in accordance with the annotations previously made. |
@FraserGreenroyd to confirm I have now resolved the copyright compliance and pushed a commit to this Pull Request. |
Co-authored-by: Fraser Greenroyd <[email protected]>
@BHoMBot check compliance |
@michaelhoehn to confirm, the following actions are now queued:
|
Co-authored-by: Fraser Greenroyd <[email protected]>
@BHoMBot check null-handling |
@IsakNaslundBh to confirm, the following actions are now queued:
There are 1 requests in the queue ahead of you. |
@BHoMBot check null-handling |
@FraserGreenroyd to confirm, the following actions are now queued:
|
@BHoMBot check compliance |
@FraserGreenroyd to confirm, the following actions are now queued:
|
@FraserGreenroyd to confirm, the following actions are now queued:
|
@BHoMBot check versioning |
@FraserGreenroyd to confirm, the following actions are now queued:
|
@BHoMBot check versioning |
@FraserGreenroyd to confirm, the following actions are now queued:
There are 18 requests in the queue ahead of you. |
I think I've figured out why versioning is failing - installer would too if we ran it - LadybugTools_Toolkit has a dependency on this toolkit, I think this PR is breaking LadybugTools_Toolkit's ability to compile, thus resulting in versioning issues when the methods/objects don't exist. I will take another look when back on the laptop to see if I can compile LadybugTools_Toolkit against this branch and if not, work out what the rectification might need to be early in the week with @tg359 |
@BHoMBot check versioning |
@FraserGreenroyd to confirm, the following actions are now queued:
|
@BHoMBot check installer |
@FraserGreenroyd to confirm, the following actions are now queued:
|
@IsakNaslundBh just to let you know, I have provided a |
@IsakNaslundBh just to let you know, I have provided a |
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.
Now that checks are passing, approving based on previous reviews.
FAO: @FraserGreenroyd The check they wish to have dispensation on is null-handling. 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. 10455686053 |
@FraserGreenroyd I have now provided a passing check on reference |
@BHoMBot check ready-to-merge |
@IsakNaslundBh to confirm, the following actions are now queued:
|
Issues addressed by this PR
Closes #224
Add capability to capture rhino view.
Note that this requires an uptick in the nugets to Rhino 6, as the ViewCapture class used is internal in rhino 5. There are ways around this for rhino 5 as well, but from testing them, they perform significantly worse and does not give the same level of control.
Before this PR is merged an agreement that this uptick is ok, as it means no more support for rhino 5 (as in, might still work, but can not be as relied upon any longer). @al-fisher @FraserGreenroyd , have discussed this with you before raising this, but noting you in here as well.
Test files
Changelog
Additional comments