-
Notifications
You must be signed in to change notification settings - Fork 52
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
Fix lidar near plane clipping #356
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/github/workspace/ogre2/src/Ogre2GpuRays.cc:196: Missing spaces around = [whitespace/operators] [4]
This PR almost completely breaks CTU_CRAS_NORLAB robots with Ouster lidars. I tested with Spot and MARV. I think it is because the newly defined near clip plane is inside the lidar's geometry. The lidar radius is 0.0435 m, min range is set to 0.1. If you take 0.1 * 0.6, it gives 0.06, which means the geometry should not obstruct the view, but apparently, it does (most of the time). I think there is something more related to gazebosim/gz-sensors#128 (see the screenshots with rotated robots). This PR makes the rotated behavior much worse (and the non-rotated, too). With this PR: MARV is affected, too: Just to get an idea how does the scaling parameter help, I also tried with Isn't there a simple way to tell the gpu lidar to ignore some geometries? At least those attached to the same link as the lidar sensor? (but that would make problems on models which are just a single link, like MARBLE_HUSKY). Or is it possible to turn on backface culling for the robot meshes? Also, is there a way to visualize the output of the shaders (the depth images) before they go through the 1st and 2nd passes? That could help figuring out a better solution (or finding out where obstructing geometries are). Increasing the near clip in SDF helps a bit, but still some false points are observed:
|
Codecov Report
@@ Coverage Diff @@
## ign-rendering6 #356 +/- ##
==================================================
+ Coverage 53.51% 53.58% +0.06%
==================================================
Files 192 194 +2
Lines 19569 19635 +66
==================================================
+ Hits 10473 10521 +48
- Misses 9096 9114 +18
Continue to review full report at Codecov.
|
yeah I think that may be a problem for other robots and cases where the geometry is meant to be visible.
the rendering engine should be doing culling, and just by looking at the Ouster lidar mesh, the mesh actually has triangles facing inwards, hence they are seen by the camera if it's beyond the near clip plane. I think one solution is to set visibility mask/flags for the lidar and the ouster lidar visual. That way we can tell the sensor to ignore the ouster lidar mesh, and would also be a solution for gazebosim/gz-sensors#128. This would require changes in sdf and ign-sensors, and the robot model.sdf. I'll think about this and see if there is an easier way |
I noticed that. For testing, I removed the sensor mesh and the false points are gone. We def get some weird values when the sensor is inside another geometry. |
Which is, however, very common. I think this deserves a generally usable and good working solution. Or a tutorial for mesh design. Or an automatic warning in console when Gazebo detects all/most points from the lidar projected onto the minimum distance. One general way I was thinking about was removing triangles which intersect the min-distance sphere (maybe a little inflated). But I don't know how to implement it. |
This problem sounds like it needs a custom piece in HlmsPbs (Ogre 2.1+) to apply custom clipping math. Whenever the interpolated attribute of a triangle is gl_ClipDistance[i] < 0 (for any i) the triangle at that pixel will not be rendered. This problem seems like it would need simple math: That way geometry will be clipped spherically, while being able to pull the near plane closer without worrying about geometry of the robot that's between the virtual camera pos and the lidar's min range. How do you want to proceed?
This diagram visually shows the solution: Update: By "customize HlmsPbs" I don't meant modifying Ogre code. I mean using its extensibility options (listeners, custom pieces, or even create a custom class that derives from HlmsPbs) to add custom shader code and pass our own variables to customize rendering without touching Ogre's src. |
I think we can go for this option. It's great we can do this without modifying ogre code. The envCams in Ogre2GpuRays are the ones that will need this custom clipping: for now, custom clipping should always be on for these env cameras but not for other cameras. Ideally after the customization is done, we can easily apply them to other cameras later if needed, e.g. we are still considering if we need to do this for Ogre2DepthCamera. Note that we are currently doing additional clipping in the shaders so I think we'll need to remove that logic: https://github.com/ignitionrobotics/ign-rendering/blob/ign-rendering4/ogre2/src/media/materials/programs/gpu_rays_1st_pass_fs.glsl#L125-L126 |
Great!
Ok!
From what you're saying and what I can see, yes that test should be redundant.
You asked this one via email but I'm replying it here: I don't think so. API and ABI should not break. |
The latest added commit (7fb493b) made a difference - it removed the ball around the lidar, but the far points are still not displayed with the robot rotated. |
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
I've noticed some strange behavior. I modified the
The rays produced are truncated, such as in this image: When I increase the samples to 40, I get better results, but some of the side rays are still short. |
Signed-off-by: Ian Chen <[email protected]>
I increased the minimum texture size of the for the raw depth texture that data is sampled from. The currently setup does not work well for large FOV with low sample count. After testing with a 360 degree lidar and 4 samples, I found that I had to increase the min texture size to 128x128 (from 2x2) in order to produce more correct range results. 1f50023 |
Signed-off-by: Ian Chen <[email protected]>
With the increase of minimum lidar texture size, wouldn't the performance degrade too much for point lidars? Could you please test it on CTU_CRAS_NORLAB_MARV_SENSOR_CONFIG_1 which has 4 point lidars each simulated by a 3x3 ray lidar, and a 128-ray Ouster? |
Rasterization-based GPUs (vs raytraced-based GPUs) aren't efficient when low resolutions are involved thus I wouldn't be surprised there's not much perf difference between 2x2 and 128x128, nonetheless it'd need to be profiled Edit: this doesn't apply to CPU emulation (e.g. Mesa SW rasterization) |
here's a performance comparison of the 3x3 lidar sensor that I took from the CTU_CRAS_NORLAB_MARV_SENSOR_CONFIG_1 model. The time shown in the graph is the time it takes to render a frame on my machine before ( like @darksylinc mentioned, It's good to see that there isn't noticeable difference for this particular lidar configuration. I have an nvidia card / driver and I'm not using SW rendering As for the VRAM usage, a quick test launching a simple ignition world with a a 3x3 lidar and a box, it increased slightly from 263MB to 268MB, which is small so I think that's acceptable. |
Great, that sounds good. Could you also test with software rendering? Is there actually a way to determine whether SW or HW rendering is used? Maybe different techinques could be used for SW and HW rendering... |
The EGL windowless feature includes a way to select the device (SW mesa appears as a device) I'm not sure about Nvidia but normally If it still doesn't work then try And obviously check Ogre.log to see what driver is being used |
I ran into the another issue doing this. Same error as the one described in this post so I followed one of the methods mentioned there: Here's what I did:
and that got me SW rendering.
Here's the plot. Note I manually removed the time for the first frame as that takes much longer (0.11 seconds) to render and changes the y scale quite a bit Probably slightly higher average framerate but hard to tell. So it looks like there also isn't noticeable difference for this lidar configuration. |
Fix lidar near plane clipping
- Update Metal shaders to match changes made in PR gazebosim#356 - Update CrossPlatformSettings_piece to #define outVs_gl_ClipDistance - Update Ign_piece_vs_any to use cross platform version of gl_ClipDistance Signed-off-by: Rhys Mainwaring <[email protected]>
- Update Metal shaders to match changes made in PR gazebosim#356 - Update CrossPlatformSettings_piece to #define outVs_gl_ClipDistance - Update Ign_piece_vs_any to use cross platform version of gl_ClipDistance - cherry-pick a2a3760 Signed-off-by: Rhys Mainwaring <[email protected]>
- Update Metal shaders to match changes made in PR gazebosim#356 - Update CrossPlatformSettings_piece to #define outVs_gl_ClipDistance - Update Ign_piece_vs_any to use cross platform version of gl_ClipDistance - cherry-pick a2a3760 Signed-off-by: Rhys Mainwaring <[email protected]>
- Update Metal shaders to match changes made in PR gazebosim#356 - Update CrossPlatformSettings_piece to #define outVs_gl_ClipDistance - Update Ign_piece_vs_any to use cross platform version of gl_ClipDistance - cherry-pick a2a3760 Signed-off-by: Rhys Mainwaring <[email protected]>
- Update Metal shaders to match changes made in PR gazebosim#356 - Update CrossPlatformSettings_piece to #define outVs_gl_ClipDistance - Update Ign_piece_vs_any to use cross platform version of gl_ClipDistance - cherry-pick a2a3760 Signed-off-by: Rhys Mainwaring <[email protected]>
- Update Metal shaders to match changes made in PR gazebosim#356 - Update CrossPlatformSettings_piece to #define outVs_gl_ClipDistance - Update Ign_piece_vs_any to use cross platform version of gl_ClipDistance - cherry-pick a2a3760 Signed-off-by: Rhys Mainwaring <[email protected]>
- Update Metal shaders to match changes made in PR gazebosim#356 - Update CrossPlatformSettings_piece to #define outVs_gl_ClipDistance - Update Ign_piece_vs_any to use cross platform version of gl_ClipDistance - cherry-pick a2a3760 Signed-off-by: Rhys Mainwaring <[email protected]>
* [Metal] enable loading of the Metal render system for ogre2 - Add parameter "metal" and a member variable useMetalRenderSystem to Ogre2RenderEngine - Modify LoadPlugins to load the Metal render system if enabled - Modify CreateRenderSystem to select the Metal render system - Modify RegisterHlms to load the Metal resources - Add missing Metal shaders for the Terra (from Ogre2) Signed-off-by: Rhys Mainwaring <[email protected]> * [Metal] enable metal render system for some examples Signed-off-by: Rhys Mainwaring <[email protected]> * [Metal] add Metal shader for skybox and update material - Add Metal shader for skybox - Update skybox material to support universal shaders - Create subdirectories for GLSL and Metal in media/materials/programs and update Ogre2RenderEngine with new resource locations Signed-off-by: Rhys Mainwaring <[email protected]> * [Metal] add Metal and unified shaders to each material (wip) - Add Metal and unified shaders to each material - Add placeholders for a number of shaders to prevent compile errors when running examples Signed-off-by: Rhys Mainwaring <[email protected]> * [Metal] enable metal render system for remaining examples [do not merge] - Note: these changes will need to be modified / reverted before merging Signed-off-by: Rhys Mainwaring <[email protected]> * [Metal] update examples to accept optional command line option "metal" - Examples accept an additional command line option metal when specifying the ogre2 render engine. - Pass parameters to the createCamera function. Signed-off-by: Rhys Mainwaring <[email protected]> * [Metal] fix formatting on example ogre2_demo Signed-off-by: Rhys Mainwaring <[email protected]> * [Metal] revert changes to example text_geom - This example is for ogre only (not ogre2) Signed-off-by: Rhys Mainwaring <[email protected]> * [Metal] update formatting of shaders - Minor formatting changes to plain_color, point and skybox Metal shaders. Signed-off-by: Rhys Mainwaring <[email protected]> * [Metal] port depth_camera and selection_buffer shaders - Port depth_camera_vs and selection_buffer_fs to Metal - The mouse_picker, ogre2_demo and transform_control demos now work correctly. Signed-off-by: Rhys Mainwaring <[email protected]> * [Metal] shader formatting - replace tabs with whitespace Signed-off-by: Rhys Mainwaring <[email protected]> * [Metal] port gaussian_noise shaders - Port gaussian_noise vertex and pixel shaders from the GLSL counterparts - The ogre2_demo and render_pass examples now work correctly. Signed-off-by: Rhys Mainwaring <[email protected]> * [Metal] port thermal_camera pixel shader - Correct error in thermal material script - Port thermal_camera pixel shaders from the GLSL counterparts - The thermal_camera examples now runs but further requires testing Signed-off-by: Rhys Mainwaring <[email protected]> * [Metal] add placeholders for remaining shaders - Add stubs for the remaining vertex and pixel shaders to port from GLSL Signed-off-by: Rhys Mainwaring <[email protected]> * [Metal] port heat_signature pixel shader Signed-off-by: Rhys Mainwaring <[email protected]> * [Metal] port depth_camera and gpu_rays shaders [wip] - Initial ports of depth_camera and gpu_rays shaders - This is work in progress: - On Metal the compositor in Ogre2GpuRay has a pixel format exception - Texture sampling for depth_camera_final_fs.metal is not using the equivalent of OpenGL's texelFetch Signed-off-by: Rhys Mainwaring <[email protected]> * [Metal] update custom_scene_viewer example to accept optional command line option "metal" Signed-off-by: Rhys Mainwaring <[email protected]> * [Metal] fix fragment program specifier in gaussian noise material - Unified shader had incorrect specifier Signed-off-by: Rhys Mainwaring <[email protected]> * [Metal] fix depth camera vertex shader not found in selection buffer material - This is the same issue as fixed in #456 Signed-off-by: Rhys Mainwaring <[email protected]> * switch to user RGBA format for internal textures in gpu ray sensor Signed-off-by: Ian Chen <[email protected]> * [Metal] complete port of gpu rays 1st pass shader - Complete port of the GPU Rays shader - This depends upon the pixel format fix in #468 Signed-off-by: Rhys Mainwaring <[email protected]> * [Metal] update shaders to fix lidar near plane clipping - Update Metal shaders to match changes made in PR #356 - Update CrossPlatformSettings_piece to #define outVs_gl_ClipDistance - Update Ign_piece_vs_any to use cross platform version of gl_ClipDistance Signed-off-by: Rhys Mainwaring <[email protected]> * Fix style for code check - Remove line end whitespace causing CI to fail. Signed-off-by: Rhys Mainwaring <[email protected]> * [Metal] incorporate review feedback - Revert debug output in Ogre2RenderEngine - Fix codecheck issue with variable declaration shadows previous local - Revert changes to simple_demo_qml - Move useMetalRenderSystem to Ogre2RenderEnginePrivate Signed-off-by: Rhys Mainwaring <[email protected]> * [Metal] introduce enum for graphics interface - Add enum class GraphicsAPI to RenderEngine.hh - Add utils class GraphicsAPIUtils for setting enum from string - Update Ogre2RenderEngine to use GraphicsAPI enum - Update examples to use GraphicsAPI enum Signed-off-by: Rhys Mainwaring <[email protected]> * [Metal] refactor enum for graphics interface - Move GraphicsAPI to own file Signed-off-by: Rhys Mainwaring <[email protected]> * [Metal] fix examples - Fix custom_scene_viewer Signed-off-by: Rhys Mainwaring <[email protected]> * [Metal] fix class description - Fix code check - Change GraphicsAPIUtils class documentation - Improve graphics API check in Ogre2RenderEngine Signed-off-by: Rhys Mainwaring <[email protected]> * [Metal] fix codecheck - Fix code check - uninitialised variable warning Signed-off-by: Rhys Mainwaring <[email protected]> Co-authored-by: Ian Chen <[email protected]>
Signed-off-by: Ian Chen [email protected]
🦟 Bug fix
Fixes osrf/subt#888
Summary
In our GPU lidar implementation, the min range currently translates to the near clip plane of the cube camera used to sample the depth values of the scene. This is not correct. The problem is that objects that are near the corner of the image can be incorrectly clipped.
For example, take a lidar with a min range of 1m. An object is now placed at a position at
p = [0.9, 0.9, 0]
relative to the robot (0.9m in front of it and 0.9m to its left). The object should be visible because it is beyond the min range, i.e.length(p)
issqrt(0.9*0.9 + 0.9*0.9) = 1.27
, which is larger than min range. However, the current implementation clipsp
because the clipping plane is at 1m which is larger than the 0.9.To fix this, we set a smaller near clip plane for the cube camera and let the shaders handle the clipping of range values.
For comparison, take a look at the point clouds produced by the cerberus gagarin robot in osrf/subt#888. The point clouds at the bottom is box shaped because of incorrect clipping. Here're the point clouds after the fix. It's now a spherical shape :
Testing with ign-gazebo
Another test is to run ign-gazebo with the near_clip.sdf world:
Points
Checklist
codecheck
passed (See contributing)Note to maintainers: Remember to use Squash-Merge