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 gamma correction to simple_demo_qml example #1019

Merged

Conversation

athenaz2
Copy link
Contributor

@athenaz2 athenaz2 commented Jul 24, 2024

🦟 Bug fix

Summary

When running with Ogre2 and OpenGL, simple_demo_qml has darker shadows and the scene is a bit darker overall when compared to simple_demo. This is because the texture from the render engine is in sRGB color space, but Qt's textures are created in linear color space. As per gazebosim/gz-gui#629, this fix for simple_demo_qml ensures that the render engine texture, originally in sRGB format, is correctly passed to Qt in linear format. Thus, now the simple_demo_qml scene is lighter and matches simple_demo's scene.

Edit: modified changes as per gazebosim/gz-gui#630. Original changes copied render texture to a new internal texture before passing to Qt. Now, add a parameter to the render texture that disables sRGB decoding (only one time, not every frame) such that there is no sRGB -> linear conversion when texture is copied from Ogre2 to Qt, and now Qt displays it as-is as sRGB.

Before
image

After
simple demo no qml vs yes qml fix gl_gamma

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.

@athenaz2 athenaz2 requested a review from iche033 as a code owner July 24, 2024 23:50
@github-actions github-actions bot added the 🎵 harmonic Gazebo Harmonic label Jul 24, 2024
@athenaz2 athenaz2 force-pushed the athenaz/simple_demo_qml_gamma branch from 4ecb579 to fc95a6c Compare July 25, 2024 00:12
Copy link

codecov bot commented Jul 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.63%. Comparing base (5859051) to head (72abe1c).
Report is 17 commits behind head on gz-rendering8.

Additional details and impacted files
@@                Coverage Diff                @@
##           gz-rendering8    #1019      +/-   ##
=================================================
- Coverage          75.68%   75.63%   -0.06%     
=================================================
  Files                177      177              
  Lines              16959    16993      +34     
=================================================
+ Hits               12835    12852      +17     
- Misses              4124     4141      +17     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@athenaz2 athenaz2 force-pushed the athenaz/simple_demo_qml_gamma branch from 72abe1c to e704ccc Compare July 25, 2024 20:29
GLuint rhiFbo = 0;
// internal texture ID to hold texture data from render engine texture,
// in linear GL_RGB format
GLuint rhiId = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

can you make these ids member variables so that we can delete the fbo and texture in the destructor when the app exits? e.g. see code here

f->glGetIntegerv(GL_FRAMEBUFFER_BINDING, &currentFbo);

// bind render engine texture to RHI FBO
f->glGenFramebuffers(1, &rhiFbo);
Copy link
Contributor

Choose a reason for hiding this comment

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

this will create a new fbo every frame. So related to previous comment, we need to store this id and as member var, and here only create the frame buffer if the fbo does not exist yet, i.e. rhiFbo is 0. Same goes for the glGenTextures line below

@azeey azeey added the beta Targeting beta release of upcoming collection label Jul 29, 2024
@athenaz2 athenaz2 force-pushed the athenaz/simple_demo_qml_gamma branch from 7c5ee65 to 964f911 Compare July 29, 2024 23:50
@athenaz2 athenaz2 force-pushed the athenaz/simple_demo_qml_gamma branch from 964f911 to d65f167 Compare July 29, 2024 23:56
@iche033 iche033 merged commit 6a2217f into gazebosim:gz-rendering8 Jul 30, 2024
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta Targeting beta release of upcoming collection 🎵 harmonic Gazebo Harmonic
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants