-
Notifications
You must be signed in to change notification settings - Fork 14
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
Messaging tweaked and bug in roof push fixed #1285
Conversation
Oh, please do not review yet @michal-pekacki @vietle-bh, I will have a look at the errors in the pull of panels first - will let you know when ready to review 👍 |
Oke, I have pushed one more commit + added new test file. Ready for review now 👍 |
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.
Thanks @vietle-bh for review. I have addressed all of your comments, should be ready for re-review now |
Great! Which Grasshopper script should I use to test the Push though? |
By the way @pawelbaran , I found a related issue while testing: #1286 |
I have also fixed #1286 @vietle-bh - I hope all covered now 🙈 |
@pawelbaran to confirm, the following actions are now queued:
|
@pawelbaran to confirm, the following actions are now queued:
There are 13 requests in the queue ahead of you. |
@BHoMBot check core |
@pawelbaran to confirm, the following actions are now queued:
|
@BHoMBot check required |
@pawelbaran to confirm, the following actions are now queued:
|
The check |
The check |
The check |
The check |
There're definitely fewer errors now 👍 However, some items fail on another null exception due to nurbs curves in their boundaries. Error 1 already gives enough info so it would be good to maybe hide the second error: |
Thanks @vietle-bh for reviewing - but please stop finding new-old issues that I have not covered! 😃 This one should actually be fixed on the engine side, please see #1287. So when it comes to this PR, I think it should be fine to approve/merge if you do not find any other bits worth fixing before the Beta release. |
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.
Approved based on the conversation above.
@pawelbaran to confirm, the following actions are now queued:
|
Issues addressed by this PR
Closes #1283
Closes #1284
Closes #1286
Test files
On SharePoint
Changelog
Additional comments