feat(rhino): DUI3-142 adds render materials to objects on send and receive #63
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #63 +/- ##
========================================
- Coverage 7.98% 7.97% -0.01%
========================================
Files 229 230 +1
Lines 4397 4401 +4
Branches 497 497
========================================
Hits 351 351
- Misses 4035 4039 +4
Partials 11 11 ☔ View full report in Codecov by Sentry. |
Connectors/Rhino/Speckle.Connectors.Rhino7/HostApp/RhinoMaterialManager.cs
Outdated
Show resolved
Hide resolved
|
|
||
| // get physically based render material | ||
| Material pbMaterial = material; | ||
| if (!material.IsPhysicallyBased) |
There was a problem hiding this comment.
I'm a noob on Rhino materials. Why do we need to check it like this?
There was a problem hiding this comment.
Maybe you can show me later on Rhino
There was a problem hiding this comment.
|
|
||
| int matIndex = doc.Materials.Add(rhinoMaterial); | ||
|
|
||
| // POC: check on matIndex -1, means we haven't created anything - this is most likely an recoverable error at this stage |
There was a problem hiding this comment.
or does it mean we have it already with the same name? This is also something we need to check if you haven't already. "What will happen if we have the materials with the same name already?"
There was a problem hiding this comment.
Any material with the same name should've been purged already
| var currentDoc = RhinoDoc.ActiveDoc; // POC: too much right now to interface around | ||
| foreach (Material material in currentDoc.Materials) | ||
| { | ||
| if (!material.IsDeleted && material.Name.Contains(namePrefix)) |
There was a problem hiding this comment.
No action needed: later we should handle these checks with UserDictionaries instead of messing with the name of materials.
|
I guess we do not receive again if the material name is "default" which seems to me a bit fragile to have it with that simple naming. Maybe it is worth to consider add prop to render material object as |
not necessarily - depends on the expected behavior when receiving default materials in other applications. Eg the default material in rhino != default material in autocad, i'd rather receive the rhino default material in autocad rather than set incoming objs to the autocad default |
clairekuang
deleted the
DUI3-142-Basically-do-the-block-thing-in-connectors-x-y-z
branch
Adds render materials to atomic objects on send and receive.
Behavior notes:
MaterialSourceattribute, which by default is set toByLayer. This pr sends arenderMaterialIdon every object, even if it is inheriting its material. This means that on receive, theMaterialSourceproperty is not preservedMaterialSourceproperty on receive is set toByObjectif a validrenderMaterialIdis found on the object, orByLayerif no material is found (default material is used in this case)baseLayerName(same as for block definitions). They are named with the same convention as block definitions.TODO: add render materials to layers