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

Enable phong shading and lighting configurations #489

Merged
merged 11 commits into from
Feb 19, 2020

Conversation

matthewjmay
Copy link
Contributor

Motivation and Context

This PR enables phong shading by default for added objects (scene remains flat shaded), and sets up the framework for multiple light configurations to be used, another step toward #319.

Without Phong (before)

no_phong_shading

With Phong

with_phong

The default light configuration for dynamically added objects consists of lights at the corners of the scene bounding box.

In later PRs, the following functionality will be exposed through the simulator

  • selecting light configurations to use for added/existing objects
  • updating existing light configurations

How Has This Been Tested

  • End to end test in SimTest.GetPinholeCameraRGBAObservation shows no changes to images rendered with no lighting. Once lighting selection is exposed through simulator, I will add tests for different phong lighting configurations
  • Visual test using viewer shows phong lighting works!

Benchmarks

Scene Only

Tested locally with:
python examples/benchmark.py --scene ~/habitat-sim/data/scene_datasets/habitat-test-scenes/skokloster-castle.glb --max_frames 4000 --resolution=256 --num_procs=1

On branch master:

 ================ Performance (FPS) NPROC=1 ===================================
Resolution      rgb             rgbd            depth_only
256 x 256       438.2           249.0           462.8
 ==============================================================================

On this branch:

 ================ Performance (FPS) NPROC=1 ===================================
Resolution      rgb             rgbd            depth_only
256 x 256       442.8           225.7           425.5
 ==============================================================================

Should have no impact since scene is rendered with no lights

With Phong Shaded Physics objects

Tested locally with:
python examples/benchmark.py --enable_physics --scene ~/habitat-sim/data/scene_datasets/habitat-test-scenes/skokloster-castle.glb --max_frames 4000 --resolution=256 --num_procs=1

On branch master:

 ================ Performance (FPS) NPROC=1 ===================================
Resolution      rgb             rgbd            depth_only      phys_rgb        phys_rgbd
256 x 256       484.9           232.9           454.0           122.0           116.7
 ==============================================================================
 ================ Performance (step time: milliseconds) NPROC=1 ===================================
Resolution      rgb             rgbd            depth_only      phys_rgb        phys_rgbd
256 x 256       0.00            0.00            0.00            0.57            0.50
 ==============================================================================

On this branch:

 ====== FPS (256 x 256, phys_rgbd): 119.0 ======
 ================ Performance (FPS) NPROC=1 ===================================
Resolution      rgb             rgbd            depth_only      phys_rgb        phys_rgbd
256 x 256       441.8           231.4           452.3           106.8           119.0
 ==============================================================================
 ================ Performance (step time: milliseconds) NPROC=1 ===================================
Resolution      rgb             rgbd            depth_only      phys_rgb        phys_rgbd
256 x 256       0.00            0.00            0.00            0.82            0.49
 ==============================================================================

Negligible impact.

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have completed my CLA (see CONTRIBUTING)
  • I have added tests to cover my changes.
  • All new and existing tests passed.

update resource manager to use new ShaderManager

flat shading override for scenes
@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Feb 12, 2020
@dhruvbatra
Copy link
Contributor

Great job on the benchmarks.

@codecov
Copy link

codecov bot commented Feb 12, 2020

Codecov Report

Merging #489 into master will increase coverage by 0.05%.
The diff coverage is 61.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #489      +/-   ##
==========================================
+ Coverage   59.77%   59.83%   +0.05%     
==========================================
  Files         158      158              
  Lines        7076     7096      +20     
  Branches       84       84              
==========================================
+ Hits         4230     4246      +16     
- Misses       2846     2850       +4
Flag Coverage Δ
#CPP 55.35% <83.33%> (+2.51%) ⬆️
#JavaScript 10% <ø> (-90%) ⬇️
#Python 79.49% <43.33%> (+0.11%) ⬆️
Impacted Files Coverage Δ
src/esp/sim/Simulator.h 100% <ø> (ø) ⬆️
tests/test_sensors.py 100% <ø> (+11.36%) ⬆️
src/esp/geo/geo.cpp 34.28% <ø> (ø) ⬆️
src/esp/gfx/Renderer.h 100% <ø> (ø) ⬆️
src/tests/Mp3dTest.cpp 15.38% <0%> (-0.62%) ⬇️
src/esp/scene/Mp3dSemanticScene.cpp 0.84% <0%> (-0.01%) ⬇️
src/tests/SimTest.cpp 100% <100%> (ø) ⬆️
examples/example.py 76.38% <100%> (+0.67%) ⬆️
habitat_sim/simulator.py 94.35% <100%> (+0.02%) ⬆️
examples/settings.py 100% <100%> (ø) ⬆️
... and 13 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 dcb61c2...7b2e668. Read the comment docs.

src/esp/assets/GltfMeshData.cpp Outdated Show resolved Hide resolved
src/esp/assets/ResourceManager.h Show resolved Hide resolved
src/esp/gfx/GenericDrawable.cpp Outdated Show resolved Hide resolved
src/esp/gfx/LightSetup.cpp Outdated Show resolved Hide resolved
src/esp/assets/ResourceManager.cpp Outdated Show resolved Hide resolved
src/esp/geo/geo.cpp Outdated Show resolved Hide resolved
src/esp/gfx/LightSetup.cpp Outdated Show resolved Hide resolved
@erikwijmans
Copy link
Contributor

@matthewjmay Looks like we have functionality in habitat-api that gets broken by a singleton also. We have a threaded vector env (instead of the normal subprocess vector env) that is nice for debugging. This is fine with OpenGL contexts as those are local to a thread, but a singleton will get shared across threads. @mathfac do you know if its fine to limit the threaded vector env to just 1 sub-thread for a little bit? AFAIK, that functionality is only really useful for debugging where 1 process would likely be sufficient?

@mathfac
Copy link
Contributor

mathfac commented Feb 13, 2020

@erikwijmans, great suggestion about making num_ens=1. Yes, Threaded Vec envs are useful for sure and we want to keep it, @mosra what is ETA to remove the singleton constrain and update us to new version of Magnum?

@mosra
Copy link
Collaborator

mosra commented Feb 14, 2020

what is ETA to remove the singleton constrain and update us to new version of Magnum?

ETA is now -- see #496 :) I came to a conclusion that removing the deprecated functionality is the lesser evil, even though it would break things for existing users who didn't have a chance to upgrade yet.

Copy link
Contributor

@aclegg3 aclegg3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, let's land this. 👍

@matthewjmay matthewjmay merged commit f2845b5 into master Feb 19, 2020
@matthewjmay matthewjmay deleted the simpler-shader-manager branch March 17, 2020 23:00
Ram81 pushed a commit to Ram81/habitat-web-sim that referenced this pull request Dec 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants