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

Don't keep DEPTH_CLAMP enabled after it's used #393

Conversation

darksylinc
Copy link
Contributor

Probably a copy-paste bug, after enabling DEPTH_CLAMP we must disable
once we're done; but we call glEnable again instead of disabling it.

Signed-off-by: Matias N. Goldberg [email protected]

This is a silly bug

🦟 Bug fix

No number

Summary

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • 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

Probably a copy-paste bug, after enabling DEPTH_CLAMP we must disable
once we're done; but we call glEnable again instead of disabling it.

Signed-off-by: Matias N. Goldberg <[email protected]>
@darksylinc darksylinc requested a review from iche033 as a code owner September 4, 2021 17:59
@codecov
Copy link

codecov bot commented Sep 4, 2021

Codecov Report

Merging #393 (054249d) into ahcorde/update/ogre2.2 (be61509) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@                    Coverage Diff                     @@
##           ahcorde/update/ogre2.2     #393      +/-   ##
==========================================================
- Coverage                   58.71%   58.70%   -0.01%     
==========================================================
  Files                         174      174              
  Lines                       17184    17184              
==========================================================
- Hits                        10089    10088       -1     
- Misses                       7095     7096       +1     
Impacted Files Coverage Δ
ogre2/src/Ogre2ThermalCamera.cc 89.51% <100.00%> (ø)
...e/ignition/rendering/base/BaseGaussianNoisePass.hh 96.66% <0.00%> (-3.34%) ⬇️

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 be61509...054249d. Read the comment docs.

@chapulina chapulina added the 🏯 fortress Ignition Fortress label Sep 8, 2021
@iche033 iche033 merged commit 95af386 into gazebosim:ahcorde/update/ogre2.2 Sep 8, 2021
iche033 added a commit that referenced this pull request Sep 8, 2021
* Ogre2.2

Signed-off-by: ahcorde <[email protected]>

* Added media files

Signed-off-by: ahcorde <[email protected]>

* Added Headless mode

Signed-off-by: ahcorde <[email protected]>

* Added feedback

Signed-off-by: ahcorde <[email protected]>

* Added feedback

Signed-off-by: ahcorde <[email protected]>

* Add OGRE2.2 to dependencies

Signed-off-by: Michael Carroll <[email protected]>

* Fixed GPUray and depthCamera test

Co-authored-by: Michael Carroll <[email protected]>

Signed-off-by: ahcorde <[email protected]>

* cast based on pf

Signed-off-by: Ian Chen <[email protected]>

* Fix cleanup with packaged debs

Signed-off-by: Michael Carroll <[email protected]>

* fix copy raw data

Signed-off-by: Ian Chen <[email protected]>

* fix material test

Signed-off-by: Ian Chen <[email protected]>

* Lint and compiler warning

Signed-off-by: Michael Carroll <[email protected]>

* Trim whitespace

Signed-off-by: Michael Carroll <[email protected]>

* Apply gamma correction to sky

Signed-off-by: ahcorde <[email protected]>

