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

Added render order to material #188

Merged
merged 8 commits into from
Jan 12, 2021
Merged

Added render order to material #188

merged 8 commits into from
Jan 12, 2021

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Dec 22, 2020

The goal of this PR is to add the feature render order in case some meshes in the scene are coplanar. With this parameter will avoid the artifact generated by z-fighting.

Coplanar planes issues:
Gazebo_069

Fixed:
Gazebo_068

This PR depends on:

Signed-off-by: ahcorde [email protected]

@ahcorde ahcorde changed the base branch from ign-rendering4 to main December 23, 2020 21:10
@ahcorde ahcorde force-pushed the ahcorde/feature/renderOrder branch from 54f35d5 to b50de42 Compare December 23, 2020 21:18
@ahcorde
Copy link
Contributor Author

ahcorde commented Dec 23, 2020

Retargeted to main

@ahcorde ahcorde requested a review from chapulina December 23, 2020 21:19
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Works for me, I tried the demo on #190 with Ogre 1 and 2. I think a test could be added which checks that all pixels are the expected color, but I'll leave that up to you if you think it's worth it.

include/ignition/rendering/base/BaseMaterial.hh Outdated Show resolved Hide resolved
Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

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

looks good to me. Just a few comments on the tutorial. Can you also add the tutorial to tutorials.md.in?

tutorials/21_render_order.md Outdated Show resolved Hide resolved
tutorials/21_render_order.md Outdated Show resolved Hide resolved
tutorials/21_render_order.md Outdated Show resolved Hide resolved
tutorials/21_render_order.md Outdated Show resolved Hide resolved
Signed-off-by: ahcorde <[email protected]>
@ahcorde ahcorde requested a review from iche033 January 5, 2021 07:52
tutorials.md.in Outdated Show resolved Hide resolved
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
@codecov
Copy link

codecov bot commented Jan 6, 2021

Codecov Report

Merging #188 (4b9c584) into main (b7620f4) will decrease coverage by 0.00%.
The diff coverage is 54.16%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #188      +/-   ##
==========================================
- Coverage   53.08%   53.08%   -0.01%     
==========================================
  Files         143      143              
  Lines       13545    13568      +23     
==========================================
+ Hits         7191     7203      +12     
- Misses       6354     6365      +11     
Impacted Files Coverage Δ
include/ignition/rendering/Material.hh 100.00% <ø> (ø)
ogre/src/OgreMaterial.cc 0.00% <0.00%> (ø)
include/ignition/rendering/base/BaseMaterial.hh 59.92% <44.44%> (-0.52%) ⬇️
ogre2/src/Ogre2Material.cc 95.27% <100.00%> (+0.13%) ⬆️

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 b7620f4...4b9c584. Read the comment docs.

@chapulina chapulina merged commit 821dd9d into main Jan 12, 2021
@chapulina chapulina deleted the ahcorde/feature/renderOrder branch January 12, 2021 02:06
@ahcorde ahcorde mentioned this pull request Feb 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔮 dome Ignition Dome needs upstream release Blocked by a release of an upstream library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants