habitat-sim icon indicating copy to clipboard operation
habitat-sim copied to clipboard

Sharing of rendering results

Open erikwijmans opened this issue 4 years ago • 4 comments

Motivation and Context

Habitat-sim currently has the ability to draw RGB, Depth, and Semantic observations all on the same render pass, however, this isn't currently leveraged! This PR adds the ability for sensor to "borrow" eachother's render results so that if you draw for an RGB sensor and have a Depth sensor that is configured identically, the Depth sensor won't invoke another draw but will instead just read the Depth Buffer from the RGB draw. For Replica scenes, we can share between RGB, Depth, and Semantic!

One question for reviewers: I also tried an implementation that copies between RenderTargets instead of sharing them. While this seems safer, that copy obviously has a cost and grows with resolution. We currently don't make any assumptions that would be violated by this direct sharing so maybe it is fine? If we do want this copy, my testing says that the cost of doing the copy in draw_observation is pretty high (performance with GPU2GPU drops to bellow without for 256x256 for instance). Doing the copy in get_observation is less costly.

@mosra I left the code to copy between framebuffers in the PR to see if you knew if there was a way to do it faster.

How Has This Been Tested

New tests.

I re-ran the benchmark on the normal MP3D test scene and got

 ====== FPS (512 x 512, depth_only): 5624.3 =============
 ================ Performance (FPS) NPROC=1 ========
Resolution      rgb             rgbd            depth_only
128 x 128       3151.7          1510.4          2361.9
256 x 256       2750.8          1137.1          1893.8
512 x 512       1607.2          697.9           1315.9
 ====================================================
 ================ Performance (FPS) NPROC=3 =============
Resolution      rgb             rgbd            depth_only
128 x 128       7684.1          3423.9          5935.4
256 x 256       6339.5          2885.4          4878.6
512 x 512       3817.1          1929.5          3610.2
 ======================================================
 ================ Performance (FPS) NPROC=5 ===============
Resolution      rgb             rgbd            depth_only
128 x 128       10651.7         4819.9          9146.4
256 x 256       8940.7          4056.9          7848.4
512 x 512       6674.5          2939.8          5624.3
 =====================================================

Comparing to the existing results, this maybe matters for higher resolutions, but otherwise it surprisingly doesn't. However, 17DRP5sb8fy is a reasonably lightweight scene.
1LXtFkjw3qL on the other-hand is much heavier and there it matters. Before the PR:

 ================ Performance (FPS) NPROC=1 ==========
Resolution      rgb             rgbd            depth_only
128 x 128       1227.5          557.1           1129.8
256 x 256       1128.7          501.7           918.3
512 x 512       833.3           415.0           802.9
 =================================================

After:

 ================ Performance (FPS) NPROC=1 ========
Resolution      rgb             rgbd            depth_only
128 x 128       1289.9          868.7           1073.2
256 x 256       1270.8          855.0           1044.9
512 x 512       994.9           566.6           843.0
 ===============================================

Then with GPU2GPU transfer enabled we get close to zero-cost RGB-D

 ================ Performance (FPS) NPROC=1 ======
Resolution      rgb             rgbd            depth_only
128 x 128       1319.1          909.5           1219.3
256 x 256       1426.3          924.4           1207.0
512 x 512       1364.3          959.6           1125.2
 ==========================================

Types of changes

New feature (non-breaking change which adds functionality)

erikwijmans avatar May 09 '20 19:05 erikwijmans

This looks like it will help speed up the simulator quite a bit for many of the challenge tracks. Well done.

Skylion007 avatar May 09 '20 20:05 Skylion007

Codecov Report

Merging #626 into master will increase coverage by 0.18%. The diff coverage is 97.82%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #626      +/-   ##
==========================================
+ Coverage   58.30%   58.48%   +0.18%     
==========================================
  Files         130      130              
  Lines        5905     5931      +26     
  Branches       84       84              
==========================================
+ Hits         3443     3469      +26     
  Misses       2462     2462              
Flag Coverage Δ
#CPP 54.48% <100.00%> (+0.10%) :arrow_up:
#JavaScript 10.00% <ø> (ø)
#Python 76.52% <97.43%> (+0.19%) :arrow_up:
Impacted Files Coverage Δ
src/esp/gfx/RenderTarget.h 100.00% <ø> (ø)
src/esp/sensor/Sensor.h 85.71% <ø> (ø)
src/esp/sim/Simulator.h 100.00% <ø> (ø)
habitat_sim/agent/agent.py 98.21% <90.00%> (+0.01%) :arrow_up:
habitat_sim/sensors/sensor_suite.py 100.00% <100.00%> (ø)
habitat_sim/simulator.py 96.45% <100.00%> (+0.20%) :arrow_up:
src/esp/sensor/Sensor.cpp 87.17% <100.00%> (+2.80%) :arrow_up:

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 bfc7623...27a4f54. Read the comment docs.

codecov[bot] avatar May 09 '20 23:05 codecov[bot]

@erikwijmans Any reason this still hasn't been merged? Are we blocked by something?

Skylion007 avatar Jul 08 '20 16:07 Skylion007

The speedups aren't high enough to justify the ugliness that is this solution IMO. @mosra and I had a chat when I originally made this and the result of that (after some initial resistance from me) is that a better design would be to have a VisualSensor that inherently supports rendering multiple modalities (this would also reduce the ugliness that is making an agent with RGB-D + Sem agent currently), but that is a larger change that I haven't had time to make yet.

erikwijmans avatar Jul 08 '20 18:07 erikwijmans