nglviewer / ngl

WebGL protein viewer
http://nglviewer.org/ngl/
MIT License
661 stars 167 forks source link

Color management in NGL #775

Open garyo opened 4 years ago

garyo commented 4 years ago

Recent versions of three.js (since r112) have enabled a proper linear color workflow. I'm working on implementing that; I have most of the core color management working using the recommendations in Don McCurdy's Color Management in three.js doc.

My question for other users is, should I just turn it on always? The results should be quite similar, though definitely not identical. Colors overall should be more accurate, especially where they are blended (in surface representations, for example). It's simpler to implement without having to maintain the old path.

I can enable it via a switch in the viewer, but passing that state around everywhere is nontrivial -- if users would be satisfied with just switching over, that would be perfect for me. Opinions or thoughts?

fredludlow commented 4 years ago

It's not something I have any expertise in, but if it's an improvement and doesn't break things then it sounds good to me! Do you have a demo somewhere?

garyo commented 3 years ago

This took me ages to get to, but here's a demo of color-managed NGL viewer: http://oberbrunner.com/ngl/examples/webapp.html The main difference you should see is that the current method darkens the darks excessively, and adding proper color management keeps them from being quite so crushed. You can see it clearly if you load a molecule like 2c8n and turn the metallic almost all the way up in the ball+stick ligand view. With the existing version, it's pretty much black except for the brightest highlights. With the color-managed view you can see the sticks and the balls. As another test, try 1d3e (I like to set vws surface and scale factor of 0.7); the existing version is super dark around the edges of the atoms. Some people will like the excessive contrast of the current version, to be sure, and by adjusting the gamma you can get back to that look. In any case my color management code can be enabled/disabled in the API.

Opacity is where you see it most clearly. Here's 4rik with ball+stick and a 50% opacity default surface, without color management: image

and the same, with color management: image

Here's 1d3e with a surface (vws, scale=0.7) with no color management: image

and with color management: image

Ball+stick, no color management, metalness 0.9 roughness ~0.3: image

Ball+stick, same params, with color management: image

The basic principle is that while color pickers on the web are sRGB, internally WebGL should always be run as scene-referenced linear, otherwise compositing doesn't work correctly (comes out too dark). Only at the end, when showing the image on the screen, do we apply the sRGB transfer curve.

garyo commented 3 years ago

My changes are on my feature/color-management branch. Diffs here: https://github.com/nglviewer/ngl/compare/master...garyo:feature/color-management

arose commented 3 years ago

This is very nice @garyo. I tried this for Mol* as well and is very nice indeed, especially for the metallic material. It might be good to adjust the lightIntensity and ambientIntensity global parameters a bit.

Regarding the implementation. One option is to do the sRGB to linear conversion in the shader. It seems one looses a bit of precision when transmitting the colors as uint8 (see https://blog.demofox.org/2018/03/10/dont-convert-srgb-u8-to-linear-u8/).

garyo commented 3 years ago

Yes, you're right about some precision loss in the darks. It doesn't seem bad to me, especially since we're just converting & storing the colormaker values, not entire textures. Also for my purposes, I'm exporting the three.js scenes, and if this were implemented in the shader, the scene data (vertex colors etc.) would still be sRGB, which isn't correct for glTF at least. Ideally we should be using float textures and colors everywhere, but that's another question, or at least a separate PR. I'm hoping the current implementation is seen as enough better than the current status to be worth including. I'll submit a PR shortly and we can discuss there.

panda-byte commented 1 week ago

I think is resolved by https://github.com/nglviewer/ngl/pull/808.