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

Image-based lighting #7172

Merged
merged 103 commits into from Dec 19, 2018

Conversation

Projects
None yet
7 participants
@bagnell
Copy link
Member

bagnell commented Oct 19, 2018

Adds the ability to add image-based lighting to a model.

  • Model.sphericalHarmonicCoefficients is a property that is an array of 9 Cartesian3 coefficients used to evaluate the diffuse irradiance.
  • Model.specularEnvironmentMaps is a property that is a url to a KTX file that contains a cubmap of the specular contribution plus the specular convolution mipmap levels.

TODO:

  • Tests
  • Update CHANGES.md

NOTE: This is a PR into the hdr branch.

bagnell and others added some commits Sep 25, 2018

Merge pull request #7107 from OmarShehata/ibl
Add octahedral projection
Merge pull request #7117 from OmarShehata/ibl
Fix artifacts with Octahedral Projection
Merge pull request #7122 from OmarShehata/ibl
Fix Octahedral sampling artifacts
Merge pull request #7126 from OmarShehata/ibl
Add Trilinear Filtering to Octahedral Maps
@bagnell

This comment has been minimized.

Copy link
Member

bagnell commented Nov 19, 2018

@lilleyse This is ready for another look. The two things I haven't fixed are a different model for the Sandcastle example and the comment I made here: #7172 (comment)

bagnell and others added some commits Nov 19, 2018

@lilleyse

This comment has been minimized.

Copy link
Contributor

lilleyse commented Nov 20, 2018

Procedural IBL seems to be a lot brighter in this branch compared to master. Maybe tone down the default luminance?

dark

bright

Also the environment map seems a lot darker than it did before. Can that also be adjusted?

dark-env

@lilleyse

This comment has been minimized.

Copy link
Contributor

lilleyse commented Nov 20, 2018

It might be good to look at the NYC building while tuning the default lighting. Right now they look washed out.

washed-out

I would say master is too dark. We should aim for something in the middle that has a stronger contrast between the dark and light areas.

nyc-master

@lilleyse

This comment has been minimized.

Copy link
Contributor

lilleyse commented Nov 20, 2018

localhost example

The lighting orientation looks wrong in the NYC tileset compared to a model. After talking offline @bagnell thinks this is because the tiles don't have a model matrix and aren't sampling the environment map/spherical harmonics in the right direction.

ibl-dir
ibl-dir-good

@lilleyse

This comment has been minimized.

Copy link
Contributor

lilleyse commented Nov 20, 2018

In this demo with two models added I see TextureCache.addTexture getting called twice. In a separate demo with 3D Tiles I only see it get called once. Maybe there is some edge case with how the demo adds models.

Demo.

@lilleyse

This comment has been minimized.

Copy link
Contributor

lilleyse commented Nov 20, 2018

Textures will get cleaned up from the cache every 120 frames. Should this be longer?

I think this is fine. The texture cache code looks good.

@bagnell

This comment has been minimized.

Copy link
Member

bagnell commented Nov 29, 2018

Also the environment map seems a lot darker than it did before. Can that also be adjusted?

This is what it looked like before:
image

The reason is because the prototype wasn't using the BRDF LUT. You can see by commenting out this line:
https://github.com/AnalyticalGraphicsInc/cesium/pull/7172/files#diff-6f6a773d137da52954260087ddc76a95R747

@bagnell

This comment has been minimized.

Copy link
Member

bagnell commented Dec 1, 2018

@lilleyse This is ready for another look. The only thing I didn't change was the model in the IBL example. I could barely see any reflection from the environment map on that helicopter model.

@lilleyse

This comment has been minimized.

Copy link
Contributor

lilleyse commented Dec 7, 2018

@bagnell Try Pawns.glb.txt which cuts down on the sample model file size.

@bagnell

This comment has been minimized.

Copy link
Member

bagnell commented Dec 18, 2018

@lilleyse This is ready for another review. If you think this is ready, I can update the Sandcastle example image and update CHANGES.

@lilleyse

This comment has been minimized.

Copy link
Contributor

lilleyse commented Dec 19, 2018

Nothing else from me. 👍

@bagnell

This comment has been minimized.

Copy link
Member

bagnell commented Dec 19, 2018

@lilleyse The Sandcastle image and CHANGES have been updated. This should be ready to go.

@lilleyse lilleyse merged commit 700bbb8 into master Dec 19, 2018

7 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
deployment Deployed
Details
npm package Deployed
Details
zip file Deployed
Details

@lilleyse lilleyse deleted the ibl branch Dec 19, 2018

@TimvanScherpenzeel

This comment has been minimized.

Copy link

TimvanScherpenzeel commented Jan 3, 2019

Hi,

I was looking over the code (great work! 👍), as I'm looking to implement something similar, and was wondering about the spherical harmonics reconstruction in: https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Shaders/Builtin/Functions/sphericalHarmonics.glsl

From what I understand you are using the spherical harmonics generated with cmgen. There are a couple of things to be aware of when doing so because the developers of Filament have tried to pre-compute as much as possible in order to make reconstruction in the shader as cheap as possible.

This means that reconstruction in the shader should be as simple as:

screen shot 2019-01-03 at 12 09 37

As implemented here in the Filament engine:

https://github.com/google/filament/blob/f676d3d3ea55539774c7eefd894ff156c3f9cd86/shaders/src/light_indirect.fs#L127-L147

There is mention about Fd_Lambert() being baked into the SH, not really sure what that is about but could possibly lead to some brightness issues if not taken into account.

https://github.com/google/filament/blob/2c4ecebf11bdcd0099d869197d9c6ee1ff7bcfbf/shaders/src/light_indirect.fs#L442

Another thing I noticed is that cmgen automatically mirrors your cubemap (you can prevent this by passing the --no-mirror flag).

One last thing you might not have been aware of is that in the metadata of the outputted .ktx file the spherical harmonics are embedded meaning you could possibly save some extra manual steps.

screen shot 2019-01-03 at 12 27 16

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment