Fix FSAA in UI and lower VRAM consumption #313

darksylinc · 2021-05-01T22:12:19Z

PR NOTE 1

This fixes a regression from ign-rendering4.
In ign-rendering4 FSAA worked fine because it was relying on compositor scripts. This broke when the compositor script was replaced with C++ code.

The VRAM consumption issue affects both branches, but I think it's just fine to make it ign-rendering5 and onwards only

PR NOTE 2 (important)

How do I test the camera gaussian filter? It's affected directly (i.e. if renderTargetResultsIdx is set to the wrong value then the image won't look like the filter was applied, or some other similar issue) and I had no idea how to test it.

Update: ~~CI says gaussian filter test failed. How shocking. I should check that test locally~~

Update 2: Found the bug and fixed it. ~~Though DepthGaussianNoise test is still failing and I'm still looking into it~~

Update 3: DepthGaussianNoise is failing in my machine because the test is buggy. Opened Ticket #315

PR NOTE 3

Affected classes are mostly Ogre2RenderTarget, Ogre2RenderTexture and Ogre2DepthCamera

PR NOTE 4

Currently fixing ABI breakage.

Before vs After

Comments

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]

Summary

Checklist

Signed all commits for DCO
codecheck passed (See contributing)

Note to maintainers: Remember to use Squash-Merge

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

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

darksylinc · 2021-05-02T23:35:09Z

Converting this PR back to DRAFT since multiple issues were found

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

darksylinc · 2021-05-08T19:42:54Z

I fixed the issues I found so it's no longer a draft again.

However I wanted to see if the ABI checker would tell me ~~what I broke~~ if I broke something, but the ABI checker seems to be malfunctioning :(

Are there instructions on how to run it locally?

iche033 · 2021-05-11T00:02:24Z

@osrf-jenkins run tests please

iche033 · 2021-05-11T01:59:07Z

However I wanted to see if the ABI checker would tell me what I broke if I broke something, but the ABI checker seems to be malfunctioning :(

something is wring with this particular build machine. I've triggered the build again on a different machine:
https://build.osrfoundation.org/job/ignition_rendering-abichecker-any_to_any-ubuntu_auto-amd64/569/

Are there instructions on how to run it locally?

We don't have public instructions I think. We use abi-compliance-checker. You can probably extract the cmds that we use by looking at the console output of one of the successful ABI builds:
https://build.osrfoundation.org/job/ignition_rendering-abichecker-any_to_any-ubuntu_auto-amd64/564/console

abi-compliance-checker -lang C++ -lib ign-rendering -old pkg.xml -new devel.xml

The contents of pkg.xml and devel.xml are also in the console output.

darksylinc · 2021-05-11T02:28:13Z

We don't have public instructions I think. We use abi-compliance-checker. You can probably extract the cmds that we use by looking at the console output of one of the successful ABI builds:

Sigh. I guess I'll have to get my hands dirty. I hope it's easy. This tool is too useful for this project and my computer is vastly faster than the CI machine.

Thanks for highlighting the commands though

When merging to newer branches that can break the ABI, revert this commit Signed-off-by: Matias N. Goldberg <[email protected]>

darksylinc · 2021-05-15T20:47:56Z

Apparently isRenderWindow is the only function that broke ABI. So I fixed it.

But I could use a 2nd pair of eyes.

If everything's ok it should be ready for merge

PS. I managed to use abi-compatibility-checker in my machine :) Thanks for the help. It just worked!

iche033 · 2021-05-17T20:56:28Z

can you take a look at the latest ABI report from our CI? It found a few more issues:
https://build.osrfoundation.org/job/ignition_rendering-abichecker-any_to_any-ubuntu_auto-amd64/577/API_5fABI_20report/

Usually the Added Symbols issues are harmless. It's mainly the Removed Symbols and Problems with Data Types that cause ABI incompatibility.

iche033 · 2021-05-17T21:19:36Z

ogre2/include/ignition/rendering/ogre2/Ogre2RenderTarget.hh

+      /// We didn't do it to preserve ABI.
+      /// Look in commit history for '#Ogre2IsRenderWindowABI' to
+      /// see changes made and revert
+      public: bool isRenderWindow() const;


capitalize I, i.e. IsRenderWindow

iche033 · 2021-05-17T21:22:03Z

ogre2/include/ignition/rendering/ogre2/Ogre2RenderTarget.hh

+          const std::vector<RenderPassPtr> &_renderPasses,
+          bool _recreateNodes,
+          Ogre::Texture *(*_ogreTextures)[2],
+          bool _isRenderWindow);


add doxygen comments for these new params

we'll need to keep the existing function since adding the args here breaks API/ABI.

darksylinc · 2021-05-17T22:12:01Z

Usually the Added Symbols issues are harmless. It's mainly the Removed Symbols and Problems with Data Types that cause ABI incompatibility.

Mmm....

Ogre2RenderTarget::UpdateRenderPassChain changed signature, so it counts as 1 removal, 1 add.
I can keep two copies, deprecated the old one, and make it throw (since the new code cannot work without the missing arguments). My questions are:

If client application is directly calling Ogre2RenderTarget::UpdateRenderPassChain, it will throw. Does this count as ABI breakage? The client app at this point was interfacing directly with a non-virtual function and AFAIK this is not guaranteed (by Gazebo) to stay stable.
Or is this just to fix potential unrelated ABI breakage? (i.e. removal of function causes unknown crashes in unrelated functions)

Removal of Ogre2RenderTexture::RenderTarget is controversial, because it is a virtual function. What happened was that this:

class Ogre2RenderTarget
{
  virtual OgrePointer* RenderTarget() = 0; // was pure
};

class Ogre2RenderTexture : public Ogre2RenderTarget
{
  virtual OgrePointer* RenderTarget() override;
};

Became this:

class Ogre2RenderTarget
{
  virtual OgrePointer* RenderTarget(); // no longer pure
};

class Ogre2RenderTexture : public Ogre2RenderTarget
{
  // Nothing here
};

From a vtable point of view nothing relevant changed (calling Ogre2RenderTarget::RenderTarget() should still work), but trying to directly call Ogre2RenderTexture::RenderTarget() (i.e. bypassing the vtable) would fail.

I can play on the safe side here and reimplement Ogre2RenderTexture::RenderTarget.

Btw I'm a bit worried about this error:

The layout of v-table has been changed for unknown reason. Call of any method in this class may result in crash or incorrect behavior of applications. is this normal?

iche033 · 2021-05-17T23:52:40Z

If client application is directly calling Ogre2RenderTarget::UpdateRenderPassChain, it will throw. Does this count as ABI breakage? The client app at this point was interfacing directly with a non-virtual function and AFAIK this is not guaranteed (by Gazebo) to stay stable.

We can deprecate old function but instead of making it throw, just print a warning msg to tell users to use the new function (Ignition has a no throw policy which is probably not well documented). We can do something like this to prevent the warning msg being printed out repeatedly: https://github.com/ignitionrobotics/ign-gazebo/blob/ign-gazebo5/include/ignition/gazebo/components/Component.hh#L124-L132

Or is this just to fix potential unrelated ABI breakage? (i.e. removal of function causes unknown crashes in unrelated functions)

In the UpdateRenderPassChain particular case, I think it would be less of a problem. Other issues related to vtable changes and data types found by the ABI build is likely to cause unknown / undefined behavior and crashes.

The layout of v-table has been changed for unknown reason. Call of any method in this class may result in crash or incorrect behavior of applications

We've seen this before and the consensus is that it that we need to fix this issue to avoid unexpected crashes. My guess is that it's related to changes done to the RenderTarget() function.

I can play on the safe side here and reimplement Ogre2RenderTexture::RenderTarget

This is one thing I would try first to see if it helps fix the v-table issue.

The ABI checker also picked up issues with the new ogreTexture[2] var and the change to existing variable materialApplicator which caused changes in memory layout.

Fixing ABI is not fun and usually involves many workarounds :/ I can also take a look at fixing some of these ABI issues later this week if you haven't gotten to it yet.

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

darksylinc · 2021-05-23T20:10:16Z

I updated the code to be ABI compatible.

Maaan I'm beat. This is 20% work, 80% figuring out ABI issues.
If this keeps up I'll just target a newer version.

My ABI checker says 100% compatibility. Let's cross fingers CI agrees.

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

iche033

this should get rid of the build warnings:

