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

Add include guard to materials #1637

Merged
merged 1 commit into from
Apr 12, 2023

Conversation

LuAPi
Copy link

@LuAPi LuAPi commented Jan 21, 2021

Prevents material already defined errors when adding multiple types of camera to a robot.

I was making a URDF for the d455 based on the d435 as I have a robot with both on and I ran into a material is not unique error. This fixes that.

I think it would actually be better if the materials were macros like

<xacro:macro name="aluminum_material">
  <material name="aluminum">
    <color rgba="0.5 0.5 0.5 1"/>
  </material>
<xacro:macro name="plastic_material">
    <material name="plastic">
      <color rgba="0.1 0.1 0.1 1"/>
    </material>
</xacro:macro>

Then changing the places they get used from <material name="plastic"/> to <xacro:plastic_material />
That way you don't end up putting very generic names like "aluminum" and "plastic" in the global namespace.

Prevents material already defined errors when adding multiple types of camera to a robot
@doronhi
Copy link
Contributor

doronhi commented Feb 17, 2021

I think your suggestion regarding the materials as macros is very reasonable.
Would you care to create a PR so it can be tested?
Also, a URDF for d455 is definitely something that would benefit many users. If already created, would you care to share and create a PR for that as well?

@SamerKhshiboun
Copy link
Collaborator

SamerKhshiboun commented Apr 11, 2023

@Nir-Az you I think can merge this PR for ros1-legacy.
I have duplicated this PR for ros2-development and assigned you as reviewer
#2689

@Nir-Az Nir-Az merged commit 9398c98 into IntelRealSense:ros1-legacy Apr 12, 2023
@civerachb-cpr
Copy link

Can you please bloom the ROS1 version of this package with that fix? The latest compiled .deb version still appears to be 2.3.2, which has the bug that this PR fixes.

@Nir-Az
Copy link
Collaborator

Nir-Az commented May 7, 2023

Can you please bloom the ROS1 version of this package with that fix? The latest compiled .deb version still appears to be 2.3.2, which has the bug that this PR fixes.

@civerachb-cpr sorry, we are no longer maintaining the ROS1 version, focus is on ROS2 only.

@jeff-o
Copy link

jeff-o commented May 8, 2023

Ouf, just ran into this bug on a robot I'm working on. This fix would be very welcome in the ROS1 .deb!

@aditya2592
Copy link

I ran into this issue and found this PR. The PR suggests the following change but this change is not made in the PR itself. Is this expected? I think without this change, we still get the message Error: material 'aluminum' is not unique.

Then changing the places they get used from <material name="plastic"/> to <xacro:plastic_material />

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants