search

compas-dev / compas

Core packages of the COMPAS framework.
https://compas.dev/compas/
MIT License
316 stars 109 forks source link

blender Robotmodelartist does not create collections correctly #759

Closed robin-gdwl closed 3 years ago

robin-gdwl commented 3 years ago

The function draw_geometry from the compas_blender.artists.RobotModelArtist creates a new collection every time it is called. Since the method create() of the base_artist calls draw_geometry for every mesh in the urdf file the blender artist creates a new collection each time (at least thats how I understand it). In addition: the mesh is also not put into the collection that was created.

Location of the draw_geometry call in the base artist: https://github.com/compas-dev/compas/blob/416e490caf4bfb11a47d8e208d19f04e1d76eb16/src/compas/robots/base_artist/_artist.py#L153

blender Robotmodelartist method: https://github.com/compas-dev/compas/blob/416e490caf4bfb11a47d8e208d19f04e1d76eb16/src/compas_blender/artists/robotmodelartist.py#L42-L44

A possible solution could be to simply check if a collection with the name defined in self.layer already exists and use that one instead of creating a new one.

It appears to me that the blender robotartist is at an early stage in the developement. I might implement a fix for this and create a pull request if a rewrite of the robotartist is not already under way. image

example script tested in Blender 2.9.1 and compas 1.0.0 on Windows 10: 

import compas
from compas.robots import GithubPackageMeshLoader
from compas.robots import RobotModel
import compas_blender
from compas_blender.artists import RobotModelArtist, BaseArtist

compas_blender.clear()
#compas_blender.draw()

compas.PRECISION = '12f'
github = GithubPackageMeshLoader('ros-industrial/abb', 'abb_irb6600_support', 'kinetic-devel')
model = RobotModel.from_urdf_file(github.load_urdf('irb6640.urdf'))
model.load_geometry(github)

artist = RobotModelArtist(model, layer='COMPAS FAB::Example')

artist.draw_visual()
beverlylytle commented 3 years ago

Thanks for using COMPAS and bringing up this issue! I agree that it is a bit strange to have a new collection for every mesh included in the robot and not one collection for the entire robot. We do not have something in the works yet for the blender RobotModelArtist and would welcome your pull request! PS What I find a little more troublesome is that in your script draw_visual is called, but both collision and visual meshes have been drawn. I will make an issue for this.

robin-gdwl commented 3 years ago

Thank you for your answer. I am currently familiarizing myself with the compas codebase. I will create a pull request once I created a solution to the problems.

the draw_visual - problem probably stems from the fact that any geometry loaded into Blender is displayed by default. I would check if the mesh has a colour and make it invisible if it doesnt. Currently, if color is None a white material with full alpha is created. However, Blender does not use the material for the normal display in the viewport, which is why it is visible.

https://github.com/compas-dev/compas/blob/416e490caf4bfb11a47d8e208d19f04e1d76eb16/src/compas/robots/base_artist/_artist.py#L143

https://github.com/compas-dev/compas/blob/416e490caf4bfb11a47d8e208d19f04e1d76eb16/src/compas_blender/artists/robotmodelartist.py#L35-L40

beverlylytle commented 3 years ago

Hi @robin-gdwl Just wanted to check in and see how it's going with eliminating all these excess collections? I'd be happy to help out, or if you find you don't have much time right now, I'd gladly take a stab at it. Let me know.

robin-gdwl commented 3 years ago

@beverlylytle I did manage to get the files to all go into the same collection but not any of the further things (visibility of the collision meshes) I will clean up my code and make a pull request today.

beverlylytle commented 3 years ago

Great! About the visibility of the collision and visual meshes, I've already made some progress on that, so no need to do anything about that.

robin-gdwl commented 3 years ago

While cleaning the code I remembered the main confusion I had during coding: The word "layer" is not ideal when using Blender as blender uses collections to organize geometry. View- Layers in Blender are much closer to "Snapshots" in Rhino than to actual layers. I changed the compas_blender.artists.Robotmodelartist init method to ask for collection as well as layer I did not find any ways this might break something inside the api but scripts created before will not work as the layer parameter will have no effect.

tomvanmele commented 3 years ago

fwiw, we used layer in all Blender artists to be consistent with the nomenclature of Rhino. the layer is treated internally as the name for the collection. if that is confusing, we should consider changing it everywhere. it doesn't make sense to me to only change this for the robot artist...

beverlylytle commented 3 years ago

I don't see where in compas_blender layer is used except in RobotModelArtist. Every other reference is commented out or seems to have been changed to collection.

robin-gdwl commented 3 years ago

@brgcode This is true. I was wondering what would be the priority: consistency with blender vs consistency with other compas features. Feel free to ignore/change this if you think it would create inconsistencies.
The other Blender Artists use the word collections but they don't seem to expose it through function params.

tomvanmele commented 3 years ago

I don't see where in compas_blender layer is used except in RobotModelArtist. Every other reference is commented out or seems to have been changed to collection.

yes you are right. i just noticed this as well. updated my comment on the PR. in the other artists, the base collection is named after the data or data structure name, with implicitly named sub-collections for the various components of the object.

perhaps we should add the option to give the parent collection a name explicitly via the collection parameter.

but we don't need to keep layer for consistency with Rhino because that is not really a goal anymore per se...

tomvanmele commented 3 years ago

@brgcode This is true. I was wondering what would be the priority: consistency with blender vs consistency with other compas features. Feel free to ignore/change this if you think it would create inconsistencies. The other Blender Artists use the word collections but they don't seem to expose it through function params.

in the past i would have said the latter, but lately we are more trying to play to the strengths of the target softwares, so now this would be the former :)

welcome on board by the way :) nice to see people contribute!

robin-gdwl commented 3 years ago

welcome on board by the way :) nice to see people contribute!

Thank you! Although my contribution so far is minimal. I really think this is a worthwhile endeavour as I felt like reinventing the wheel every time I wanted to create code that handles geometry. A flexible and expandable Framework will be a great tool to have.

beverlylytle commented 3 years ago

I really think this is a worthwhile endeavour as I felt like reinventing the wheel every time I wanted to create code that handles geometry. A flexible and expandable Framework will be a great tool to have.

That's really nice to hear!