diff --git a/ogre2/src/Ogre2RenderTarget.cc b/ogre2/src/Ogre2RenderTarget.cc
index a0e8fda6..59b66397 100644
--- a/ogre2/src/Ogre2RenderTarget.cc
+++ b/ogre2/src/Ogre2RenderTarget.cc
@@ -647,10 +647,11 @@ void Ogre2RenderTarget::UpdateRenderPassChain()
 
 //////////////////////////////////////////////////
 void Ogre2RenderTarget::UpdateRenderPassChain(
-    Ogre::CompositorWorkspace *_workspace, const std::string &_workspaceDefName,
-    const std::string &_baseNode, const std::string &_finalNode,
-    const std::vector<RenderPassPtr> &_renderPasses,
-    bool _recreateNodes)
+    Ogre::CompositorWorkspace * /*_workspace*/,
+    const std::string & /*_workspaceDefName*/,
+    const std::string & /*_baseNode*/, const std::string & /*_finalNode*/,
+    const std::vector<RenderPassPtr> & /*_renderPasses*/,
+    bool /*_recreateNodes*/)
 {
   ignwarn << "Warning: This Ogre2RenderTarget::UpdateRenderPassChain "
           << "overload is deprecated" << std::endl;
@@ -856,9 +857,12 @@ void Ogre2RenderTarget::SyncOgreTextureVars()
 //////////////////////////////////////////////////
 // Ogre2RenderTexture
 //////////////////////////////////////////////////
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
 Ogre2RenderTexture::Ogre2RenderTexture()
 {
 }
+#pragma GCC diagnostic pop
 
 //////////////////////////////////////////////////
 Ogre2RenderTexture::~Ogre2RenderTexture()
@@ -917,7 +921,10 @@ Ogre::RenderTarget *Ogre2RenderTexture::RenderTarget() const
 //////////////////////////////////////////////////
 void Ogre2RenderTexture::SetOgreTexture(Ogre::Texture *_ogreTexture)
 {
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
   this->ogreTexture = _ogreTexture;
+#pragma GCC diagnostic pop
 }
 
 /////////////////////////////////////////////////

iche033 · 2021-05-27T23:55:24Z

ABI report: https://build.osrfoundation.org/job/ignition_rendering-abichecker-any_to_any-ubuntu_auto-amd64/584/API_5fABI_20report/

looking at the data type problem description and the vtable, I think the issue could be due to Ogre2RenderTarget::RenderTarget() no longer being pure virtual. I don't think there's a behavior change since the logic is just moved to the base class and we are not re-implementing a virtual function. This may be ok.

iche033 · 2021-05-27T23:58:44Z

Confirmed that the FSAA issue is fixed. Thanks for iterating!

Maaan I'm beat. This is 20% work, 80% figuring out ABI issues.
If this keeps up I'll just target a newer version.

sure, if the changes are looking like they are not trivial to make to a release version without breaking ABI, you can target main first. We would then evaluate afterwards to see if it's worth backporting them.

Hopefully this will prevent ABI breakage Signed-off-by: Matias N. Goldberg <[email protected]>

darksylinc · 2021-05-28T15:58:23Z

looking at the data type problem description and the vtable, I think the issue could be due to Ogre2RenderTarget::RenderTarget() no longer being pure virtual. I don't think there's a behavior change since the logic is just moved to the base class and we are not re-implementing a virtual function. This may be ok.

I suspect the same. It passed on my machine, but it seems it's not consistent. Just to be sure I made it pure virtual again and let's see what the abi checker says this time.

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

iche033 · 2021-06-01T17:26:01Z

latest ABI build is green :)

the INTEGRATION_render_pass test failed in the latest ubuntu build. I haven't seen that before. I just retriggered another build and it failed again. I haven't been able to reproduce it locally - could be a timing issue. This led me to try out running the ogre2_demo in examples again and noticed that the mouse movement is laggy / jumpy. See gif below. I don't see this happen on ign-rendering5 branch. If I enable the noise render pass (post processing) effect by pressing P, the lag is gone, not sure why

darksylinc · 2021-06-01T18:30:04Z

the INTEGRATION_render_pass test failed in the latest ubuntu build
It's been failing randomly (but quite frequently) on my machine. I was gonna take a look at some point as I feared it could be a problem with the PostRender branch, but interesting to see that it repros here instead!

It may mean that this patch accidentally tries to show the wrong texture, containing uninitialized or old results, which would explain why the error fails (i.e. it contains whatever was in memory)

This led me to try out running the ogre2_demo in examples again and noticed that the mouse movement is laggy / jumpy

This further fuels this theory, if the same texture is shown twice. Definitely worth taking a look. Thanks!

