This patch adds a tonemapper to the color management, the operator is based on the "Corona Highlight compression" code from https://corona-renderer.com/forum/index.php/topic,1807.0.html.
It is applied before the curve correction, as its main task is to bring values > 1 into the visible range.
I'm not exactly sure about the parameter range, but decided to set it as 1-10 since bigger compression has had no visible effect in all my tests (since the operator is v * (1 + v/(comp*comp))/(1 + v), for large values of comp it turns into the classic v/(1+v) operator).
Since it's in the color management, it gets applied to viewport rendering, the render result after compositing, all images with "view as render" enabled and LDR image exports, but not to HDR, just as it's supposed to be.
The idea comes from http://blenderartists.org/forum/showthread.php?282540-Cycles-tonemapping and @Marco G (marcog).
Details
Diff Detail
- Repository
- rB Blender
- Branch
- tmo
Event Timeline
Tested and works really well! There are couple of examples here made by @Lukas Stockner (lukasstockner97) - link
Burnt out parts of the image are brought back while mid tones and overall image is kept perfectly, (unlike raising the white point and adjusting curves for example). Front lights of the BMW are a great test case. Plus, is really easy to use, non destructive and doesn't affect older renders, everything can work as before from what i tested, in case someone doesn't need it.
The TMO operates now on pixel luminance and exposure is applied before the tonemapping.
The new code is based on http://imdoingitwrong.wordpress.com/2010/08/19/why-reinhard-desaturates-my-blacks-3/, turns out that Corona's fancy new tonemapper is just a Reinhard...
Tonemapping now operates on RGB again, parameter was renamed to "White Value" and now uses f-stops as unit
From quick first skim looks OK. I guess the exposure part that has been removed is meant to be replaced by the tonemapper? Maybe it is possible to write it in OCIO instead? Sergey will know better here.
The exposure part originally was done as the first step by OCIO, but the problem now is that OCIO is applied after tonemapping while exposure needs to be done before it, so it must be reordered.
I am totally unconvinced with this patch.
Bigger picture concern: it declares "let's go out away OCIO and start re-implementing it all hardcoded in C". And at the same time it keeps full OCIO pipeline, maybe just for extra confusion.
So first question is: why not to do it all in OCIO?
Smaller picture concerns:
- By moving exposure in the color pipeline you're just declaring all the files which ever used it as broken. There should be really awesome outcome from the change to compensate this.
- RGB curves are there mainly for compatibility reasons, so do not construct all the color pipeline around them.
- If you need to map color to "visible range" (which is incorrect wording, because it's all visible, just some display devices are incapable of representing it) for the curves, there's already black and white levels for the curves. So your patch adds an extra slider which controls the same thing just in different term.
But guess the main question here should be: which exact pipeline issue your patch is trying to solve?
Well, I'm no expert in OCIO, but it seems that it only supports a fixed set of transform types (MatrixTransform, LogTransform...) and looks that are read from a file, but no runtime-defined LUTs (I guess that's why the Curves are also done in C). So, the options are:
- Do the tonemapping in C / GLSL
- Define a fixed and precomputed set of looks (which limits flexibility)
- Add a Transform directly to OCIO (which is quite unlikely to happen)
Regarding the exposure: As far as I see, "only" files that use both curves and exposure would be broken (because Curves->Exposure->OCIO turns into Exposure->Curves->OCIO), but that could be solved by either moving curves in front of both exposure and tonemapping (which would result in exactly the same operation as before as long as tonemapping remains at zero) or adjusting the values of the curve settings when loading an older file (which is probably not a very clean solution...)
I don't really see how I construct the color pipeline around the curves, the patch doesn't modify them in any way. If anything, it would reduce their usage since they aren't needed as a tonemapping replacement anymore. I don't really see your point with the curves: First you say that they're deprecated, and immediately afterwards you suggest to use them instead...
As for the visible range, yes, it should be called displayable range.
The initial reason for the patch was to have a way to see burned out areas in Cycles viewport rendering better, that's what started the BA thread. In theory, this is also possible with curves, but this way you can just turn up a slider and have a tonemapper that is well-tested and also used in many other tools instead of having to fiddle around with curves to get a somewhat useful result. So, essentially, it doesn't make anything possible that wasn't before, but it greatly improves usability and adds a fast and intuitive way to get an image that's not totally burned out.
Well, I'm no expert in OCIO, but it seems that it only supports a fixed set of transform types (MatrixTransform, LogTransform...) and looks that are read from a file, but no runtime-defined LUTs (I guess that's why the Curves are also done in C). So, the options are:
This is wrong. First thing is: curves are done in C because it was implemented already before OCIO integration project and this part just kept unchanged for compatibility reasons. Second thing is called OCIO documentation: http://opencolorio.org/developers/api/OpenColorIO.html#look
You could extent OCIO ocnfiguration on runtime.
Regarding the exposure: As far as I see, "only" files that use both curves and exposure would be broken (because Curves->Exposure->OCIO turns into Exposure->Curves->OCIO), but that could be solved by either moving curves in front of both exposure and tonemapping (which would result in exactly the same operation as before as long as tonemapping remains at zero) or adjusting the values of the curve settings when loading an older file (which is probably not a very clean solution...)
It's bad idea to modify curve mapping on file load. It still breaks forward compatibility which i believe we could avoid.
I don't really see how I construct the color pipeline around the curves, the patch doesn't modify them in any way. If anything, it would reduce their usage since they aren't needed as a tonemapping replacement anymore. I don't really see your point with the curves: First you say that they're deprecated, and immediately afterwards you suggest to use them instead...
That's exactly the thing. You shouldn't decouple tonemapping from OCIO pipeline. And if you _really_ want to, i guess there are possible improvements to the RGB curves themselves. I.e. replace white point with tonemapping or so (as an options).
But ideally all this tonemapping of overexposed areas should be done as a Look as per OCIO specification. You just need to implement runtime extension and editing of them which is much better idea anyway because it's more powerful solution. Namely the idea of looks is that you can define mapping between shot and look in OCIO configuration, so your shot is always rendered with an appropriate color management.
Okay, I tried adding a TonemapTransform by just overloading OpenColorIO::Transform, but it's not as easy as it seems. When the Processor object is constructed, it calls BuildOps, which then uses a hardcoded ifs to translate the Transform to an Op. Adding an Op is also possible, but getting it into the Processor isn't since that would require an own entry in the BuildOps, which is part of the library. This makes it impossible to have any own Transform, only predefined Transforms can be used for Look creation or in the DisplayTransform.
A way to work around this would be to have a CustomTransform in OCIO that stores a pointer to an Op, which could then be any Op defined in Blender. However, this means either always patching OCIO before building or sending a pull request, which is probably overkill.
Any ideas around this?
Lukas, I'm not sure how OCIO works, but I found that it can already be done in master (without the patch, so it can be done without bypassing the current system).
See the setup in this image
The key here would be creating a special look at that extends the curve so that it doesn't hit the white-point until it's X value is well beyond 1. As seen in this .blend.
Well, yes, after all it's just a simple global mapping. However, things become trickier once the scene has both "regular" curve mapping and tonemapping, and also the curve mapping code is classic C code, which is just the problem with the current code. Also, the main reason for this feature is not making the impossible possible, but making it easier and more convenient.
After all, in the adaptive patch three parameters were already too unintuitive...
Sergey is 100% correct: this patch is sub-optimal as it currently exists.
OCIO can handle transfer curves via 1D LUTs. It is ridiculously basic.
As Sergey has already said, this is an entirely backwards approach.
For examples as to how to generate unique transfer curves, the OCIO repository has Python scripts that include several log transforms, a standard display referred sRGB transform, and an sRGB curve that uses approximately four stops over and maps it down to display referred sRGB range.
For the Reinhard obsessed, why not just craft a 1D LUT and let the folks who insist on LDR representations (because doing it on anything other than a view transform is ridiculous) simply use it?
Scale whitepoint to whatever high value is set for the Reinhard LUT, and done.