Fix for Multiple collections created by Blender RobotmodelArtist. #765
Conversation
- linting - added comment
Use the utility function to create blender collection instead of a new bpy-method
This may be more involved than necessary at this point in the Tutorial, but it should be there for completeness IMO.
went through CONTRIBUTING.md Also ran: invoke clean invoke check invoke lint invoke docs invoke test
| def __init__(self, model, collection=None, layer=None): | ||
| self.view_layer = layer | ||
| self.collection = collection |
There was a problem hiding this comment.
the term layer was introduced here to be consistent with Rhino. internally it is supposed to be used as the name for the collection. if that is confusing we should change this to collection for all blender artists...
There was a problem hiding this comment.
actually i need to take that back.
it seems we already removed this from the other artist in favor of implicitly named collections. perhaps this would be a good time to decide on a global strategy
There was a problem hiding this comment.
I think it makes more sense to use collection and that layer should be removed from RobotModelArtist.
There was a problem hiding this comment.
It would be great if (in the future) one could port a script from Rhino to Blender just by changing the import statement, this would only work if all Robotmodelartists use the same parameter names. However using the word "layer" in the Blender context is generally confusing.
There was a problem hiding this comment.
agreed, but perhaps the solution to this is that we take a step back and use our own conventions, rather than transplanting the conventions from one system onto another. but i imagine that will be part of a lengthy discussion :)
beverlylytle
left a comment
beverlylytle
left a comment
There was a problem hiding this comment.
Looks good! The documentation is nice, especially the formatting. And now the Blender artist will behave as expected. Thanks! Just one or two comments (in particular, with the CHANGELOG.md) to address, and we can merge.
|
Another review comment: I noticed that only one check was run because the default branch name changed since your fork last pulled. So it would be good if you could merge the main branch into your fork. |
sync with remote repository
|
this ready to be merged? |
I think not. @robin-gdwl said he would make one more change to the robot tutorial. |
|
Sorry for the wait, I was creating a section about importing urdf-files and got stuck because importing into Rhino had issues with the unit scale. |
beverlylytle
left a comment
beverlylytle
left a comment
There was a problem hiding this comment.
Looks good! Let's get this merged!
3 participants
Simple fix for the blender RobotmodelArtist.
Responds to this Issue: #759
Changed:
compas_blender.artists.Robotmodelartistwould create a new collection for each mesh and then also not put the mesh iton the created collection.compas_blender.artists.Robotmodelartistto include acollection-parameter instead of alayer-parameter to be more consistent with Blender's nomenclature.compas_blender.utilitiesto create the collection if none exists instead of using a new call to a bpy-method.What type of change is this?
Checklist
CHANGELOG.mdfile in theUnreleasedsection under the most fitting heading (e.g.Added,Changed,Removed).invoke test).invoke lint).