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

Allow to convert URDF color to SDF material tag #526

Merged
merged 15 commits into from
May 24, 2021
Merged

Conversation

ahcorde
Copy link
Collaborator

@ahcorde ahcorde commented Mar 30, 2021

Signed-off-by: ahcorde ahcorde@gmail.com

🦟 Bug fix

This PR tries to fix this issue #513 where the URDF gazebo material tags are incorrectly translated

As a general overview:

  • URDF defined the color as rgba but in ignition the color is defined as ambient, specular and diffuse. URDF definition
  • The other way to use a color in the URDF format is with OGRE materials, but according with the migration guide there is no support for this.

This solution will not break the old behaviour, if there is any <material> tag under a <gazebo> tag, then the tag will be replaced.

Summary

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Code check passed (In source directory, run sh tools/code_check.sh)
  • All tests passed (See
    test coverage)
  • While waiting for a review on your PR, please help review
    another open pull request
    to support the maintainers

Note to maintainers: Remember to use Squash-Merge

Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde self-assigned this Mar 30, 2021
@github-actions github-actions bot added the 🏢 edifice Ignition Edifice label Mar 30, 2021
@chapulina chapulina changed the base branch from main to sdf11 April 1, 2021 20:35
@chapulina chapulina requested a review from nkoenig April 5, 2021 18:48
@nkoenig
Copy link
Contributor

nkoenig commented Apr 6, 2021

Is it possible to add a test?

@peci1
Copy link
Contributor

peci1 commented Apr 6, 2021

Will this be compatible with Gazebo Classic in case only the URDF color is defined?

@ahcorde
Copy link
Collaborator Author

ahcorde commented Apr 12, 2021

Will this be compatible with Gazebo Classic in case only the URDF color is defined?

Gazebo11 (classic) is using sdformat9, this PR is targeting sdformat11. This should not affect Gazebo classic: Gazebo9 or Gazebo11.

What do you think @nkoenig ? should I retarget this to sdformat9? and make it compatible with Citadel ?

@nkoenig
Copy link
Contributor

nkoenig commented Apr 12, 2021

Will this be compatible with Gazebo Classic in case only the URDF color is defined?

Gazebo11 (classic) is using sdformat9, this PR is targeting sdformat11. This should not affect Gazebo classic: Gazebo9 or Gazebo11.

What do you think @nkoenig ? should I retarget this to sdformat9? and make it compatible with Citadel ?

I don't see a problem leaving this PR targeted toward sdformat11. No reason to potentially upset Gazebo classic.

ahcorde and others added 2 commits April 12, 2021 23:40
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Copy link
Collaborator Author

ahcorde commented Apr 12, 2021

@nkoenig Test added

Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Copy link
Collaborator Author

ahcorde commented Apr 13, 2021

I'm going to close this PR because there is a way to include the color without adding new code, for example:

<link name='cart'>
...
</link>
<gazebo reference="cart">
    <visual>
      <material>
        <ambient>0 0 1 1</ambient>
        <diffuse>0 0 1 1</diffuse>
        <specular>0 0 1 1</specular>
      </material>
    </visual>
</gazebo>

@ahcorde ahcorde closed this Apr 13, 2021
@peci1
Copy link
Contributor

peci1 commented Apr 13, 2021

there is a way to include the color without adding new code

That's only part-wise solution. This way, you can only set a single material to all <visual> tags of the link. Using the URDF color tags, you set the color per-<visual>. This can be overcome by splitting the visual elements to multiple lumped links, but it's impractical at least...

@ahcorde ahcorde reopened this Apr 13, 2021
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Copy link
Collaborator Author

ahcorde commented Apr 15, 2021

friendly ping @nkoenig

Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde requested a review from azeey May 12, 2021 11:58
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Copy link
Collaborator Author

ahcorde commented May 19, 2021

friendly ping @azeey

Copy link
Collaborator

@azeey azeey left a comment

Choose a reason for hiding this comment

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

I tried to using an example to compare the colors between Rviz and ign-gazebo (with examples/world/empty.sdf). It still doesn't match quite right. ign-gazebo is much brighter.
image. I don't know if this an issue with ign-gazebo or not. I'll go ahead and approve since the values used for the conversion seem reasonable to me.

@ahcorde
Copy link
Collaborator Author

ahcorde commented May 24, 2021

Merging I ticketed an issue

@ahcorde ahcorde merged commit 8e4d3e8 into sdf11 May 24, 2021
@ahcorde ahcorde deleted the ahcorde/urdf/material branch May 24, 2021 11:20
@peci1
Copy link
Contributor

peci1 commented Jun 1, 2021

Are there plans for backporting to sdf10?

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

Successfully merging this pull request may close these issues.

5 participants