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

INTEGRATION_depth_camera fails due to precision issue #332

Closed
1 task done
darksylinc opened this issue Jun 5, 2021 · 0 comments · Fixed by #333 or #785
Closed
1 task done

INTEGRATION_depth_camera fails due to precision issue #332

darksylinc opened this issue Jun 5, 2021 · 0 comments · Fixed by #333 or #785
Labels
bug Something isn't working

Comments

@darksylinc
Copy link
Contributor

darksylinc commented Jun 5, 2021

I had INTEGRATION_depth_camera consistently fail on me in #323 so I had to take a deep look.

Note: It is also failing in the ign-rendering5 branch, I just noticed it now because due to a setup error this sample wasn't launching.

Note2: This bugs repros with both GPU HW and LIBGL_ALWAYS_SOFTWARE=1.

At first I thought this was the same bug as #327 but that was not the case (although I suspect they are the same bug, more on this later), because the failure was on the blue channel:

ign/src/ign-rendering/test/integration/depth_camera.cc:257: Failure
Expected: (mb) > (0u), actual: 0 vs 0

// This one repeats A LOT
ign/src/ign-rendering/test/integration/depth_camera.cc:449: Failure
Expected: (b) > (0u), actual: 0 vs 0

After an hour of debugging I found the most horrendously bizarre problem.

The short version:

  1. Colours are correctly packed by packFloat in ogre2/src/media/materials/programs/depth_camera_fs.glsl
  2. The RGBA value is 0 0 143 255 which corresponds to floating point value: 8.21616e-40 stored in alpha
  3. Another pass (ogre2/src/media/materials/programs/depth_camera_final_fs.glsl) copies the calculated alpha value 'as is'. However after that copy, the resulting value is 0.0f

This is the RenderDoc capture of the first rendered frame:

BadRepro.rdc.zip

If we go to pixel[135][255].a we'll see correctly the pattern 0 0 143 255 (Texture '81' in our capture, byteswapped in the picture)

Gaz01

But when we go to the results of the copy (stored in Texture '83' in our capture), we see the values are gone:

Gaz02

