--[PBR Update 2 of 3] PBR rewrite-motivated default lighting and IBL fix/reworking #2119

jturner65 · 2023-05-29T15:25:55Z

Motivation and Context

As part of the ongoing PBR shader rewrite/functional expansion effort, this PR contains changes that were made to the default lighting configurations, as well as providing a few fixes. Specifically :

A heuristically-derived constant intensity scaling factor was added to the PBR shader code, instead of having a large scaling/intensity factor hardcoded into the default lighting values themselves. This way the lights will work acceptably for both phong and PBR shaded objects. NOTE : This hardcoded value is obviated and removed in the PBR fragment shader PR.
The default lights themselves were tweaked to give reasonable performance, both intensity and visuals, for both PBR and Phong shaded scenes. NOTE : PBR performance is in reference to the new shader functionality, not the existing shader, although the existing shader also looks better.
The IBL implementation was fixed so that it will work with the python viewer. This required a rework of how it was instantiated (i.e. taking it out of the ResourceManager constructor).
Shader direct/indirect diffuse & specular lighting balance in PBR was adjusted so that IBL lighting is not overpowered/washed out. NOTE : Although this balancing was made with the new PBR shader in mind, the existing PBR output looks better as well.
New test images were added to reflect the new lighting. NOTE : These new test images will need to be replaced again when the new PBR shader work is merged.

NOTE : Many of these changes were driven by the new PBR shader, and so may not appear ideal with the current shader.

How Has This Been Tested

Existing c++ and python tests pass, with the addition of new test images to account for lighting changes.

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.

jturner65 · 2023-05-29T15:32:21Z

One thing to note : any existing dataset that has lighting configs defined and is intended for PBR shading will probably need those lights adjusted. AFAIK ReplicaCAD is the only dataset currently that would need this rebalancing, but I'm not sure. @aclegg3 ?

Edit : I've renormalized the lighting configs in ReplicaCAD Interactive to account for the changes in this PR and have made a new version of the dataset (1.6 I guess) with these changes. Just need to upload it to wherever it goes.

0mdc · 2023-05-29T15:56:47Z

src/esp/gfx/LightSetup.cpp

+       LightPositionModel::Camera},  // forward
+      {{0.0, -0.3, 1.0, 0.0},
+       {0.5, 0.5, 0.5},
+       LightPositionModel::Camera},  // backward (for glancing speculars)


I would avoid emitting light from the camera by default here.

This is done specifically so that surface details such as normal or clearcoat texture/specular can be readily seen in the new PBR shader regardless of camera orientation (i.e. demo screen captures).

Like I said below, I can remove these in favor of a custom lighting config added to /data that folks could just load up to take screen shots and such.

0mdc · 2023-05-29T15:58:30Z

src/esp/assets/ResourceManager.h

+   * TODO: this will be replaced by config-driven IBL asset loading and
+   * registration
+   */
+  void initPbrImageBasedLighting(const std::string& hdriImageFilename);


What is the current workflow to generate these cubemaps?

Same as this, only it is instantiated from ResourceManager constructor. This will fail if there's no context existing, which is what happens when enabled from python.

src/esp/sim/Simulator.cpp

mosra · 2023-05-29T17:19:11Z

src/esp/gfx/LightSetup.cpp

+       {0.5, 0.5, 0.5},
+       LightPositionModel::Camera},  // backward (for glancing speculars)
+
+  };


Just so everyone is aware -- this is 7 lights instead of 5, which makes the fragment shader roughly as much as 40% more computationally expensive.

Personally I don't think that many lights is needed, even a three-point lighting was often enough to get a balanced output.

I was aware of the potential impact of these two additional camera-related lights, but their intention is specifically to give easily visualized specular/textural response for those materials that have texture-based surface normals, clearcoat texture and cc normal texture and anisotropy (so far).

If folks think this is too expensive, I can remove the two camera facing lights, and instead provide a lighting config that would define the lights in this manner that folks making demos could consume.

It is also pretty easy to just author a new light config with fewer lights if the default is considered too computationally expensive.

But you know how it usually goes -- people will treat the default as the "normal perf", completely unaware that it could get faster with some tweaks.

Same as they treated the extraordinary light setup that was here before as "this is how magnum renders things".

I think that one directional light and an ambient light would suffice as a default.

A single directional light will not look good at all for PBR scenes (I've tried it), and ambient light is neither used nor useful in PBR, since it lacks a light vector. Of course it probably will be fine with Phong lighting, so perhaps we should set up a different default for Phong?

~~At least for PBR, I would suggest the 5 base global directional lights I have in the current default setup (minus the camera-relative ones).~~

I pushed up a 4-light setup that seems to work pretty well for Phong and PBR (both old/current and new).

src/esp/sim/ClassicReplayRenderer.cpp

src_python/habitat_sim/utils/settings.py

jturner65 · 2023-05-31T14:26:55Z

@0mdc @aclegg3 Ok, I've modified the default lights - now there are only 4, and they seem to give reasonable illumination in both Phong and PBR scenes (using the old PBR shader). See images

Phong shader :

Current (before refactor/rewrite) PBR shader :

jturner65 · 2023-05-31T14:38:28Z

Here are these default lights using the new PBR shader - very similar (some of the surfaces are a bit darker due to clearcoat effect).

Default lights should work well for both PBR and Phong shading. By moving the scaling value to the PBR shader, the lights can now be agnostic to shader.

The PBR 2.0 shader refactor will remove the hardcoded scaling by a change of units conversion.

By having the main lights global, constructive interference (i.e. the lights getting brighter because of overlap) is avoided when turning the camera. The camera-relative backwards light makes sure there's always a glancing specular when looking at horizontal clearcoat/specular/metallic object surfaces. Also, this has a better mix of direct and indirect lighting when IBL is active

Having the IBL env map loaded in the ResourceManager constructor was failing since there usually wasn't a GL context built yet, which was needed to instantiate the shader. This is just a stop-gap measure so that IBL can be invoked successfully. Much engineering needs to be done for IBL to be fully ready for prime time.

NOTE : Some of these images will need to be updated again with the full PBR fragment shader rewrite.

aclegg3

Looks good. We'll merge once ReplicaCAD update is ready.

facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label May 29, 2023

jturner65 requested review from 0mdc, Skylion007, aclegg3 and eundersander May 29, 2023 15:26

0mdc reviewed May 29, 2023

View reviewed changes

mosra reviewed May 29, 2023

View reviewed changes

src/esp/sim/Simulator.cpp Show resolved Hide resolved

mosra reviewed May 29, 2023

View reviewed changes

Skylion007 reviewed May 29, 2023

View reviewed changes

src/esp/sim/ClassicReplayRenderer.cpp Outdated Show resolved Hide resolved

Skylion007 reviewed May 29, 2023

View reviewed changes

src_python/habitat_sim/utils/settings.py Show resolved Hide resolved

jturner65 force-pushed the pbr_lighting_update branch 3 times, most recently from 9d58dfe to 5486123 Compare May 31, 2023 14:05

jturner65 added 14 commits May 31, 2023 12:46

--scale lights only in PBR shader;

c2ba4ff

Default lights should work well for both PBR and Phong shading. By moving the scaling value to the PBR shader, the lights can now be agnostic to shader.

--better light balance

c746d5d

The PBR 2.0 shader refactor will remove the hardcoded scaling by a change of units conversion.

--rebalance IBL mix

5458f92

--make default lights camera-relative (temporarily)

2027eab

--modify replay test to account for added default light.

e9d6776

--change case of ibl flag

f4b778a

--enable viewer.py ibl flag

cdaa467

--fix simConfig comparator to cover IBL flag

0a12af3

--minor default light adjustment

d324b5c

--updated test images for new lighting

6bab5db

NOTE : Some of these images will need to be updated again with the full PBR fragment shader rewrite.

--reviewer suggestion

fa320ab

--add hdr image;

aefa1c6

jturner65 added 2 commits May 31, 2023 12:47

--removed camera-relative lights from default light setup;

47d45fb

--tweaked and removed unnecessary downward light

16a3900

jturner65 force-pushed the pbr_lighting_update branch from 9cfc2aa to 16a3900 Compare May 31, 2023 16:47

aclegg3 approved these changes Jun 1, 2023

View reviewed changes

aclegg3 mentioned this pull request Jun 1, 2023

ReplicaCAD v1.6 #1783

Merged

11 tasks

jturner65 merged commit d32d751 into main Jun 1, 2023

jturner65 deleted the pbr_lighting_update branch June 1, 2023 21:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

--[PBR Update 2 of 3] PBR rewrite-motivated default lighting and IBL fix/reworking #2119

--[PBR Update 2 of 3] PBR rewrite-motivated default lighting and IBL fix/reworking #2119

jturner65 commented May 29, 2023 •

edited

Loading

jturner65 commented May 29, 2023 •

edited

Loading

0mdc May 29, 2023

jturner65 May 29, 2023

jturner65 May 29, 2023

0mdc May 29, 2023

jturner65 May 29, 2023 •

edited

Loading

mosra May 29, 2023

jturner65 May 29, 2023

jturner65 May 29, 2023

mosra May 29, 2023

0mdc May 31, 2023

jturner65 May 31, 2023 •

edited

Loading

jturner65 commented May 31, 2023 •

edited

Loading

jturner65 commented May 31, 2023

aclegg3 left a comment

--[PBR Update 2 of 3] PBR rewrite-motivated default lighting and IBL fix/reworking #2119

--[PBR Update 2 of 3] PBR rewrite-motivated default lighting and IBL fix/reworking #2119

Conversation

jturner65 commented May 29, 2023 • edited Loading

Motivation and Context

How Has This Been Tested

Types of changes

Checklist

jturner65 commented May 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jturner65 May 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jturner65 May 31, 2023 • edited Loading

Choose a reason for hiding this comment

jturner65 commented May 31, 2023 • edited Loading

jturner65 commented May 31, 2023

aclegg3 left a comment

Choose a reason for hiding this comment

jturner65 commented May 29, 2023 •

edited

Loading

jturner65 commented May 29, 2023 •

edited

Loading

jturner65 May 29, 2023 •

edited

Loading

jturner65 May 31, 2023 •

edited

Loading

jturner65 commented May 31, 2023 •

edited

Loading