-
Notifications
You must be signed in to change notification settings - Fork 152
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
Update visual meshes to collada file #117
Update visual meshes to collada file #117
Conversation
@gavanderhoorn and @Jmeyer1292, Are either of you available to review this PR? |
Logistical: could you please rebase instead of merge? There is no history to preserve here and it makes the history of the repository much easier to follow without all those extra merge commits everywhere. |
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.
Haven't checked the meshes themselves yet, so this is only a partial review.
</geometry> | ||
<material name="abb_orange"> | ||
<color rgba="1 0.43 0 1"/> | ||
</material> |
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.
Don't the collision meshes get their colours from this material definition?
It's not needed for the visual
meshes, but might be nice to keep around (might be even nicer to use the material definitions in abb_resources
, but we'd first have to migrate the pkg to this repository from the experimental one).
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 collision models have a yellow material definition, but when viewing the collision model in rviz it is red.
It's not needed for the visual meshes, but might be nice to keep around (might be even nicer to use the material definitions in abb_resources, but we'd first have to migrate the pkg to this repository from the experimental one).
I think it would be good to migrate the abb_resources pkg. I will create PR add it to here and removing it from abb_experimental.
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.
I think it would be good to migrate the abb_resources pkg. I will create PR add it to here and removing it from abb_experimental.
yes, agreed.
If you first remove it from abb_experimental
, and then add it to abb
(while keeping a copy of the files around), we can refer to the removal commit hash in the addition commit. A bit of a poor mans move across two repositories.
</geometry> | ||
<material name="abb_orange"/> |
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.
Same here (and all the rest below).
03f55a1
to
aef31bd
Compare
Is using the GitHub squash and merge feature ok? |
@gavanderhoorn, Is this PR ready to be merged? |
re: squash & merge: that is probably ok. I'll test the changes on my Linux machine tomorrow. |
Sound good. I used the original stl meshes and using blender added the color, then exported it as a dae. |
Just did a visual inspection. Meshes look ok. Did you do the all additional steps in Blender? I haven't compared with the old meshes, but I saw some sharp edges here and there which made me wonder. |
Yea, I imported the stl, then ran the modifier edge split, smooth shading, and applied the correct colors. |
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.
Visual inspection: looks ok.
I like that the meshes haven't increased too much in size (approx 650KB -> 950KB). |
This PR update the visual meshes using a dae files with correct colors for the 2400 and 5400 to address issue #101.