-
Notifications
You must be signed in to change notification settings - Fork 396
Add Basic Materials Functionality #1923
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1923 +/- ##
==========================================
+ Coverage 95.77% 95.80% +0.02%
==========================================
Files 29 29
Lines 7856 7911 +55
Branches 1183 1188 +5
==========================================
+ Hits 7524 7579 +55
Misses 192 192
Partials 140 140 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
adam-urbanczyk
left a comment
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.
If I understand the intent of this PR correctly, you actually want to use XCAFDoc_Material
|
@adam-urbanczyk I need to better understand what you are thinking. The current |
|
In the end this class should wrap NB: it can be used for now to simply store a name. |
|
@adam-urbanczyk How about this? |
|
Ok, let me take a look. |
adam-urbanczyk
left a comment
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.
Looks good in general, but could you implement pickling? If it is too much work you could for now get rid of the vis part.
@adam-urbanczyk I'll see what I can do. |
|
FYI: here is how it is done for cadquery/cadquery/occ_impl/assembly.py Line 145 in fea85a5
|
|
@adam-urbanczyk I tried to bring this PR up to par with the Color class. |
adam-urbanczyk
left a comment
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.
LGTM, but please check if you need all methods.
|
|
||
| return (name, description, density, densityUnit) | ||
|
|
||
| def __hash__(self): |
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.
Do you actually use hash and eq? AFAICT it is not needed for pickiling
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.
They are not needed for pickling, it just seemed worthwhile to add them while I was going through that part of the code.
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.
OK, clear. Thanks!
This is a smaller PR which wraps
Graphic3d_MaterialAspectXCAFDoc_MaterialandXCAFDoc_VisMaterialfrom OCC and leaves open the possibility of adding inphysicalvisualization properties later.