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

HDR Rendering #7017

Merged
merged 107 commits into from
Nov 14, 2018
Merged

HDR Rendering #7017

merged 107 commits into from
Nov 14, 2018

Conversation

bagnell
Copy link
Contributor

@bagnell 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.

@lilleyse
Copy link
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
Copy link
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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
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
Copy link
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.

@lilleyse
Copy link
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
Copy link
Contributor Author

bagnell commented Nov 8, 2018

@lilleyse Updated.

@lilleyse
Copy link
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
Copy link
Contributor Author

bagnell commented Nov 12, 2018

@lilleyse Yes, thanks. Updated.

@lilleyse
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Contributor Author

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?

@lilleyse
Copy link
Contributor

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

@lilleyse lilleyse merged commit 63fbb70 into master Nov 14, 2018
@lilleyse
Copy link
Contributor

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants