-
Notifications
You must be signed in to change notification settings - Fork 231
Add KR10 R900-2 from Agilus-2 series #162
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
Add KR10 R900-2 from Agilus-2 series #162
Conversation
|
Sorry for bothering you @gavanderhoorn , I'd love to ping/request somebody else for load distribution but ... Do you know why the test is failing and would you mind to review? |
|
So you asked about rebase merging to avoid creating merge commits in open PRs? :) |
I only see 4 checks that succeeded? |
|
I'm underwhelmed by github ... I just clicked that button ... will do a proper rebase manually :-D |
f21131e to
34a4892
Compare
gavanderhoorn
left a comment
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.
Support package looks good. 👍 for that.
The meshes seem a bit large though. Especially link_2 and link_4. They're almost 1.5MB each. I'd recommend to sub sample them a little bit (using Blender fi, with the decimate modifier).
| <node name="joint_state_publisher" pkg="joint_state_publisher" type="joint_state_publisher"> | ||
| <param name="use_gui" value="true" /> | ||
| </node> |
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.
do we still want to support Kinetic? If not: we should probably move to using JSP GUI here.
Or we could migrate everything at once in a single PR.
I'm ok with either.
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.
currently there is only indigo-devel but I would be fine with opening a melodic-devel (opw etc)
kuka_kr10_support/package.xml
Outdated
| </p> | ||
| </description> | ||
| <author email="[email protected]">Christopher Schindlbeck (University of Hanover)</author> | ||
| <author email="[email protected]">Simon Schmeisser (isys vision GmbH)</author> |
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.
Would this need to be updated?
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.
You're well informed! Optonic is the legal successor (and Isys Vision still a brand name) so nothing bad about the old name but you're right, yes
| <xacro:include filename="$(find kuka_resources)/urdf/common_constants.xacro"/> | ||
| <xacro:include filename="$(find kuka_resources)/urdf/common_materials.xacro"/> |
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.
might want to investigate moving these two lines inside the macro def.
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.
does that make a difference?
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. Newer versions of xacro support macro scope.
|
I'm amazed once more by how much better the decimate algorithm in blender works than the quadric edge collapse in meshlab. I need to figure out how to include that in our post processing scripts |
|
Sorry for the noise, I'll squash all "Fixup" commits to the second commit after review |
0dcf2c1 to
8df7f4b
Compare
8df7f4b to
2ba615f
Compare
gavanderhoorn
left a comment
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.
Looks good. 👍
thanks for iterating.
|
I think we can ignore the CI failure of |
This adds the KR10 R900-2 from Agilus-2 series. Despite its similar name it has quite a different shape and color when compared to the KR10 R900 sixx(KR6 R900 sixx) as introduced in #134