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

MJCF to SDF: Add Material #42

Merged
merged 10 commits into from
Jun 3, 2022
Merged

MJCF to SDF: Add Material #42

merged 10 commits into from
Jun 3, 2022

Conversation

ahcorde
Copy link
Collaborator

@ahcorde ahcorde commented May 23, 2022

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

🎉 New feature

Summary

Add Material

I have two issues:

  • file name are changed, we used in the mjcf file file.png and we got from the python API file-hash.png, should I create this file ? add a flag to export them ?
  • According with XML Reference, tight now we only support gridsize="1 1". There is other way to define textures, please review this field gridlayout: string, “…………” and share your thoughts. This way is not defined ignition, this may require some work in other ignition libraries

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • 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 and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde requested a review from azeey May 23, 2022 11:56
@ahcorde ahcorde self-assigned this May 23, 2022
@codecov
Copy link

codecov bot commented May 23, 2022

Codecov Report

Merging #42 (e19d5a2) into main (7faaf8d) will decrease coverage by 0.33%.
The diff coverage is 92.15%.

@@            Coverage Diff             @@
##             main      #42      +/-   ##
==========================================
- Coverage   97.54%   97.20%   -0.34%     
==========================================
  Files          18       19       +1     
  Lines         773      824      +51     
==========================================
+ Hits          754      801      +47     
- Misses         19       23       +4     
Impacted Files Coverage Δ
...o_sdformat/mjcf_to_sdformat/converters/material.py 91.48% <91.48%> (ø)
...cf_to_sdformat/mjcf_to_sdformat/converters/link.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7faaf8d...e19d5a2. Read the comment docs.

@ahcorde ahcorde mentioned this pull request May 31, 2022
9 tasks
@azeey
Copy link
Collaborator

azeey commented May 31, 2022

According with XML Reference, tight now we only support gridsize="1 1". There is other way to define textures, please review this field gridlayout: string, “…………” and share your thoughts. This way is not defined ignition, this may require some work in other ignition libraries

I have created an issue about the difference in texture mapping between Gazebo and Mujoco for primitive shapes (#58). I want to test how it works with meshes as well, but we don't have mesh conversion yet. I think it's safe to say SDFormat doesn't support gridlayout, so we can just emit an warning/error if we see a file with that attribute.

mjcf_to_sdformat/mjcf_to_sdformat/converters/material.py Outdated Show resolved Hide resolved
mjcf_to_sdformat/mjcf_to_sdformat/converters/material.py Outdated Show resolved Hide resolved
mjcf_to_sdformat/mjcf_to_sdformat/converters/material.py Outdated Show resolved Hide resolved
material.set_diffuse(su.rgba_to_color(geom.rgba))
material.set_ambient(su.rgba_to_color(geom.rgba))
material.set_specular(su.rgba_to_color(geom.rgba))
material.set_emissive(su.rgba_to_color(geom.rgba))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we should be setting the emissive property based on the rgba, but I don't have a good suggestion. Maybe we can come back to it later.

@ahcorde ahcorde requested a review from azeey June 3, 2022 08:42
azeey added 2 commits June 3, 2022 11:10
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@ahcorde ahcorde merged commit ab8fd15 into main Jun 3, 2022
@ahcorde ahcorde deleted the ahcorde/mjcf2sdf/material branch June 3, 2022 18:47
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.

None yet

2 participants