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

Fix Ogre2RenderTarget::TargetFSAA method that caused black screen when used with llvmpipe (i.e. software rendering on Linux) #661

Merged
merged 1 commit into from
Jul 1, 2022

Conversation

traversaro
Copy link
Contributor

The Ogre2RenderTarget::TargetFSAA method has a logic to check if the selected antialiasing level is supported.

If the requested antialiasing level is not supported, the 0 level is selected instead. Furthermore, a warning is printed only once for process. Before this PR, the 0 level was only set only when the warning was printed, resulting in an unsupported antialiasing being set if Ogre2RenderTarget::TargetFSAA was called two or more times.

🦟 Bug fix

Fixes gazebosim/gz-sim#1116 .

Summary

Before this fix, running LIBGL_ALWAYS_SOFTWARE=1 ign gazebo shapes.sdf --verbose with a recent enough mesa (for example the one provided by default in Ubuntu 22.04) resulted in a black screen in the render area. With this fix, everything works is rendered correctly.

Checklist

  • Signed all commits for DCO
  • Added tests
  • 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.

@traversaro traversaro requested a review from iche033 as a code owner July 1, 2022 08:20
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Jul 1, 2022
Fix Ogre2RenderTarget::TargetFSAA method that caused black screen when used with llvmpipe (i.e. software rendering on Linux)

The Ogre2RenderTarget::TargetFSAA method has a logic to check if the selected antialiasing level is supported.

If the requested antialiasing level is not supported, the 0 level is selected instead. Furthermore, a warning is printed only once for process. Before this PR, the 0 level was only set only when the warning was printed, resulting in an unsupported antialiasing being set if Ogre2RenderTarget::TargetFSAA was called two or more times.

Signed-off-by: Silvio <[email protected]>
@traversaro
Copy link
Contributor Author

traversaro commented Jul 1, 2022

As the bug was introduced (I think) in https://github.com/gazebosim/gz-rendering/pull/272/files#diff-d0c4d95c8bb6f3ff2d80c1c9b72dc8c4d5635d31d3a71d4d75bb3b4dca632407L488, thecode in Ogre1 and Ignition Citadel is not affected by this problem, so no other PRs are required:

Other parts of the code can be improved (the warning should be printed again if a SetAliasing with a different value is called or a SetAliasing on a different instance, why the aa value is hardcoded to 1 if 0 is actually selected). However, this problems can be discussed in a separated issue, as the point of this PR is to fix gazebosim/gz-sim#1116 with minimal modifications.

@codecov
Copy link

codecov bot commented Jul 1, 2022

Codecov Report

Merging #661 (52b0b59) into ign-rendering6 (d773877) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@                Coverage Diff                 @@
##           ign-rendering6     #661      +/-   ##
==================================================
- Coverage           54.95%   54.93%   -0.02%     
==================================================
  Files                 202      202              
  Lines               21102    21102              
==================================================
- Hits                11596    11592       -4     
- Misses               9506     9510       +4     
Impacted Files Coverage Δ
ogre2/src/Ogre2RenderTarget.cc 85.68% <100.00%> (-0.96%) ⬇️

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 d773877...52b0b59. Read the comment docs.

@traversaro
Copy link
Contributor Author

FYI @srmainwaring as you recently modified this code in #470 .

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, thanks for the fix.

@iche033 iche033 merged commit 2adcb97 into gazebosim:ign-rendering6 Jul 1, 2022
@srmainwaring
Copy link
Contributor

as you recently modified this code in #470 .

@traversaro thanks! Checked this on an Ubuntu 22.04 VM (VMware Fusion) and running fine under llvmpipe (LLVM 13.0.1).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏯 fortress Ignition Fortress
Projects
Archived in project
3 participants