Environment

  • OS Version: Ubuntu 18.04 LTS (with mainline PPA kernel 5.11)
  • Source or binary build?
    Source, branch ign-gazebo5 (diagnostic was made in my PostFrame branch though, since it's easier to capture RenderDoc there)
  • If this is a GUI or sensor rendering bug, describe your GPU and rendering system. Otherwise delete this section.
    • Rendering plugin: ogre2
      • running on real hardware
    • Rendering system info:
LANG=C glxinfo -B | grep -i '\(direct rendering\|opengl\|profile\)'  # might require installing mesa-utils package
direct rendering: Yes
    Preferred profile: core (0x1)
    Max core profile version: 4.6
    Max compat profile version: 4.6
    Max GLES1 profile version: 1.1
    Max GLES[23] profile version: 3.2
OpenGL vendor string: X.Org
OpenGL renderer string: Radeon RX 560 Series (POLARIS11, DRM 3.40.0, 5.11.0-051100-generic, LLVM 10.0.1)
OpenGL core profile version string: 4.6 (Core Profile) Mesa 20.1.3 (git-663fa46287)
OpenGL core profile shading language version string: 4.60
OpenGL core profile context flags: (none)
OpenGL core profile profile mask: core profile
OpenGL version string: 4.6 (Compatibility Profile) Mesa 20.1.3 (git-663fa46287)
OpenGL shading language version string: 4.60
OpenGL context flags: (none)
OpenGL profile mask: compatibility profile
OpenGL ES profile version string: OpenGL ES 3.2 Mesa 20.1.3 (git-663fa46287)
OpenGL ES profile shading language version string: OpenGL ES GLSL ES 3.20

ps aux | grep Xorg
root      1774  2.6  0.7 2973636 255116 tty7   Ssl+ 12:26   6:08 /usr/lib/xorg/Xorg -core :0 -seat seat0 -auth /var/run/lightdm/root/:0 -nolisten tcp vt7 -novtswitch
matias   10920  0.0  0.0  17988  1100 pts/5    S+   16:19   0:00 grep --color=auto Xorg


sudo env LANG=C X -version

X.Org X Server 1.20.8
X Protocol Version 11, Revision 0
Build Operating System: Linux 4.15.0-140-generic x86_64 Ubuntu
Current Operating System: Linux matias-ubuntu 5.11.0-051100-generic #202102142330 SMP Sun Feb 14 23:33:21 UTC 2021 x86_64
Kernel command line: BOOT_IMAGE=/boot/vmlinuz-5.11.0-051100-generic root=UUID=98733a50-8c43-4a07-bceb-ed2cc319135b ro quiet splash amdgpu.dc=0 amdgpu.gttsize=8192 acpi_osi=Linux vt.handoff=1
Build Date: 08 April 2021  01:40:27PM
xorg-server-hwe-18.04 2:1.20.8-2ubuntu2.2~18.04.5 (For technical support please see http://www.ubuntu.com/support) 
Current version of pixman: 0.34.0
	Before reporting problems, check http://wiki.x.org
	to make sure that you have the latest version.

Description

  • Expected behavior: Unit test should pass
  • Actual behavior: Unit test fails

Steps to reproduce

  1. Just run ign/build/ignition-rendering5/bin/INTEGRATION_depth_camera
  2. I'm using Mesa 20.1.3 (git-663fa46287)

Notes

Is this a driver bug?

  • Could be, yes. But I find it strange that LIBGL_ALWAYS_SOFTWARE fails on me too. Perhaps the problem has to do with interpolation during sampling. I checked it and samplers according to RenderDoc is set to "Point" "Mipmaps: None"
  • But I've ran into similar problems in the past. For example Ogre ran into a D3D11 bug where an uint32 mask was stored in a float, but it would fail when the mask resulted in a NaN. Why? Because after inspecting the assembly the code looked something like this:
mov freg0, [mem] // loads NaN into reg0
mul freg0, 1.0f // multiplies freg0 against 1.0f, destroying the NaN's bits
mov ireg0, freg0 // moves from floating point register to integer register, we have garbage now

This was a compiler codegen error in fxc (D3D11's compiler), If the float is a valid value, this spurious multiplication was harmless. But if the float was a NaN, all the binary information was lost and when we reinterpreted the float back to uint32, the relevant data was lost. Bottom line, manipulating uints in floats is always risky in terms of GPU

Are other samples/unit tests affected?

Unknown.

Is this the real cause of #327 instead of RGB vs RGBA?

Unknown.

Why are the CI server tests unaffected?

Unknown. Could be simply due to different Mesa driver versions.
Or maybe the CI servers aren't properly launching the ogre2 test (e.g. failure to load libOgreMain.so.2.1.0 will count as a pass!)

Is this only a unit test bug?

No. This bug could be affecting whatever result where the alpha channel is too close to 0 (or possibly could affect other binary patterns such as NaN).

Solutions

I'm still researching this problem. One possibility could be to use texelFetch instead of texture() to bypass filtering completely. But it's still risky.

The sure solution would be to reinterpret the texture as RGBA32_UINT; sample it as uint4; then reinterpret the rgb to float, and copy the alpha as is.

IIRC Ogre 2.1 did not support texture reinterpretation, it was implemented in 2.2

Update:
Replacing vec4 p = texture(inputTexture, inPs.uv0); with vec4 p = texelFetch(inputTexture, ivec2( inPs.uv0 * 256.0 ), 0 ); did solve the problem at least for me (I hardcoded the resolution to 256 for a quick test)

@darksylinc darksylinc added the bug Something isn't working label Jun 5, 2021
darksylinc added a commit to darksylinc/ign-rendering that referenced this issue Jun 5, 2021
iche033 pushed a commit that referenced this issue Jun 9, 2021
iche033 added a commit that referenced this issue Jun 9, 2021
* Fix floating point precision bug handling alpha channel (#332)

Fixes #332
Fixes #108

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

* test reenabling depth camera test on mac

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

Co-authored-by: Matias N. Goldberg <[email protected]>
iche033 pushed a commit that referenced this issue Jun 14, 2021
iche033 pushed a commit that referenced this issue Jun 14, 2021
@iche033 iche033 mentioned this issue Jun 18, 2021
ahcorde pushed a commit that referenced this issue Jul 13, 2021
* Fix floating point precision bug handling alpha channel (#332) (#333)

Fixes #332
Fixes #108

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

* Test re-enabling depth camera integration test on mac (#335)

* Fix floating point precision bug handling alpha channel (#332)

Fixes #332
Fixes #108

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

* test reenabling depth camera test on mac

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

Co-authored-by: Matias N. Goldberg <[email protected]>

* Fix new [] / delete mismatch (#338)

Using a custom deallocator to avoid breaking ABI

Alternatively C++17 supports the following syntax, which was not used:

`typedef std::shared_ptr<unsigned char[]> DataPtr;`

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

* Fix heap overflow when reading (#337)

PF_RGB is 3 bytes. But later on Ogre2SelectionBuffer::OnSelectionClick
will try to read 4 bytes from it.

Fixed by ensuring it's always at least 4 bytes and zero-initializing
those 4 bytes.

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

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

* Fix floating point precision bug handling alpha channel (#332) (#333)

Fixes #332
Fixes #108

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

* Fix depth alpha (#316)

* update test

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

* reenable macos test

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

* fix typo

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

* cherry pick f9f1820 and fix conflicts

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

* Fix FSAA in UI and lower VRAM consumption (#313)

* Fix FSAA in UI and lower VRAM consumption

FSAA was being requested however due to how the compositor was setup,
this effect was not taking effect.

Additionally, the Compositor setup was improved to lower memory
consumption. Originally the setup was taken from Ogre samples which
assume they will ultimately be rendering to a window.

However this is not the case and thus IGN was creating 3 render targets
(two for ping-ponging between postprocess FXs + one for storing the
final result)
This was optimized so that we only create 2 render targets: two for
ping-ponging between postprocess FXs and we pick at runtime which one is
storing the final result via the new variable renderTargetResultsIdx

Further performance optimizations could be made in Ogre 2.2 to improve
unnecessary MSAA resolving when doing postprocess, though considering
there's currently only one postprocessing effect (the Gaussian filter) I
doubt this optimization would make much of a difference

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

* Add Ogre2RenderTarget::RenderTarget back

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

* Rewrote the compositor changes to support RenderWindows

As a bonus this new method breaks ABI far less.
Fix leak: DestroyCompositor would often not be called

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

* Mantain ABI compatibility #Ogre2IsRenderWindowABI

When merging to newer branches that can break the ABI,
revert this commit

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

* Fix ABI problems

I gave up on commit undoing

It's clear that on the next release, Ogre2RenderTarget and
Ogre2RenderTexture need to be merged together.

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

* Fix camel case convention

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

* Make Ogre2RenderTarget::RenderTarget pure virtual again

Hopefully this will prevent ABI breakage

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

* Fix deprecation warnings during build

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

* Fix invalid write of size 8

This was causing heap corruption. At best it would crash. At worst it
would manifeset in subtle weird behaviors

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

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

* Backport memory fixes found by ASAN (#340)

* Fix heap overflow when reading (#337)

PF_RGB is 3 bytes. But later on Ogre2SelectionBuffer::OnSelectionClick
will try to read 4 bytes from it.

Fixed by ensuring it's always at least 4 bytes and zero-initializing
those 4 bytes.

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

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

* Fix new [] / delete mismatch (#338)

Using a custom deallocator to avoid breaking ABI

Alternatively C++17 supports the following syntax, which was not used:

`typedef std::shared_ptr<unsigned char[]> DataPtr;`

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

Co-authored-by: darksylinc <[email protected]>

* recreate node only when needed (#342)

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

* Fix custom shaders uniforms ign version number (#343)

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

* relax gaussian test tolerance (#344)

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

* Update light map tutorial (#346)

* apply changes

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

* remove line

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

* add ifdef for apple in integration test

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

* 🎈 4.8.0 (#348)

Signed-off-by: Louise Poubel <[email protected]>

* update ign-rendering version in custom shaders uniform sample

Signed-off-by: Ian Chen <[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]>

* 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]>

Co-authored-by: Ian Chen <[email protected]>
Co-authored-by: Louise Poubel <[email protected]>
iche033 added a commit that referenced this issue Sep 20, 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]>

* Merge main branch into Ogre 2.2 and fixes (#359)

* Fix floating point precision bug handling alpha channel (#332) (#333)

Fixes #332
Fixes #108

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

* Test re-enabling depth camera integration test on mac (#335)

* Fix floating point precision bug handling alpha channel (#332)

Fixes #332
Fixes #108

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

* test reenabling depth camera test on mac

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

Co-authored-by: Matias N. Goldberg <[email protected]>

* Fix new [] / delete mismatch (#338)

Using a custom deallocator to avoid breaking ABI

Alternatively C++17 supports the following syntax, which was not used:

`typedef std::shared_ptr<unsigned char[]> DataPtr;`

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

* Fix heap overflow when reading (#337)

PF_RGB is 3 bytes. But later on Ogre2SelectionBuffer::OnSelectionClick
will try to read 4 bytes from it.

Fixed by ensuring it's always at least 4 bytes and zero-initializing
those 4 bytes.

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

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

* Fix floating point precision bug handling alpha channel (#332) (#333)

Fixes #332
Fixes #108

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

* Fix depth alpha (#316)

* update test

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

* reenable macos test

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

* fix typo

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

* cherry pick f9f1820 and fix conflicts

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

* Fix FSAA in UI and lower VRAM consumption (#313)

* Fix FSAA in UI and lower VRAM consumption

FSAA was being requested however due to how the compositor was setup,
this effect was not taking effect.

Additionally, the Compositor setup was improved to lower memory
consumption. Originally the setup was taken from Ogre samples which
assume they will ultimately be rendering to a window.

However this is not the case and thus IGN was creating 3 render targets
(two for ping-ponging between postprocess FXs + one for storing the
final result)
This was optimized so that we only create 2 render targets: two for
ping-ponging between postprocess FXs and we pick at runtime which one is
storing the final result via the new variable renderTargetResultsIdx

Further performance optimizations could be made in Ogre 2.2 to improve
unnecessary MSAA resolving when doing postprocess, though considering
there's currently only one postprocessing effect (the Gaussian filter) I
doubt this optimization would make much of a difference

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

* Add Ogre2RenderTarget::RenderTarget back

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

* Rewrote the compositor changes to support RenderWindows

As a bonus this new method breaks ABI far less.
Fix leak: DestroyCompositor would often not be called

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

* Mantain ABI compatibility #Ogre2IsRenderWindowABI

When merging to newer branches that can break the ABI,
revert this commit

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

* Fix ABI problems

I gave up on commit undoing

It's clear that on the next release, Ogre2RenderTarget and
Ogre2RenderTexture need to be merged together.

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

* Fix camel case convention

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

* Make Ogre2RenderTarget::RenderTarget pure virtual again

Hopefully this will prevent ABI breakage

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

* Fix deprecation warnings during build

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

* Fix invalid write of size 8

This was causing heap corruption. At best it would crash. At worst it
would manifeset in subtle weird behaviors

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

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

* Backport memory fixes found by ASAN (#340)

* Fix heap overflow when reading (#337)

PF_RGB is 3 bytes. But later on Ogre2SelectionBuffer::OnSelectionClick
will try to read 4 bytes from it.

Fixed by ensuring it's always at least 4 bytes and zero-initializing
those 4 bytes.

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

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

* Fix new [] / delete mismatch (#338)

Using a custom deallocator to avoid breaking ABI

Alternatively C++17 supports the following syntax, which was not used:

`typedef std::shared_ptr<unsigned char[]> DataPtr;`

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

Co-authored-by: darksylinc <[email protected]>

* recreate node only when needed (#342)

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

* Fix custom shaders uniforms ign version number (#343)

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

* relax gaussian test tolerance (#344)

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

* Update light map tutorial (#346)

* apply changes

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

* remove line

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

* add ifdef for apple in integration test

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

* 🎈 4.8.0 (#348)

Signed-off-by: Louise Poubel <[email protected]>

* update ign-rendering version in custom shaders uniform sample

Signed-off-by: Ian Chen <[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]>

* 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]>

Co-authored-by: Ian Chen <[email protected]>
Co-authored-by: Louise Poubel <[email protected]>

* Began work on Heightmaps with Ogre2. Very WIP

Hardcoded some paths in CMakeLists to get Terra compiling

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

* Register HlmsTerra

Add its Hlms data files

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

* Update all Terras and show them on screen (WIP)

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

* Update to latest Terra to get Z-up support

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

* Better calculation of heightmap size (WIP)

Fix crash when heightmap is deleted
Update to latest Terra for setCustomSkirtMinHeight

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

* Fix shader compiler error

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

* Fix alignment issues in terrain

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

* Move Terra to its own folder and project

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

* Enable assert that was accidentally disabled

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

* Customize Terra for Ignition's requirements

Fix barrier assert

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

* Use custom blending modes to adjust to ign

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

* HeightmapTexture::Size is in meters, it's not a scale

Now it looks close to the reference in ogre1
No longer normalize weights, it's not needed anymore
Also fix bug when 5 texture maps could not be supported

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

* Use same blending algorithm for roughness & metalness

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

* Deal with edge case where terrain skirts could be above ground

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

* Fix wrong indentation

Remove dead code

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

* Forgot to push this change

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

* Disable alpha blending of detail maps for Terrain

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

* Add terrain shadows to normal objects

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

* Fix copyright year

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

* Re-enable ImageHeightmap

It shouldn't have been disabled

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

* Cosmetic changes

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

* Move Terra's source code into ogre2/src

It is ogre2 specific

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

* Support ogre1 heightmaps via cropping and warn about it

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

* Support both ogre2 & ogre engines in heightmap sample

Default to ogre2

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

* Force-disable JSON parts of Terra

It's not used by ignition.
This avoids detecting rapidjson during ign build
Fix Windows build

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

* Add license to Terra files

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

* Update to latest version of Terra

Adds spotlight & point shadow map support (not working/set yet)

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

* Add terrain shadows from spot & point lights

Use unique_ptr instead of raw ptrs when possible

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

* Skip Terra headers in cppcheck

They should be treated as external code

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]>

* Fix crash on shutdown due to ptr destroyed too late

It should be destroyed before Ogre Root

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

* Update to latest Terra to fix bug from upstream

Upstream
OGRECave/ogre-next@d0a01d2

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

* Update to latest Hlms from upstream

This fixes some compiler bugs with Terra
Also adds supports for HlmsPbs::setShadowReceiversInPixelShader which
was written specifically for Gazebo

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

* Fix compiler warning

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

* Fix Terra shadows on Spot and Point lights

Update Terra to latest from upstream

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

* Fix Terra's point light shadows

Terra's workspace listener was being executed before the camera is
rotated for cubemap rendering

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

* Fix documentation

Silence warnings from external code to pass build checks

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

* fixing CI

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

* add include path

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

* windows warning

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

* fix

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

Co-authored-by: ahcorde <[email protected]>
Co-authored-by: Michael Carroll <[email protected]>
Co-authored-by: Ian Chen <[email protected]>
Co-authored-by: Louise Poubel <[email protected]>
iche033 added a commit that referenced this issue Dec 28, 2022
This PR adds a heightmap test.

Almost all issues in #546 are covered.

Only Thermal Camera is not covered.

Fixes #332 (again) for macOS and possibly any OS/HW/Driver combination.
Fixes #369

Signed-off-by: Matias N. Goldberg <[email protected]>
Co-authored-by: Ian Chen <[email protected]>
iche033 pushed a commit that referenced this issue Dec 28, 2022
Fixes #332 (again) for macOS and possibly any OS/HW/Driver combination.
Fixes #369

For a very long time macOS (and possibly other OS/HW/Driver combination) would fail the tests because the shader code was doing (simplified, in C to convey the explanation):

```
uint32_t valueToStore = r << 24 | g << 16 | b << 8 | a;
float output;
output = (float*)&valueToStore;
return output;
```

In ticket #332 I made a deep analysis and found out that given the wrong conditions, the bit pattern would result in either a denormal or a NaN in floating point; which risks being flushed to 0 (or set all types of NaN to the same type bit pattern of NaN).

Although the ticket was closed and tagged as "fixed", on some OS/HW/Driver combinations (e.g. macOS + Metal) this error would reappear.

This isn't just a mere test failure. This is an actual bug. Running DepthCamera in macOS could result in wrong readings.

This PR fixes the problem for good by using RGBA32_UINT instead of RGBA32_FLOAT; and having uints be reinterpreted to floats and back when needed; which is well-defined and much more stable as far as GPU shader compilers go.

The same fix is also part of PR #785 which targets Garden and includes other changes.

This PR is a backport specific for Fortress.

Signed-off-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
bug Something isn't working
Projects
None yet
1 participant