Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce lidar data discretization #296

Merged
merged 6 commits into from
Apr 24, 2021
Merged

Conversation

iche033
Copy link
Contributor

@iche033 iche033 commented Apr 1, 2021

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

🦟 Bug fix

Summary

For lidars with high sample count, there is apparent discretization in the data generated. The discretization effect is more evident at long distances. This is mainly due to the resolution of the textures used in the 1st pass, which were hardcoded to 1024x1024. For a lidar with high sample count, e.g. 10000, the 1024 texture size is too low. The 2nd pass uses a texture with the actual sample count (10000) but it's just sampling from this 1024x1024 texture.

This PR changes the way the 1st pass texture size is set - It is now based on the sample count and field of view of the lidar. Lidars with low sample count will now also use smaller textures to save memory. The max texture size is capped to 4K in order to run on most graphics cards.

Here is a before and after comparison of point cloud visualization in RVIZ showing the discretization effect for a 360 degrees lidar with 10000 horizontal samples.

Before: 1st pass texture resolution fixed to 1024
lidar_discretization_before

After: 1st pass texture resolution capped to 4096
lidar_discretization_after

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

Note to maintainers: Remember to use Squash-Merge

Signed-off-by: Ian Chen <[email protected]>
@github-actions github-actions bot added the 🔮 dome Ignition Dome label Apr 1, 2021
@iche033
Copy link
Contributor Author

iche033 commented Apr 6, 2021

I did some profiling to see the impact of increasing to use 4096 sized textures and found that there is some noticeable changes.

For the case of a 360 degrees lidar with 4K textures (worst case), the VRAM usage went up by around 1.5GB. Manual calculation seems to be consistent with observations:

4096:

  • depth texture: 4096 * 4096 * 4 (bytes per channel) * 3 (no. of channels per by pixel) * 6 (cube faces) = 1208 MB
  • particle texture: 2048 (half of depth texture size) * 2048 * 4 * 3 * 6 = 302 MB
  • total: ~1.5GB

1024:

  • depth texture: 1024 * 1024 * 4 * 3 * 6 = 75 MB
  • particle texture: 512 * 512 * 4 * 3 * 6 = 19 MB
  • total: < 100 MB

There is also some noticeable performance hit in the PostRender call:

  • 4096: ~0.02s
  • 1024: ~0.002s

I did one more test with a texture size limit of 2048 and the results are:

  • VRAM: ~400MB
  • PostRender: ~0.0045s

Here's the lidar visualization of using 2048 sized textures:

lidar_discretization_2048


@iche033
Copy link
Contributor Author

iche033 commented Apr 6, 2021

Based on the profiling results above, I'm leaning towards capping the texture size to 2048 (or keeping in as 1024) instead of 4096 to avoid a significant change in performance in a released version of ign-rendering

Signed-off-by: Ian Chen <[email protected]>
@iche033
Copy link
Contributor Author

iche033 commented Apr 9, 2021

After some discussion offline, we decided to keep the limit to 1024 due to the impact it has on VRAM usage. 42f7466

So the changes here would no longer reduce lidar discretization for sensors with high sample count. I think the changes are still worth getting int because it makes the 1st pass texture resolution configurable based on sample size and so reduces VRAM usage for lidars with low sample count.

@codecov
Copy link

codecov bot commented Apr 20, 2021

Codecov Report

Merging #296 (4a2de0c) into ign-rendering4 (a2c172a) will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@                Coverage Diff                 @@
##           ign-rendering4     #296      +/-   ##
==================================================
+ Coverage           53.48%   53.53%   +0.05%     
==================================================
  Files                 145      145              
  Lines               13760    13776      +16     
==================================================
+ Hits                 7359     7375      +16     
  Misses               6401     6401              
Impacted Files Coverage Δ
ogre2/src/Ogre2GpuRays.cc 94.94% <100.00%> (+0.16%) ⬆️

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 a2c172a...4a2de0c. Read the comment docs.

@iche033 iche033 merged commit 63b7e12 into ign-rendering4 Apr 24, 2021
@iche033 iche033 deleted the lidar_discretization branch April 24, 2021 00:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔮 dome Ignition Dome
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants