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

HDR Rendering #7017

Merged
merged 107 commits into from Nov 14, 2018

Conversation

Projects
None yet
6 participants
@bagnell
Member

bagnell commented Sep 6, 2018

Adds the ability to render with high dynamic range.

  • Scene.highDynamicRange will enable disable HDR rendering when supported. Defaults to true
  • Scene.gamma will change the gamma correction. Defaults to 2.2.

Auto-exposure was added but is hard-coded to be disabled. Most built-in primitives still render LDR values so the average luminance of the scene is normally < 1.0 which would make everything much brighter.

The sun, sky atmosphere, ground atmosphere, and potentially specular highlights from PBR models use the higher range.

Requires #6877 and #6943 to be merged first.

TODO:

  • Fix issue when toggling HDR (There is no issue when set before the first render).
  • Fix tests.
  • Add tests.
  • Update CHANGES.md

Future:

  • Update all primitives to output HDR values.
  • Enable auto-exposure
  • Update auto exposure to use linear sampling
  • Add different bloom algorithm if floating-point texture linear interpolation is supported.
  • Expose different tone mapping algorithms.
  • Re-add lens flare not just when in space.

bagnell and others added some commits May 19, 2018

Correct gamma correction for glTF 2.0 models, billboards, and materia…
…ls given by components. Hack for glTF 1.0 models.
@lilleyse

This comment has been minimized.

Contributor

lilleyse commented Oct 31, 2018

Thanks the recent updates look good. Hopefully we've caught all of the areas that need gamma correction. I'll merge soon after the release.

bagnell added some commits Nov 2, 2018

@lilleyse

This comment has been minimized.

Contributor

lilleyse commented Nov 2, 2018

Sorry to hold this up with more side by sides, but I think it's worth improving how ground atmosphere looks before we merge.

hdr-side-by-side

@lilleyse

This comment has been minimized.

Contributor

lilleyse commented Nov 2, 2018

Billboards seems to disappear when I toggle hdr off in the KML example. Is toggling hdr on and off sufficiently tested?

Local example

@bagnell

This comment has been minimized.

Member

bagnell commented Nov 5, 2018

Billboards seems to disappear when I toggle hdr off in the KML example.

For the KML example, Sandcastle.reset is removing the data source. If you comment out that line and toggle HDR, it'll be fine.

Is toggling hdr on and off sufficiently tested?

I haven't tested it exhaustively for all of the primitives, but I shouldn't need to since all it does is switch the derived command executed. If it works for one, it should work for all of them.

@bagnell

This comment has been minimized.

Member

bagnell commented Nov 5, 2018

Sorry to hold this up with more side by sides, but I think it's worth improving how ground atmosphere looks before we merge.

There's not much I can do about that. It's the result of tonemapping. I like the way it looks since you can get views like at the bottom of this post: https://cesium.com/blog/2018/08/23/siggraph-2018-trip-report/

If you think it should change, perhaps @ggetz could take a look.

@lilleyse

This comment has been minimized.

Contributor

lilleyse commented Nov 7, 2018

I added a bit of saturation and modified the darkness falloff of the globe. HDR vs. non-HDR still have a different "feel" but I'm happy enough with the way it looks now.

hdr

@lilleyse

This comment has been minimized.

Contributor

lilleyse commented Nov 7, 2018

For the KML example, Sandcastle.reset is removing the data source. If you comment out that line and toggle HDR, it'll be fine.

Ahh that's my bad.

bagnell and others added some commits Nov 7, 2018

@lilleyse

This comment has been minimized.

Contributor

lilleyse commented Nov 8, 2018

Did a final sweep and noticed just a couple issues:

The power plant tileset was updated to be glTF2 + KHR_techniques_webgl and doesn't seem to be gamma corrected properly anymore.
powerplant

I'm seeing a crash when loading any demo in IE.
ie-crash

Firefox, Edge, and Chrome all work fine. I haven't tried Safari.

@bagnell

This comment has been minimized.

Member

bagnell commented Nov 8, 2018

@lilleyse Updated.

@lilleyse

This comment has been minimized.

Contributor

lilleyse commented Nov 8, 2018

I think it would be better to apply the gamma correction in the Model layer since 4c33a68 will only work for 3D Tiles. In the same way that we detect a source model is gltf 1.0 here can we detect if the source model uses KHR_techniques_webgl?

@bagnell

This comment has been minimized.

Member

bagnell commented Nov 12, 2018

@lilleyse Yes, thanks. Updated.

@lilleyse

This comment has been minimized.

Contributor

lilleyse commented Nov 12, 2018

@bagnell this breaks glTF 2.0 PBR models because even they use KHR_techniques_webgl after PBR shaders are generated. The model will need to store whether it originally used the extension, like what sourceVersion does.

@pjcozzi

This comment has been minimized.

Member

pjcozzi commented Nov 13, 2018

@bagnell as we discussed offline, please test performance - given the increased memory bandwidth - and suggest if this should default to true in general and for mobile in particular - or if we should default to false and/or encourage mobile to turn it off.

@bagnell

This comment has been minimized.

Member

bagnell commented Nov 13, 2018

@pjcozzi There are two fullscreen framebuffers that increased in size. If the half-float extension is available, they'll be 2x as big. If not and float textures are supported, they'll be 4x as big. I didn't see a performance difference on a Pixel. On the iPad and iPhone, HDR wasn't enabled by default.

@lilleyse Updated. Is there anything else?

bagnell added some commits Nov 14, 2018

@lilleyse

This comment has been minimized.

Contributor

lilleyse commented Nov 14, 2018

Thanks @bagnell, that model issue was the last thing.

@lilleyse lilleyse merged commit 63fbb70 into master Nov 14, 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

This comment has been minimized.

Contributor

lilleyse commented Nov 14, 2018

Note for everyone: if you see any colors that look more faded than before make sure to open an issue. This usually means something is missing gamma correction. See some of the side-by-sides for examples of what this would look like.

We went through all the demos pretty carefully but it's possible there are areas we missed.

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