Skip to content
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

Experimental support for CRB 15000-12/1.27 #149

Merged
merged 6 commits into from
Sep 22, 2024

Conversation

mmmarcopalma
Copy link
Contributor

@mmmarcopalma mmmarcopalma commented Jul 12, 2024

Extending support for CRB 15000 to model 12-1.27 (GoFa 12 kg).

Includes updates of:

  • /config, /launch, /urdf directories
  • .stl visual and collision mesh geometry
    (link-3, link-5 and link-6 are referenced to meshes/crb15000_5_95)

@mmmarcopalma
Copy link
Contributor Author

/cc @gonzalocasas

@gavanderhoorn
Copy link
Member

Thanks for the PR.

Some high-level comments:

  • could you remove all the desktop.ini files please?
  • could none of the meshes of the other variant be reused?
  • please add a roslaunch test for the new variant
  • please update the description in the manifest with the details about the new variant you added
  • and if you'd like: please add yourself as an author, seeing as you added a new variant

@mmmarcopalma
Copy link
Contributor Author

mmmarcopalma commented Jul 12, 2024

Thanks, I'll follow up on your comments.

  • could none of the meshes of the other variant be reused?

I could re-use 3+3 meshes (links 3-5-6). The ones I uploaded are exact copies of the existing ones.

I'm not fluent yet in git, apologies for missing on some best-practice.

@gavanderhoorn
Copy link
Member

  • could none of the meshes of the other variant be reused?

I could re-use 3+3 meshes (links 3-5-6). The ones I uploaded are exact copies of the existing ones.

Ok, so this is perhaps due to me not being clear by what I meant by reuse.

A xacro:macro can refer to files 'anywhere', so you don't have to copy meshes if they are identical to files for other variants.

Just have your <mesh filename="..."/> point to the file you'd like to reuse from the other variant.

No need to copy anything.

@mmmarcopalma
Copy link
Contributor Author

mmmarcopalma commented Jul 12, 2024

I added a commit with the requested changes.

  • and if you'd like: please add yourself as an author, seeing as you added a new variant

Thanks, I added my name in the Package.xml

@mmmarcopalma
Copy link
Contributor Author

Hello @gavanderhoorn, is there any news regarding this request?
Thanks!

Copy link
Member

@gavanderhoorn gavanderhoorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for the PR. Holiday period got in the way.

See the inline comments for some things that need to be checked and possibly corrected.

Apart from those, I'm wondering whether there is an issue with the normals of visual meshes link_1.stl and link_4.stl:

rviz_screenshot_2024_08_17-17_22_27

they appear to be inverted, at least partially.

abb_crb15000_support/urdf/crb15000_12_127_macro.xacro Outdated Show resolved Hide resolved
abb_crb15000_support/urdf/crb15000_12_127_macro.xacro Outdated Show resolved Hide resolved
@mmmarcopalma
Copy link
Contributor Author

Committed the requested changes.
Thanks for supporting!

@mmmarcopalma
Copy link
Contributor Author

Hello @gavanderhoorn, is there anything else I can do to push this PR?
Thanks!

@gavanderhoorn
Copy link
Member

I've re-enabled CI and added a couple of commits.

I believe this is in an OK state to merge. Thanks again for the PR @mmmarcopalma 👍

Let's wait on CI to turn green.

@gavanderhoorn
Copy link
Member

To prevent all the fixups from ending up in the commit history, I'll squash-merge this.

Provenance and attribution will be retained, of course.

@gavanderhoorn gavanderhoorn merged commit e7ee7b0 into ros-industrial:kinetic-devel Sep 22, 2024
2 checks passed
@gavanderhoorn
Copy link
Member

Thanks again @mmmarcopalma 👍

@mmmarcopalma
Copy link
Contributor Author

mmmarcopalma commented Sep 23, 2024

Great news, thanks for the support @gavanderhoorn, and thanks also to @gonzalocasas for initiating the process

@mmmarcopalma mmmarcopalma deleted the new_branch branch September 25, 2024 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants