Skip to content

support multiple material for one link#812

Closed
k-okada wants to merge 1 commit intoros-visualization:hydro-develfrom
k-okada:support_multiple_material_for_link
Closed

support multiple material for one link#812
k-okada wants to merge 1 commit intoros-visualization:hydro-develfrom
k-okada:support_multiple_material_for_link

Conversation

@k-okada
Copy link
Copy Markdown
Contributor

@k-okada k-okada commented Sep 14, 2014

This is patches for setting multiple materials for one link. I understand this is not clean patch, but anyway send PR and hoping someone will find more better way.

c.f: http://answers.ros.org/question/154816/color-issue-with-urdf-and-multiple-visual-tags/

you need following patch to urdf_parser.

diff --git a/urdf_parser/src/link.cpp b/urdf_parser/src/link.cpp
index b69824a..e324651 100644
--- a/urdf_parser/src/link.cpp
+++ b/urdf_parser/src/link.cpp
@@ -380,7 +380,8 @@ bool parseVisual(Visual &vis, TiXmlElement *config)
     }
   }

-  vis.group_name = std::string("default");
+  //vis.group_name = std::string("default");
+  vis.group_name = vis.material_name;
   const char *group_name_char = config->Attribute("group");
   if (group_name_char)
     logWarn("The notion of a group name for visual tags is not supported by URDF.");

@wjwwood wjwwood added this to the untargeted milestone Sep 19, 2014
@mmoerdijk
Copy link
Copy Markdown

mmoerdijk commented Feb 9, 2017

what is the status on this PR? Is this already implemented?

@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Feb 9, 2017

It's in the untargeted milestone: https://github.com/ros-visualization/rviz/milestone/102

It's probably this way because it requires (or required) a patch in urdf_parser.

@mmoerdijk
Copy link
Copy Markdown

mmoerdijk commented Feb 10, 2017

Thanks for the quick reply @wjwwood , I looked at urdf_parser and it is a bit different now than at the time @k-okada submitted this patch. I think the corresponding code is now here.

If I understand the patch that @k-okada submitted correctly it changes the vis.group_name from default to the material name. But if I look at the current code of the urdf_paser it already uses the matrial name for vis.name (group_name does not exist any more).

I don't have experience with the rviz or de urdfdom code base, but could it be that the urdf_parser already implements the behavior required for this PR? Or am I missing something?

This was referenced Feb 12, 2017
Merged
Merged
@k-okada
Copy link
Copy Markdown
Contributor Author

k-okada commented Feb 12, 2017

@mmoerdijk thank you for your research, See #1079 or #1080 for rewrited version of this PR using material name for vis.name. Please check if this works for you. Note that I totally forget what I did a year ago, so the code is not so confident ,

@mmoerdijk
Copy link
Copy Markdown

mmoerdijk commented Feb 24, 2017

@k-okada Thanks! I somehow totally missed this comment, I will test it on Monday.

@mmoerdijk
Copy link
Copy Markdown

mmoerdijk commented Feb 27, 2017

@k-okada I tested the indigo PR and can confirm it works for me. Thanks again.
@wjwwood could you say what you would need to merge these PR's? And if it is possible to cherry pick it to jade as well.

@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Mar 2, 2017

@mmoerdijk just time on my end. I need to test them out and merge other pr's before doing a release. I'm going to do a round of releases in preparation for ROS Lunar (which should be soonish).

@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Apr 29, 2017

closing in favor of #1079 and #1080

@wjwwood wjwwood closed this Apr 29, 2017
@k-okada k-okada deleted the support_multiple_material_for_link branch March 13, 2018 04:14
@ANogin ANogin mentioned this pull request Nov 20, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@k-okada @mmoerdijk @wjwwood