-
Notifications
You must be signed in to change notification settings - Fork 6
Added 4 methods for extracting meshes from links in RobotModel
#25
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
Conversation
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.
comments are from me and @gonzalocasas
src/compas_robots/model/robot.py
Outdated
# Note: the MeshDescriptor.meshes object supports a list of compas meshes. | ||
if isinstance(shape, MeshDescriptor): | ||
# There can be multiple mesh in a single MeshDescriptor | ||
for mesh in shape.meshes: |
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.
there is a LinkGeometry._get_item_meshes
that can be used to handle this, including any primitive shapes.
see example usage in
meshes = LinkGeometry._get_item_meshes(item) |
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.
Yes, I am aware but I don't like that code path with the coerce to tuple thing.
And I think my code is more extendable if in the future other Descriptors are supported.
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.
the code currently doesn't support primitives (i.e. proxies of Sphere, Box etc).
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.
right, this is a comment right? Or?
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.
To me it would make sense to cover all kinds of link geometries already in this PR, or could you explain why you focus on MeshDescriptor
and see all others as future extension?
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.
Oh, so you mean by using meshes = LinkGeometry._get_item_meshes(item)
we can cover the other Descriptors too?
Okay, we can do that too.
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.
Fixed.
@chenkasirer I guess I resolved everything ? |
…shes and also remove meshes_at_link_origin
I have changed my mind regarding the Note that this transformation is indeed slow on large visual meshes, so application side should take care of not over doing it. Note to future self: Use |
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
@yck011522 thanks for taking care of all the review comments~
I might create an issue or two for follow up, don't want to hold this PR any longer.
@chenkasirer Cool. Thanks |
As part of my effort in updating compass fab, particularly related to a two new features
I have come to the repeated need of extracting visual and collision meshes from each
Link
in aRobotModel
, therefore I have introduced these four methods in theRobotModel
class for doing that. This kind of methods are lacking in the current class. Consider the fact that it is not straightforward to navigate the nested URDF elements inside a Link, I think it is reasonable to provide such method for both users and developers.Four methods in total:
RobotModel.get_link_visual_meshes
RobotModel.get_link_collision_meshes
RobotModel.get_link_visual_meshes_joined
RobotModel.get_link_collision_meshes_joined
I'm in favor of separating the
_visual
and_collision
instead of having a single function with a boolean input to indicate visual vs collision, this is up for discussion.A few notes for the future:
Visual
and a list ofCollision
elements. The list can also be empty.link.visual
andlink.collision
are actually a list.element.geometry.shape
is a list of shapes that can beMeshDescriptor
,CapsuleProxy
,SphereProxy
,CylinderProxy
orBoxProxy
.MeshDescriptor
because it is very rare that others are used in existing packages for industrial robot, or used in custom tools build by users. However other shapes also have.mesh
method to return mesh representation, if there is a need to include them in the future.What type of change is this?
Checklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.CHANGELOG.md
file in theUnreleased
section under the most fitting heading (e.g.Added
,Changed
,Removed
).invoke test
).invoke lint
).