-
Notifications
You must be signed in to change notification settings - Fork 45
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
Physical_oM: Formalise Density management #1443
Physical_oM: Formalise Density management #1443
Conversation
…and Str implement it Note there is also the LCADensity fragment in LCA_oM. AS it currently does not implement IMaterialProperty I left it out for now. One to potentially be rejigged and refactored. @michaelhoehn
…Physical Material
…the type is not found
Added to allow easier use from the UIs
Thanks for comments @FraserGreenroyd , all good and Valid. Should have made this Draft when raising, mistake by myself, so potential kind of major changes upcoming (and might even delete the objects you have commented on). Have turned this to draft now, but if object are kept, I will make sure to apply fixes related to you comments! :) |
@IsakNaslundBh ah, no worries! Ignore what's not relevant, lemme know when it's ready for review again and I can take another look if you need/want me to 😄 |
@BHoMBot check compliance |
@IsakNaslundBh to confirm, the following actions are now queued:
There are 11 requests in the queue ahead of you. |
@IsakNaslundBh just to let you know, I have provided a |
@IsakNaslundBh to confirm, the following actions are now queued:
|
@IsakNaslundBh just to let you know, I have provided a |
@BHoMBot check versioning |
@IsakNaslundBh to confirm, the following actions are now queued:
|
@IsakNaslundBh to confirm, the following actions are now queued:
There are 8 requests in the queue ahead of you. |
@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.
Thanks @IsakNaslundBh for this.
Looking good now. Happy with code review now and further future compliance issues raised.
(I also raised #1445 regarding possible review and improvement of name given to IDensityProvider
- saying that, I could not immediately think of much better - so perhaps after discussion we simply close that issue!! 😄 )
However importantly testing creation and manipulation of Physical Materials and their new Density Property is working as expected in my testing this end.
I am still having issues with the provided test scripts - which is weird - so will not approve and merge right now.
@IsakNaslundBh - grab a quick chat tomorrow morning on the test scripts to see what might be going on there! |
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 to approve this now based on my previous functionality review and comments above
Issue with test scripts was resolved (un related issue to do with logs)
Have adressed comments made in review, and getting offline approval from @fraser to dismiss this review on approval.
@BHoMBot check ready-to-merge |
@al-fisher to confirm, the following actions are now queued:
|
Issues addressed by this PR
Closes #1442
Adding IDesityProvider interface and making current materials in Env and Str implement it
Note there is also the LCADensity fragment in LCA_oM. AS it currently does not implement IMaterialProperty I left it out for now. One to potentially be rejigged and refactored. @michaelhoehn
Test files
https://burohappold.sharepoint.com/:f:/s/BHoM/Eg7Y5NfGykJKr5Bc1wfki6ABOXhTCqmW-pr3oNrYvFzKYA?e=tJxtni
Changelog
Additional comments
This should make 0 difference for the Environmental and structural materials in terms of their current functionality on main when used in discipline workflows. What this will enable is to in a more formalised why be able to extract and set density to a physical material.