Use clipping distance for minimum LIDAR distance

[darksylinc]

Using Hlms custom pieces and a listener to implement it
See changes to Ogre2GpuRays::Render for usage
Update ogre2/src/media/Hlms/Pbs/GLSL/VertexShader_vs.glsl to latest
upstream version

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

🦟 Bug fix

Summary

As discussed in #356 this implements Hlms custom pieces so that LIDAR can clip correctly without the undesired side effects of pushing the near clip plane closer.

I forgot to ask for an actual test; so I did not test whether the solution does the intended job; however I'm very confident it should.
I was able to test though, that the code works by forcing it to clip on regular view

Ogre2IgnHlmsCustomizations is purposely not intended to be exposed to the user. This class could vary too much between updates (including new Ogre releases; although we try to avoid changes for patch releases) and trying to maintain ABI compatibility would be hell.

Also a user trying to mess with Ogre2IgnHlmsCustomizations could potentially fiddle too much with low level workings of rendering. Hence it's best to not expose it at all.

Usage

Use:

auto engine = Ogre2RenderEngine::Instance();
  Ogre2IgnHlmsCustomizations &hlmsCustomizations =
      engine->HlmsCustomizations();

hlmsCustomizations.minDistanceClip = minLidarRange;

To enable it.

After rendering, you can use the following to disable the feature (any negative value works):

hlmsCustomizations.minDistanceClip = -1;

A value of 0 will keep the feature enabled but won't make any visible effect. The reason for this behavior (i.e. why doesn't 0 disable the feature?) is that when the feature is toggled on/off for a given pass then new shaders may be generated, causing stutter or performance problems (and higher RAM consumption).

Keeping the feature enabled at minDistanceClip = 0 lowers the chance of useless shader generation; and I suspect it could happen during UI editing (e.g. when sliding a value between a 0 and a max range)

Problem

UPDATE: This problem has been fixed. Living the comments for history's sake. Scroll to bottom to find the solution. This PR is ready for review

This is currently a DRAFT because marking the code like the following:

inline namespace IGNITION_RENDERING_VERSION_NAMESPACE {
class IGNITION_RENDERING_OGRE2_VISIBLE Ogre2IgnHlmsCustomizations final :
    public Ogre::HlmsListener

results in a runtime crash:

Error while loading the library [/home/matias/Projects/ign/install/lib/ign-rendering-4/engine-plugins/libignition-rendering-ogre2.so]: /home/matias/Projects/ign/install/lib/ign-rendering-4/engine-plugins/libignition-rendering-ogre2.so: undefined symbol: _ZTVN8ignition9rendering2v426Ogre2IgnHlmsCustomizationsE

Which c++filt translates to:

vtable for ignition::rendering::v4::Ogre2IgnHlmsCustomizations

No matter what I do I can't seem to fix the problem unless I add a Ogre2IgnHlmsCustomizations member to a public API e.g.

class IGNITION_RENDERING_OGRE2_VISIBLE Ogre2RenderEngine :
  public virtual BaseRenderEngine,
  public common::SingletonT<Ogre2RenderEngine>
{
  private: Ogre2IgnHlmsCustomizations *foo;
};

But I can't do that because that'd break ABI (and is also unnecessary).

So far I had to workaround this problem via:

// TODO(anyone) Can't use inline namespace here. At runtime it will complain
// vtable for ignition::rendering::v4::Ogre2IgnHlmsCustomizations
// was not found
// inline namespace IGNITION_RENDERING_VERSION_NAMESPACE {
class IGNITION_RENDERING_OGRE2_HIDDEN Ogre2IgnHlmsCustomizations final :
      public Ogre::HlmsListener
{
}

Note that IGNITION_RENDERING_OGRE2_HIDDEN or IGNITION_RENDERING_OGRE2_VISIBLE doesn't seem to make a difference.

With my workaround INTEGRATION_versioned_symbols_ogre2 is now obviously failing.

Help is appreciated on how to resolve this problem because I'm out of ideas.

Other than that, the code should be functional and working, hopefully solving the intended problem

OMG I FOUND THE PROBLEM:
Some times IGNITION_RENDERING_VERSION_NAMESPACE is a macro, sometimes it's not. So the actual symbol ends up as ignition::rendering::IGNITION_RENDERING_VERSION_NAMESPACE::Ogre2IgnHlmsCustomizations instead of ignition::rendering::v4::Ogre2IgnHlmsCustomizations

Checklist

• Signed all commits for DCO
• Added tests
• Updated documentation (as needed)
• Updated migration guide (as needed)
• codecheck passed (See contributing)
• All tests passed (See test coverage)
• While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

[codecov]

Codecov Report

Merging #365 (a13ad05) into lidar_near_clip (dd8568a) will increase coverage by 0.14%.
The diff coverage is 98.38%.

❗ Current head a13ad05 differs from pull request most recent head 1b6733f. Consider uploading reports for the commit 1b6733f to get more accurate results

@@                 Coverage Diff                 @@
##           lidar_near_clip     #365      +/-   ##
===================================================
+ Coverage            58.15%   58.30%   +0.14%     
===================================================
  Files                  170      172       +2     
  Lines                16790    16852      +62     
===================================================
+ Hits                  9764     9825      +61     
- Misses                7026     7027       +1     

Impacted Files	Coverage Δ
...lude/ignition/rendering/ogre2/Ogre2RenderEngine.hh	100.00% <ø> (ø)
ogre2/src/Ogre2IgnHlmsCustomizations.cc	97.50% <97.50%> (ø)
ogre2/src/Ogre2GpuRays.cc	95.15% <100.00%> (+0.10%)	⬆️
ogre2/src/Ogre2IgnHlmsCustomizations.hh	100.00% <100.00%> (ø)
ogre2/src/Ogre2RenderEngine.cc	79.05% <100.00%> (+0.47%)	⬆️

---

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 dd8568a...1b6733f. Read the comment docs.

[ahcorde]

for the path please use common::JoinPath()

[iche033]

cc04f5e

[ahcorde]

Suggested change

	archiveUnlitLibraryFolders.push_back( customizationsArchiveLibrary );
	archiveUnlitLibraryFolders.push_back(customizationsArchiveLibrary);

[iche033]

cc04f5e

[ahcorde]

Suggested change

	archivePbsLibraryFolders.push_back( customizationsArchiveLibrary );
	archivePbsLibraryFolders.push_back(customizationsArchiveLibrary);

[iche033]

cc04f5e

[ahcorde]

Suggested change

	for (size_t i = 0; i < 16u; ++i)
	*_passBufferPtr++ = static_cast<float>(invViewProj[0][i]);
	for (size_t i = 0; i < 16u; ++i)
	{
	*_passBufferPtr++ = static_cast<float>(invViewProj[0][i]);
	}

[iche033]

cc04f5e

[iche033]

Looks promising. The point clouds appear to be rendering correctly now:

I'll do more tests with different robots.

[iche033]

nit: put & next to variable, i.e. Ogre2IgnHlmsCustomizations &HlmsCustomizations();

[iche033]

cc04f5e

[iche033]

Suggested change

	_hlms->_setProperty( "ign_spherical_clip_min_distance", 1 );
	_hlms->_setProperty( "ign_spherical_clip_idx", numClipPlanes );
	_hlms->_setProperty( "hlms_pso_clip_distances", numClipPlanes + 1 );
	_hlms->_setProperty("ign_spherical_clip_min_distance", 1);
	_hlms->_setProperty("ign_spherical_clip_idx", numClipPlanes);
	_hlms->_setProperty("hlms_pso_clip_distances", numClipPlanes + 1);

[iche033]

cc04f5e

[iche033]

After doing more testing I think this almost fixes the issue found in #356 (comment). The only problem left is that as soon as I rotate the robot to something other than multiple of 90 degrees, the sensor's view become occluded in the same way described in gazebosim/gz-sensors#128. My guess is that it's an object culling issue based on axis aligned bounding box. @darksylinc do you have any insights?

I think near clipping is working so we can merge this first if you think gazebosim/gz-sensors#128 is a separate issue.

[darksylinc]

I saw you made a new commit after this msg, so perhaps you've already solved it.

As for the problem you're describing (starts breaking when rotated) I can think of 4 reasons:

1. Floating point precision shenanigan. Just adjust the numbers a nudge.

2. Rotation is not what we're expecting. I noticed Gazebo's camera has a complex node setup (Camera is attached to a node which is child of a node of a node). If rotation isn't handling correctly we are thinking this is what's happening:

But this is what's actually happening (or any variation of it):

3. Like you said the postprocessing is discarding it out at 90°, but somehow the math fails otherwise

4. I don't really think this is it, but if the camera is rotated after SceneManager::updateSceneGraph weird things can happen. I didn't see this happening in out of the box Gazebo UI, but I don't usually test LIDAR render code except for the rgbd file you sent me.

Probably the best way to see this is to take a RenderDoc capture of 0°, 45°, 90° and compare them drawcall by drawcall to see where they diverge. Although RenderDoc capture should be done in ign-rendering5 which is where we fixed the race condition bug (without the fix RenderDoc in the capture calls are all out of order or messed up)

[darksylinc]

I just noticed he said:

This happens with "camera" and "thermal" type sensors. Types "rgbd" (both color and depth) and "depth" are not affected. The fact that the color camera from "rgbd" sensor is not affected might point to a possible direction of research for this bug...

Please note my camera changes were made to Ogre2GpuRays, not to Ogre2ThermalCamera. However it can be activated for Ogre2ThermalCamera as well.

[peci1]

I can confirm @iche033 's observations - axis-aligned robots render the cloud correctly, but rotated do not.

This is what I get with a rotated MARV - I haven't seen this kind of behavior before.

[darksylinc]

I'm relatively new: how do I test rotated vs non rotated?

[peci1]

If you open Ignition Gazebo GUI, there is a plugin called Transform tools, and that one has the rotate tool. Click the robot and turn the ring to rotate the robot.

You can view the pointclouds in Ignition Gazebo GUI using the Visualize Lidar plugin, but if you turn it on for one of these high-density 3D lidars, performance suffers a lot. It's best to first choose the "Points" type of visualization and only after that select the lidar topic.

Or, if you're testing using the SubT challenge simulator, just launch competition.ign which will also launch the required ros_ign bridges and you can then display the pointcloud in rviz (which I like much more)

[iche033]

I created a minimal example world for testing: near_clip.sdf

1. To test, launch ign gazebo

ign gazebo -v 4 -r near_clip.sdf

2. Load the Visualize Lidar plugin by going to top right menu (3 dots) and select Visualize Lidar
3. Change Visual Type from Triangle Stips to Points
4. Press the orange refresh button next to the drop down list to subscribe to the point cloud data, You should see point cloud visualization.
5. Select the lidar mesh in the middle of the scene.
6. Press R for the rotate tool and rotate the lidar to 45 degrees to see the issue

[darksylinc]

THIS IS PRELIMINARY, I'M STILL RESEARCHING

OK so I was off to a bad start because this is the output I see on the GUI. Fortunately the bug still repros on the server so it doesn't actually affect my work:

So I tried RenderDoc on the server (after a couple minor mods so that RenderDoc capture works with ign-rendering4 in server mode)

The "bad" version (i.e. rotated) is rendering 3 objects while the "good" version only renders 1 object:

1. Both versions are rendering the cube (36 / 3 = 12 triangles):

2. Bad version is rendering the sensor (13680 / 3 = 4560 triangles). This means two things at the same time:
   a. The HlmsCustomizations code I wrote is not being used. I can see it in the shader. The code is not there. I need to find out why. But I can notice from a quick look that is not using Unlit nor Pbs, but a low level material (hence min clip distance won't work on this model unless explicitly added to each low level material that uses it)
   b. Ogre did not submit to GPU the sensor in the "good" version, but it did submit it in the "bad" version. This is likely a culling issue. It could be a precision bug. But still I'll research what's going on there

3. Bad version is rendering a 2-triangle object where both triangles are degenerate (e.g. AAB and BBC) therefore it has no visual impact, but I have no clue what that is. Looks like it should be the plane / ground, but the invalid geometry puts me off.
   It could be some helper geometry gazebo uses for something else that is accidentally being left enabled during this render pass.

It's also curious why Ogre submits it to the GPU when 45°, but not at 0°

Next steps

1. I'll look into why Ogre is culling the sensor at 0° but not at 45° (could be not an Ogre bug, e.g. if the camera is at a bad position)
2. I have to add clipping code to the low level material (or find a better alternative)
3. Keep an eye on that bizarre 2-triangle invalid geometry, but for now lower priority. Maybe it's corruption related to the triangle going haywire in the GUI version (i.e. server corrupts a plane, GUI version corrupts the lidar geometry instead)

[darksylinc]

Oh btw the patch to add RenderDoc support to server mode is this quick hack (do not push it to production!)

diff --git a/ogre2/src/Ogre2RenderEngine.cc b/ogre2/src/Ogre2RenderEngine.cc
index 8a240e46..3f20e05f 100644
--- a/ogre2/src/Ogre2RenderEngine.cc
+++ b/ogre2/src/Ogre2RenderEngine.cc
@@ -42,6 +42,9 @@
 
 #include "Ogre2IgnHlmsCustomizations.hh"
 
+extern Ogre::RenderWindow *g_window;
+Ogre::RenderWindow *g_window = nullptr;
+
 class ignition::rendering::Ogre2RenderEnginePrivate
 {
 #if !defined(__APPLE__) && !defined(_WIN32)
@@ -752,6 +755,7 @@ std::string Ogre2RenderEngine::CreateRenderWindow(const std::string &_handle,
     {
       window = this->ogreRoot->createRenderWindow(
           stream.str(), _width, _height, false, &params);
+      g_window = window;
     }
     catch(const std::exception &_e)
     {
diff --git a/ogre2/src/Ogre2Scene.cc b/ogre2/src/Ogre2Scene.cc
index 91612cf6..3e511051 100644
--- a/ogre2/src/Ogre2Scene.cc
+++ b/ogre2/src/Ogre2Scene.cc
@@ -43,6 +43,10 @@
 #include "ignition/rendering/ogre2/Ogre2Visual.hh"
 #include "ignition/rendering/ogre2/Ogre2WireBox.hh"
 
+#include "OgreRenderWindow.h"
+
+extern Ogre::RenderWindow *g_window;
+
 /// \brief Private data for the Ogre2Scene class
 class ignition::rendering::Ogre2ScenePrivate
 {
@@ -135,6 +139,11 @@ void Ogre2Scene::PreRender()
     }
   }
 
+  g_window->swapBuffers();
+  //static int frame = 0;
+  //printf( "frame %i\n", frame++ );
+  //fflush( stdout );
+
   BaseScene::PreRender();
 }

[darksylinc]

OK the 3rd "invalid geometry" mesh mystery has been revealed.

ouster-os0.dae contains two submeshes: the main one; and a 2nd one which is not a triangle but a line list. I missed that part where it's not a triangle. But still it's a degenerate line because it's just 3 lines of 0 length each.

Because its part of the sensor geometry, if Ogre submits the geometry it will also submit the line. And if the sensor should not be submitted, the line will also not be submitted.

[darksylinc]

Well... this was FUN! (no sarcasm, it was fun)

Short version:

1. Ogre's frustum culling is working as intended
2. Ogre incorrectly forces depth clamp (requires an Ogre 2.1 update)
3. ign-rendering is not applying gl_ClipDistance to low level materials (fixing this is enough, no need to urgently update to latest Ogre 2.1*)
4. The triangle soup in the GUI was not investigated

(*) However I'd recommend an update at some point, since who knows what other side effects forcing depth clamping does (probably only manifest in edge cases like this one).

Long version:

Several things going on.

Why does Ogre not submit the sensor geometry to GPU at 0°?

Because it fails the frustum test.

With the near plane at 0.06 and the lidar's AABB being in range [-0.037; 0.037], the lidar is well behind the near plane:

However Ogre simplifies calculations by working with AABBs, not Oriented Bounding Boxes. When rotating an object by 45°, we use the AABB the encloses the rotated object. This means that its AABB gets bigger when rotated, and gets smaller again when getting close to being aligned to 0/90/180/270°:

When the object is at 45°, its AABB is bigger, and now this is already enough to pass the frustum test:

But let's remember that AABB is bigger but still aligned (never rotated), but the frustum is rotated. Hence this is the actual representation of the test:

It's more than enough to pass frustum test and Ogre sends the geometry to GPU.

IIRC this math simplification was there in Ogre 1.x too (the code is vastly different though). But it's possible small details change and the observable behavior ends up being different.

This part so far is working as intended.

If I edit Ogre to remove the frustum test completely (i.e always pass), then the buggy behavior happens at all orientations.

Why is the object on screen blocking the lidar?

Because Ogre 2.1 is incorrectly activating depth clamp.
Without depth clamp, the GPU would just clip the geometry as expected. With depth clamp, the GPU forces everything that is z < 0 to be z = 0 thus covering all of the screen.

Very old legacy code meant for v1 objects, RenderSystem::setClipPlane, is always being activated, causing depth clamp to be enabled. The solution was to remove this super legacy snippet because it doesn't seem like anyone is using it and only GL3+ does this (also depth clamp was properly introduced in Ogre 2.3).
This bug was affecting all Ogre branches (even 2.2 and 2.3)

Offtopic: I had a different theory and made a test but I couldn't find anything. I was wrong. But at least our coverage increased).

• Unfortunately this requires an Ogre 2.1 update.
• Fortunately that won't matter once the missing gl_ClipDistance is added to the low level material.

Solutions

Any of the following will fix it:

1. Update to latest Ogre 2.1 (literally fixed today to all branches)
2. Update the low level material to use gl_ClipDistance

[darksylinc]

OK I pushed the fix.

There's good and bad news:

1. It works!
2. It needs an Ogre update. In particular this commit.

Unfortunately I forgot shaders had to inform they need to use gl_ClipDistance and low level materials did not have such interface.
Without an Ogre 2.1 update the keyword num_clip_distances will be ignored thus it's as if the fix weren't there.

A solution without an Ogre 2.1 update would need us to switch from using low level materials to an Hlms implementation (or overriding Pbs & Unlit).

While this may be more ideal for performance and flexibility (i.e. the current solution being used is ignoring skeletal animations), that requires more time, analysis, testing and could potentially break ABI or other stuff. This is something we could discuss for Fortress.

Hence an update to Ogre 2.1 was the quickest path.

[darksylinc]

OK the triangle soup is a locale problem when parsing the DAE.

I have to check if gazebosim/gz-common#120 was applied, and if so, why it's not working.

I'll take a look

[peci1]

I have to check if ignitionrobotics/ign-common#120 was applied, and if so, why it's not working.

No, the PR you refer to is declined. I can confirm that with Dome, the locale issues are still there.

[darksylinc]

No, the PR you refer to is declined. I can confirm that with Dome, the locale issues are still there.

Oh! Great point! Thanks! That explains the issue.

[iche033]

thanks for digging deep into this and coming up with a fix! Given that this requires an ogre update, we decided that we'll have to retarget this fix to the Fortress branch (currently main) that's going to use ogre 2.2. Looks like the fix: OGRECave/ogre-next@dc23bf0 has been ported forward to ogre 2.2 so we can pull to get the fix. As for users on Dome / ign-rendering4, I think one workaround is just to increase the near clip distance.

A solution without an Ogre 2.1 update would need us to switch from using low level materials to an Hlms implementation (or overriding Pbs & Unlit).

Let's hold off on doing this. I'm inclined to keep the low level material implementation as it's fairly stable and we'll soon be preparing for the Fortress release.

I've just retargeted my branch to lidar_near_clip to main, which created conflicts here. Can you resolve the conflicts here? Thanks. I also just tested this without updating to latest ogre 2.2 - the issue is still there as expected but it looks like lidar is still functioning fine with the new material and shader changes. Can you confirm if we can merge this before the updated ogre 2.2 debs are ready?

[darksylinc]

As for users on Dome / ign-rendering4, I think one workaround is just to increase the near clip distance.

Increasing the clip distance will only work if it's big enough so that frustum culling prevents Ogre from sending the object to GPU. Otherwise the GPU won't correctly clip the geometry due to depth clamp incorrectly activated

Alternative solutions:

• Build Gazebo from source with your own Ogre source (i.e. latest version from git) which no longer force-enables depth clamp
• Use render queue ids, scene visibility flags, or Item::setVisible to avoid rendering that object. I'm not sure what is available to gazebo users though

[darksylinc]

Can you confirm if we can merge this before the updated ogre 2.2 debs are ready?

I'm not sure what you're asking? What is it that I need to confirm?

The HlmsListener probably had a few interface changes so the overloaded virtuals may 'just work', or warn that we're overriding a non-virtual function (because the argument signature changed slightly), or error that there's a pure virtual function.

[iche033]

I'm not sure what you're asking? What is it that I need to confirm?

ah I was mainly thinking of the changes in the material and shader files, e.g. the addition of num_clip_distances 1. As you mentioned before, they will be just be ignored. I don't see any complaints / error msgs in the ogre log so just want to double check if it's safe to get in without waiting for our ogre 2.2 to be updated with your commit.

[darksylinc]

I've merged with main branch.

I checked the code regarding the use of num_clip_distances in older versions of Ogre: It will be silently ignored (not even a warning) and it should have no side effects either.

[iche033]

I'll take care of @ahcorde's comments in my branch

[iche033]

I applied the patch on top of our ogre 2.x fork and tested with the minimal example world, near_clip.sdf

The lidar point clouds now look consistent at different angles.

will address these comments in #356