* Test fixes to Ogre2.2 branch (#279)

Signed-off-by: Michael Carroll <[email protected]>

* Revert gamma correction in the sky

Signed-off-by: ahcorde <[email protected]>

* enable grayscale albedo map to fix red color highlights and disable hardware gamma to fix dark sky color

Signed-off-by: Ian Chen <[email protected]>

* Local updates for Ogre2.2 against main branch (#317)

* Local updates for Ogre2.2 against main branch

Signed-off-by: Ian Chen <[email protected]>
Co-authored-by: Ian Chen <[email protected]>

* Fix merge

Signed-off-by: ahcorde <[email protected]>

* fix shadows in ogre 2.2

Signed-off-by: Ian Chen <[email protected]>

* Lint

Signed-off-by: Michael Carroll <[email protected]>

* fix toggling sky

Signed-off-by: Ian Chen <[email protected]>

* fix camera test

Signed-off-by: Ian Chen <[email protected]>

* make linters happy

Signed-off-by: ahcorde <[email protected]>

* make linters happy

Signed-off-by: ahcorde <[email protected]>

* Fix small regression from merge

Signed-off-by: Michael Carroll <[email protected]>

* Remove problematic leftover files from 2.1

The mere presence of these files can cause incorrect shader generation
or unknown visual bugs.

IMPORTANT: The installation folder (i.e. CMAKE_INSTALL_PREFIX) must
remove them as well.

`make install` won't be enough because it won't remove files, only add
new ones or update existing ones.

This folder is usually installed in
ign/install/share/ignition/ignition-rendering6/ogre2/media/Hlms

Signed-off-by: Matias N. Goldberg <[email protected]>

* Do not crash on shutdown

- ogreRoot may be nullptr
- Do not destroy textures already scheduled for destruction

Signed-off-by: Matias N. Goldberg <[email protected]>

* Remove problematic leftover files from 2.1 (#354)

The mere presence of these files can cause incorrect shader generation
or unknown visual bugs.

IMPORTANT: The installation folder (i.e. CMAKE_INSTALL_PREFIX) must
remove them as well.

`make install` won't be enough because it won't remove files, only add
new ones or update existing ones.

This folder is usually installed in
ign/install/share/ignition/ignition-rendering6/ogre2/media/Hlms

Signed-off-by: Matias N. Goldberg <[email protected]>

* Do not crash on shutdown (#355)

- ogreRoot may be nullptr
- Do not destroy textures already scheduled for destruction

Signed-off-by: Matias N. Goldberg <[email protected]>

* Removed unused variable

Signed-off-by: ahcorde <[email protected]>

* Restore FSAA support in 2.2 branch

Signed-off-by: Matias N. Goldberg <[email protected]>

* Fix changing background color not always taking immediate effect

Changed pass_clear in favour of LoadAction::Clear which is more
efficient on very modern GPUs and specially TBDR ones.

Signed-off-by: Matias N. Goldberg <[email protected]>

* Changed pass_clear in favour of LoadAction::Clear

which is more efficient on very modern GPUs and specially TBDR ones.

Signed-off-by: Matias N. Goldberg <[email protected]>

* Missing public keyword

Signed-off-by: Matias N. Goldberg <[email protected]>

* Save VRAM when FSAA is used and no postprocessing

There's an unused texture when these conditions are met (which are
fairly common)
This memory optimization could not be performed in Ogre 2.1, it needs
Ogre 2.2+

Signed-off-by: Matias N. Goldberg <[email protected]>

* Remove code deprecated in ign-rendering5

Syntax cosmetic changes for consistency

Signed-off-by: Matias N. Goldberg <[email protected]>

* Remove code deprecated in ign-rendering5

Signed-off-by: Matias N. Goldberg <[email protected]>

* Code style fixes

Signed-off-by: Matias N. Goldberg <[email protected]>

* Undo VRAM saving optimization: It cannot be applied

The "Final Composition" node requires both textures to be resident.
Thus 2nd texture must always be resident.

The optimization could still be applied if we create two Final
Composition nodes (one for when 2 textures are needed, another for when
only MSAA + 1 texture is needed) but this would:

  1. Hurt code readability too much (i.e. what is going on?)
  2. Increase debuggability difficulty too much because codepaths taken
would differ depending on whether optimization was applied. Also certain
bugs could remain hidden until compositors are toggled.

This was causing ogre2_demo to fail.

Signed-off-by: Matias N. Goldberg <[email protected]>

* fixing tests

Signed-off-by: Ian Chen <[email protected]>

* add ifdef

Signed-off-by: Ian Chen <[email protected]>

* suppress warnings

Signed-off-by: Ian Chen <[email protected]>

* update syntax

Signed-off-by: Ian Chen <[email protected]>

* update syntax

Signed-off-by: Ian Chen <[email protected]>

* Don't keep DEPTH_CLAMP enabled after it's used (#393)

Probably a copy-paste bug, after enabling DEPTH_CLAMP we must disable
once we're done; but we call glEnable again instead of disabling it.

Signed-off-by: Matias N. Goldberg <[email protected]>

Co-authored-by: Michael Carroll <[email protected]>
Co-authored-by: Ian Chen <[email protected]>
Co-authored-by: Matias N. Goldberg <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏯 fortress Ignition Fortress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants