Feature/robot object #74
Conversation
docs/_images/nurbs.jpg
Outdated
There was a problem hiding this comment.
image update of the documentation.
docs/examples/color/blending.py
Outdated
There was a problem hiding this comment.
just the example code correction. not the review focus.
docs/examples/color/colors.py
Outdated
There was a problem hiding this comment.
just the example code correction. not the review focus.
There was a problem hiding this comment.
This file should be the focus of the review. Following the logic of compas_robots, we create the robot being viewed in the viewer. related to @gonzalocasas and compas-dev/compas_robots#2
There was a problem hiding this comment.
the example file for the RobotModelObject class.
|
|
||
| for obj in objs: | ||
| if isinstance(obj, CollectionObject): | ||
| [sort(item) for item in obj.items] |
There was a problem hiding this comment.
If you do this, doesn't it mean the objects of the collection are then drawn separately instead of the combined one, hence not benefit from the performance boost of using single combined buffer?
There was a problem hiding this comment.
We need this sorting function to match the category of the object and the corresponding shader. If the CollectionObject contains different objects, i.e. a mesh with a tag, the code will fail if we don't sort them.
Yet, this function is cached by lru_cache, meaning it will not be called every frame if the inputs are the same.
There was a problem hiding this comment.
I don't think you get my point yet. In the past in view2, the Collection object is not just a group. It was created more for performance reasons. When you have many separated objects, because they all have independent buffers, they are rendered sequentially rather than in parallel in GPU. The reason to have CollectionObject is to join seperate object buffers (points, lines, faces) into a one big buffer. That's what _read_xxx_data in CollectionObject is doing, so they can be feed into GPU in one go. We were also able to combine different object types because almost all objects inherented from BufferObject except TextObject. The way you are using it now is taking the sub-objects out and skipping the buffer combination.
|
|
||
| return mesh_object | ||
|
|
||
| def update(self, joint_state: Optional[Configuration] = None, update_buffers: bool = False): |
There was a problem hiding this comment.
I feel a bit strange here to allow to supply a joint_state into the update function. This makes it misaligned from the update of other objects. I would create a separated function and call it update_joints
|
Since @tomvanmele suggested we put the there are still some very small changes in this pr so I will just go ahead and merge it. #77 and #76 are created separately for future improvement. |
Major Changes
RobotModelObjectand its example in the documentation. theRobotModelObjectclass is the main focus of this PR.Minor Changes
CollectionObject.