darksylinc · 2021-06-13T17:40:45Z

I'm afraid I couldn't repro any of your problems:

INTEGRATION_render_pass passes
ogre2_demo does not lag

I tried ogre2_demo on the following machines:

AMD Ryzen 5900X + AMD Radeon RX 560 2GB (mesa drivers) + Ubuntu 18.04 LTS (5.11.0-051100-generic)
Intel i7 7700 + AMD Radeon HD 7770 (mesa drivers) + Ubuntu 20.04 LTS (5.8.0-50-generic)
Intel i7 7700 + NVIDIA GeForce 1060 3GB (proprietary drivers) + Ubuntu 20.04 LTS (5.8.0-50-generic)

I noticed the Intel i7 machine keeps randomly complaining about a double free / mem. corruption (sometimes it complains at startup and crashes before the windows comes up, other times it complains during shutdown, other times it doesn't complain at all) but that may have been some build mistake which I am going to make a clean build now to be sure. If problem persists it may be worth looking into for me.

But overall I could not repro none of the two problems :(

Update: i7 machine still compiling, ran Valgrind on the AMD machine and it found 2 things worth looking into (may not be related with my changes):

Invalid write of size 8
  in initCamera(std::shared_ptr<ignition::rendering::v5::Camera>) in /home/matias/Projects/ign/src/ign-rendering/examples/ogre2_demo/GlutWindow.cc:331
  1: ignition::rendering::v5::Ogre2RenderTarget::UpdateRenderPassChain(Ogre::CompositorWorkspace*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<std::shared_ptr<ignition::rendering::v5::RenderPass>, std::allocator<std::shared_ptr<ignition::rendering::v5::RenderPass> > > const&, bool, Ogre::Texture* (*) [2], bool) in /home/matias/Projects/ign/src/ign-rendering/ogre2/src/Ogre2RenderTarget.cc:767
  2: ignition::rendering::v5::Ogre2RenderTarget::UpdateRenderPassChain() in /home/matias/Projects/ign/src/ign-rendering/ogre2/src/Ogre2RenderTarget.cc:622
  3: ignition::rendering::v5::Ogre2RenderTarget::PreRender() in /home/matias/Projects/ign/src/ign-rendering/ogre2/src/Ogre2RenderTarget.cc:416
  4: ignition::rendering::v5::Ogre2RenderTexture::PreRender() in /home/matias/Projects/ign/src/ign-rendering/ogre2/src/Ogre2RenderTarget.cc:906
  5: ignition::rendering::v5::BaseCamera<ignition::rendering::v5::Ogre2Sensor>::PreRender() in /home/matias/Projects/ign/src/ign-rendering/include/ignition/rendering/base/BaseCamera.hh:311
  6: ignition::rendering::v5::BaseVisual<ignition::rendering::v5::Ogre2Node>::PreRenderChildren() in /home/matias/Projects/ign/src/ign-rendering/include/ignition/rendering/base/BaseVisual.hh:335
  7: ignition::rendering::v5::BaseNode<ignition::rendering::v5::Ogre2Object>::PreRender() in /home/matias/Projects/ign/src/ign-rendering/include/ignition/rendering/base/BaseNode.hh:285
  8: ignition::rendering::v5::BaseVisual<ignition::rendering::v5::Ogre2Node>::PreRender() in /home/matias/Projects/ign/src/ign-rendering/include/ignition/rendering/base/BaseVisual.hh:306
  9: ignition::rendering::v5::BaseScene::PreRender() in /home/matias/Projects/ign/src/ign-rendering/src/base/BaseScene.cc:1235
  10: ignition::rendering::v5::Ogre2Scene::PreRender() in /home/matias/Projects/ign/src/ign-rendering/ogre2/src/Ogre2Scene.cc:168
  11: ignition::rendering::v5::BaseCamera<ignition::rendering::v5::Ogre2Sensor>::Update() in /home/matias/Projects/ign/src/ign-rendering/include/ignition/rendering/base/BaseCamera.hh:388
  12: ignition::rendering::v5::BaseCamera<ignition::rendering::v5::Ogre2Sensor>::Capture(ignition::rendering::v5::Image&) in /home/matias/Projects/ign/src/ign-rendering/include/ignition/rendering/base/BaseCamera.hh:397
  13: initCamera(std::shared_ptr<ignition::rendering::v5::Camera>) in /home/matias/Projects/ign/src/ign-rendering/examples/ogre2_demo/GlutWindow.cc:331
  14: run(std::vector<std::shared_ptr<ignition::rendering::v5::Camera>, std::allocator<std::shared_ptr<ignition::rendering::v5::Camera> > >) in /home/matias/Projects/ign/src/ign-rendering/examples/ogre2_demo/GlutWindow.cc:379
  15: main in /home/matias/Projects/ign/src/ign-rendering/examples/ogre2_demo/Main.cc:334
Address 0xe42fb38 is 0 bytes after a block of size 184 alloc'd  1: operator new(unsigned long) in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so
  2: ignition::rendering::v5::Ogre2RenderTarget::Ogre2RenderTarget() in /home/matias/Projects/ign/src/ign-rendering/ogre2/src/Ogre2RenderTarget.cc:123
  3: ignition::rendering::v5::Ogre2RenderTexture::Ogre2RenderTexture() in /home/matias/Projects/ign/src/ign-rendering/ogre2/src/Ogre2RenderTarget.cc:862
  4: ignition::rendering::v5::Ogre2Scene::CreateRenderTextureImpl(unsigned int, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) in /home/matias/Projects/ign/src/ign-rendering/ogre2/src/Ogre2Scene.cc:885
  5: ignition::rendering::v5::BaseScene::CreateRenderTexture() in /home/matias/Projects/ign/src/ign-rendering/src/base/BaseScene.cc:1165
  6: ignition::rendering::v5::Ogre2Camera::CreateRenderTexture() in /home/matias/Projects/ign/src/ign-rendering/ogre2/src/Ogre2Camera.cc:185
  7: ignition::rendering::v5::Ogre2Camera::Init() in /home/matias/Projects/ign/src/ign-rendering/ogre2/src/Ogre2Camera.cc:156
  8: ignition::rendering::v5::Ogre2Scene::InitObject(std::shared_ptr<ignition::rendering::v5::Ogre2Object>, unsigned int, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) in /home/matias/Projects/ign/src/ign-rendering/ogre2/src/Ogre2Scene.cc:928
  9: ignition::rendering::v5::Ogre2Scene::CreateCameraImpl(unsigned int, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) in /home/matias/Projects/ign/src/ign-rendering/ogre2/src/Ogre2Scene.cc:675
  10: ignition::rendering::v5::BaseScene::CreateCamera(unsigned int, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) in /home/matias/Projects/ign/src/ign-rendering/src/base/BaseScene.cc:755
  11: ignition::rendering::v5::BaseScene::CreateCamera(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) in /home/matias/Projects/ign/src/ign-rendering/src/base/BaseScene.cc:750
  12: buildScene(std::shared_ptr<ignition::rendering::v5::Scene>) in /home/matias/Projects/ign/src/ign-rendering/examples/ogre2_demo/Main.cc:263
  13: createCamera(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) in /home/matias/Projects/ign/src/ign-rendering/examples/ogre2_demo/Main.cc:286
  14: main in /home/matias/Projects/ign/src/ign-rendering/examples/ogre2_demo/Main.cc:322

Mismatched free() / delete / delete []
  in run(std::vector<std::shared_ptr<ignition::rendering::v5::Camera>, std::allocator<std::shared_ptr<ignition::rendering::v5::Camera> > >) in /home/matias/Projects/ign/src/ign-rendering/examples/ogre2_demo/GlutWindow.cc:392
  1: operator delete(void*) in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so
  2: std::_Sp_counted_ptr<unsigned char*, (__gnu_cxx::_Lock_policy)2>::_M_dispose() in /usr/include/c++/8/bits/shared_ptr_base.h:377
  3: std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release() in /usr/include/c++/8/bits/shared_ptr_base.h:155
  4: std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count() in /usr/include/c++/8/bits/shared_ptr_base.h:728
  5: std::__shared_ptr<unsigned char, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr() in /usr/include/c++/8/bits/shared_ptr_base.h:1167
  6: std::shared_ptr<unsigned char>::~shared_ptr() in /usr/include/c++/8/bits/shared_ptr.h:103
  7: ignition::rendering::v5::Image::~Image() in /home/matias/Projects/ign/src/ign-rendering/src/Image.cc:34
  8: void __gnu_cxx::new_allocator<ignition::rendering::v5::Image>::destroy<ignition::rendering::v5::Image>(ignition::rendering::v5::Image*) in /usr/include/c++/8/ext/new_allocator.h:140
  9: void std::allocator_traits<std::allocator<ignition::rendering::v5::Image> >::destroy<ignition::rendering::v5::Image>(std::allocator<ignition::rendering::v5::Image>&, ignition::rendering::v5::Image*) in /usr/include/c++/8/bits/alloc_traits.h:487
  10: std::_Sp_counted_ptr_inplace<ignition::rendering::v5::Image, std::allocator<ignition::rendering::v5::Image>, (__gnu_cxx::_Lock_policy)2>::_M_dispose() in /usr/include/c++/8/bits/shared_ptr_base.h:554
  11: std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release() in /usr/include/c++/8/bits/shared_ptr_base.h:155
  12: std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count() in /usr/include/c++/8/bits/shared_ptr_base.h:728
  13: std::__shared_ptr<ignition::rendering::v5::Image, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr() in /usr/include/c++/8/bits/shared_ptr_base.h:1167
  14: std::shared_ptr<ignition::rendering::v5::Image>::~shared_ptr() in /usr/include/c++/8/bits/shared_ptr.h:103
  15: __run_exit_handlers in /build/glibc-S9d2JN/glibc-2.27/stdlib/exit.c:108
  16: exit in /build/glibc-S9d2JN/glibc-2.27/stdlib/exit.c:139
  17: glutMainLoopEvent in /usr/lib/x86_64-linux-gnu/libglut.so.3.9.0
  18: glutMainLoop in /usr/lib/x86_64-linux-gnu/libglut.so.3.9.0
  19: run(std::vector<std::shared_ptr<ignition::rendering::v5::Camera>, std::allocator<std::shared_ptr<ignition::rendering::v5::Camera> > >) in /home/matias/Projects/ign/src/ign-rendering/examples/ogre2_demo/GlutWindow.cc:392
  20: main in /home/matias/Projects/ign/src/ign-rendering/examples/ogre2_demo/Main.cc:334
Address 0x3aff8080 is 0 bytes inside a block of size 1,440,000 alloc'd  1: operator new[](unsigned long) in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so
  2: ignition::rendering::v5::Image::Image(unsigned int, unsigned int, ignition::rendering::v5::PixelFormat) in /home/matias/Projects/ign/src/ign-rendering/src/Image.cc:30
  3: ignition::rendering::v5::BaseCamera<ignition::rendering::v5::Ogre2Sensor>::CreateImage() const in /home/matias/Projects/ign/src/ign-rendering/include/ignition/rendering/base/BaseCamera.hh:381
  4: initCamera(std::shared_ptr<ignition::rendering::v5::Camera>) in /home/matias/Projects/ign/src/ign-rendering/examples/ogre2_demo/GlutWindow.cc:329
  5: run(std::vector<std::shared_ptr<ignition::rendering::v5::Camera>, std::allocator<std::shared_ptr<ignition::rendering::v5::Camera> > >) in /home/matias/Projects/ign/src/ign-rendering/examples/ogre2_demo/GlutWindow.cc:379
  6: main in /home/matias/Projects/ign/src/ign-rendering/examples/ogre2_demo/Main.cc:334

Update 2:

The invalid write is a new bug introduced by me.
The new [] / delete operator mismatch is a pre-existing bug.

Update 3:

ASAN discovered another bug. in Ogre2SelectionBuffer::CreateRTTBuffer this->dataPtr->buffer = new uint8_t[bufferSize]; has bufferSize = 3, but later on we read 4 bytes from it (PF_RGB strikes again).
This is not caused by this PR, so I'll create another PR for that

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

darksylinc · 2021-06-14T00:25:53Z

@iche033 Although I could not repro the bug you mentioned, I fixed a heap corruption that could definitely cause the behavior you were seeing on your machine. Could you try again?

As for the other two bugs I found, they're unrelated to my fixes so I'll create a PR. I think I'll split it into two more PRs because both are simple, but one of them may break ABI (I'm not sure how we can avoid ABI breakage there, the root of the problem is in the syntax declaration)

codecov · 2021-06-14T00:41:03Z

Codecov Report

Merging #313 (81a1263) into ign-rendering5 (e452c39) will increase coverage by 5.28%.
The diff coverage is 71.01%.

❗ Current head 81a1263 differs from pull request most recent head 2e49d36. Consider uploading reports for the commit 2e49d36 to get more accurate results

@@                Coverage Diff                 @@
##           ign-rendering5     #313      +/-   ##
==================================================
+ Coverage           52.52%   57.80%   +5.28%     
==================================================
  Files                 143      161      +18     
  Lines               13274    15851    +2577     
==================================================
+ Hits                 6972     9163    +2191     
- Misses               6302     6688     +386

Impacted Files	Coverage Δ
include/ignition/rendering/ArrowVisual.hh	`100.00% <ø> (ø)`
include/ignition/rendering/AxisVisual.hh	`100.00% <ø> (ø)`
include/ignition/rendering/Camera.hh	`100.00% <ø> (ø)`
include/ignition/rendering/CompositeVisual.hh	`100.00% <ø> (ø)`
include/ignition/rendering/Geometry.hh	`100.00% <ø> (ø)`
include/ignition/rendering/GpuRays.hh	`0.00% <ø> (ø)`
include/ignition/rendering/Image.hh	`100.00% <ø> (ø)`
include/ignition/rendering/Light.hh	`100.00% <ø> (ø)`
include/ignition/rendering/Material.hh	`100.00% <ø> (ø)`
include/ignition/rendering/Mesh.hh	`100.00% <ø> (ø)`
... and 131 more

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 1421901...2e49d36. Read the comment docs.

iche033 · 2021-06-14T23:12:59Z

ok thanks for looking into the ogre2_demo problem and fixing the memory issues.

I did more testing and found that I was also able to reproduce the problem in latest ign-rendering4 branch so it's not introduced by this PR, sorry for the noise! I tested with the 2 memory fixes but the problem is still there. I ticketed #341 with more info added there. I think it came down to high GPU usage that's causing the lag. Surprisingly, the GPU usage drops when post-processing effect enabled - I would have thought that it would go up instead of down.

In any case, since it's not introduced by this PR, the changes look good here. The INTEGRATION_render_pass test is also passing in the last two builds. I think this is good to go.

darksylinc · 2021-06-15T00:14:50Z

Yyyyeeeeessss!!

This PR was starting to drive me a little nuts.

I think it came down to high GPU usage that's causing the lag

Ahh!!! This reminds me that I noticed Ogre2RenderTarget::UpdateRenderPassChain ended up calling recreateAllNodes every frame. It didn't seem like that's intended behavior because it should notice nothing changed and hence early out.

That could be causing the lag. I was going to investigate that, then forgot.

Apparently it would always hit this:

// check if we need to create all nodes or just update the connections.
// if node does not exist then it means it has not been added to the
// chain yet, in which case, we need to recreate the nodes and
// connections
if (!node)
{
  _recreateNodes = true; // --> Always reaches here
}

iche033 · 2021-06-15T04:15:58Z

Ahh!!! This reminds me that I noticed Ogre2RenderTarget::UpdateRenderPassChain ended up calling recreateAllNodes every frame.

that's it! I've created a PR to fix it: #342

darksylinc · 2021-06-15T05:14:08Z

that's it! I've created a PR to fix it: #342

Awesome! I would've probably taken longer to figure that out.

And yeah, it makes sense because shadow node's textures kept being recreated. Resource creation is expensive

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

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

darksylinc requested a review from iche033 as a code owner May 1, 2021 22:12

github-actions bot added the 🏢 edifice Ignition Edifice label May 1, 2021

darksylinc force-pushed the matias-fsaa-fix5 branch from 2b43a63 to 36a94a9 Compare May 1, 2021 22:24

darksylinc force-pushed the matias-fsaa-fix5 branch from 36a94a9 to 3ef8156 Compare May 2, 2021 17:21

Add Ogre2RenderTarget::RenderTarget back

400811d

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

darksylinc marked this pull request as draft May 2, 2021 23:34

darksylinc marked this pull request as ready for review May 8, 2021 16:00

Rewrote the compositor changes to support RenderWindows

a1c1968

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

darksylinc force-pushed the matias-fsaa-fix5 branch from 48b035b to a1c1968 Compare May 8, 2021 16:02

darksylinc mentioned this pull request May 10, 2021

Race condition between Qt and Ogre2 in presentation #304

Closed

5 tasks

Mantain ABI compatibility #Ogre2IsRenderWindowABI

b52af92

When merging to newer branches that can break the ABI, revert this commit Signed-off-by: Matias N. Goldberg <[email protected]>

iche033 reviewed May 17, 2021

View reviewed changes

Fix ABI problems

d83468d

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

darksylinc force-pushed the matias-fsaa-fix5 branch from a095aba to 3bffe08 Compare May 23, 2021 20:12

Fix camel case convention

10d493f

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

darksylinc force-pushed the matias-fsaa-fix5 branch from 3bffe08 to 10d493f Compare May 23, 2021 20:14

Merge branch 'ign-rendering5' into matias-fsaa-fix5

fbcea16

iche033 reviewed May 27, 2021

View reviewed changes

Make Ogre2RenderTarget::RenderTarget pure virtual again

40d04d9

Hopefully this will prevent ABI breakage Signed-off-by: Matias N. Goldberg <[email protected]>

Fix deprecation warnings during build

bd4aa2b

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

Fix invalid write of size 8

9b01d22

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

Merge branch 'ign-rendering5' into matias-fsaa-fix5

2e49d36

iche033 mentioned this pull request Jun 14, 2021

ogre2_demo has high GPU usage and camera movement is laggy / jumpy #341

Closed

2 tasks

iche033 approved these changes Jun 14, 2021

View reviewed changes

iche033 merged commit b29c295 into gazebosim:ign-rendering5 Jun 14, 2021

iche033 mentioned this pull request Jun 15, 2021

Fix ogre2 render pass high GPU usage #342

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix FSAA in UI and lower VRAM consumption #313

Fix FSAA in UI and lower VRAM consumption #313

darksylinc commented May 1, 2021 •

edited

Loading

darksylinc commented May 2, 2021

darksylinc commented May 8, 2021

iche033 commented May 11, 2021

iche033 commented May 11, 2021

darksylinc commented May 11, 2021

darksylinc commented May 15, 2021 •

edited

Loading

iche033 commented May 17, 2021

iche033 May 17, 2021

iche033 May 17, 2021

darksylinc commented May 17, 2021

iche033 commented May 17, 2021

darksylinc commented May 23, 2021

iche033 left a comment

iche033 commented May 27, 2021

iche033 commented May 27, 2021

darksylinc commented May 28, 2021

iche033 commented Jun 1, 2021

darksylinc commented Jun 1, 2021

darksylinc commented Jun 13, 2021 •

edited

Loading

darksylinc commented Jun 14, 2021

codecov bot commented Jun 14, 2021 •

edited

Loading

iche033 commented Jun 14, 2021

darksylinc commented Jun 15, 2021 •

edited

Loading

iche033 commented Jun 15, 2021

darksylinc commented Jun 15, 2021 •

edited

Loading

Fix FSAA in UI and lower VRAM consumption #313

Fix FSAA in UI and lower VRAM consumption #313

Conversation

darksylinc commented May 1, 2021 • edited Loading

PR NOTE 1

PR NOTE 2 (important)

PR NOTE 3

PR NOTE 4

Before vs After

Comments

Summary

Checklist

darksylinc commented May 2, 2021

darksylinc commented May 8, 2021

iche033 commented May 11, 2021

iche033 commented May 11, 2021

darksylinc commented May 11, 2021

darksylinc commented May 15, 2021 • edited Loading

iche033 commented May 17, 2021

iche033 May 17, 2021

Choose a reason for hiding this comment

iche033 May 17, 2021

Choose a reason for hiding this comment

darksylinc commented May 17, 2021

iche033 commented May 17, 2021

darksylinc commented May 23, 2021

iche033 left a comment

Choose a reason for hiding this comment

iche033 commented May 27, 2021

iche033 commented May 27, 2021

darksylinc commented May 28, 2021

iche033 commented Jun 1, 2021

darksylinc commented Jun 1, 2021

darksylinc commented Jun 13, 2021 • edited Loading

darksylinc commented Jun 14, 2021

codecov bot commented Jun 14, 2021 • edited Loading

Codecov Report

iche033 commented Jun 14, 2021

darksylinc commented Jun 15, 2021 • edited Loading

iche033 commented Jun 15, 2021

darksylinc commented Jun 15, 2021 • edited Loading

darksylinc commented May 1, 2021 •

edited

Loading

darksylinc commented May 15, 2021 •

edited

Loading

darksylinc commented Jun 13, 2021 •

edited

Loading

codecov bot commented Jun 14, 2021 •

edited

Loading

darksylinc commented Jun 15, 2021 •

edited

Loading

darksylinc commented Jun 15, 2021 •

edited

Loading