-
-
Notifications
You must be signed in to change notification settings - Fork 21.1k
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
Add AgX and AgX Punchy tonemapper options to Environment #87260
base: master
Are you sure you want to change the base?
Conversation
8ca7b7d
to
4217f1e
Compare
4217f1e
to
7e101f9
Compare
7e101f9
to
37ec580
Compare
That's just due to how the reflection probe is set up in the scene (one probe near the red box covers the entire scene). It's not related to this PR 🙂 |
This relies upon the implementation found in this article, which I believe is closer to Troy's version (but not 100% identical). |
Thanks for the clarification. You are right, if you compare the example of the sweeps (from Troy) with the one on the website you shared, they are similar, only the red seems more saturated than in the original implementation, and since in the code the punchy version has an increase in saturation of 1.4 I think that can end up creating unwanted clipping, and it seems to be messing with the luminance values?. |
Not sure if it's helpful but you can also look at three.js's implementation as reference: BTW the term "white point" has a very specific meaning in RGB color, it means the chromaticity of R=G=B, in a lot of colorspaces like Rec.709, sRGB, Rec.2020 etc. it's D65, for other spaces like DCI-P3, they have their own white points like the DCI "theatre" white point. So when the PR says something about white point being 16, it looks very confusing. |
Godot doesn't have color management, so it uses an arbitrary whitepoint unit. Higher values result in less blown out highlights, but will slightly darken the whole scene. There are diminishing effects to increasing the whitepoint value, so usually, a value like 10 will look pretty close to something like 20. |
My point is, "white point" is a widely accepted language for chromaticity, I.E how warm, how cold should the white be, not brightness or intensity. A white point value is usually a CIE XYZ or CIE xyY coordinate (For example, D65's CIE XYZ is [0.95047, 1, 1.08883]), not a scalar value. If Godot uses a scalar value to refer to an intensity value and then call it "white point", the terminology is confusing. As for the white clipping point, how steep the curve is also affects the rate of change in gradients. It's best to test against EXRs like Red Xmas etc. to make sure things are still smooth. |
@Calinou Greetings! Is the feature gonna be in 4.3? Would be nice to have alongside with the new Global Illumination to achieve overall proper lighting and coloring in complex scenes. |
I can't give an ETA for merging this, as this PR still needs a review from other contributors before it can be merged. |
Google's Filament uses slightly different matrices. It says that the matrices are taken from Blender's AgX implementation. Below are the differences between the implementation in this PR and the Filament (Blender) implementation using a custom software renderer as an example. There is almost no difference, but the Filament (Blender) implementation gives a slightly darker image.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I absolutely love the AgX Punchy tonemapping. I have merged this pull request in my custom build and have been using it in compatibility mode in VR with the meta quest 2.
I'm a big fan of cartoony saturated colors, and I always need to do color correction after the fact with a LUT or with the "adjustment" post processing to adjust contrast/saturation, with the AGX Punchy tonemapping I feel I don't even need further color correction, I'm getting pretty much the colors I want right away. I'll be using this in all of my projects moving forward, thank you!
All images taken with Exposure 0.7 and White 10 in Compatibility Mode with the Compatibility Mode Glow PR
Tonemap | Far view | Close up |
---|---|---|
Linear | ||
Reinhard | ||
Filmic | ||
ACES | ||
AgX | ||
AgX Punchy |
Thanks! I really enjoy how "natural" AgX looks, as I did not like how ACES compressed the dark colors. This is really noticeable when using global illumination. AgX seems a bit brighter than the others. AgX punchy is a bit too punchy for my use-case (I'm sure it looks better after tweaking the assets for a bit.)
(Car model CC-BY Link to Sketchfab ) |
It would be really nice if you guys could prioritize this for 4.3 Tonemappers affect the whole asset production pipeline, so with this we could go ham on making the assets without worrying that they'll look different later when upgrading to the release build. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a quick test, big fan of the results. AgX is doing a better job at compressing bright values compared to ACES
Exposure was adjusted to get approximately the same look (AgX needs about twice as much exposure compared to ACES)
With ACES it's always a struggle for me to not get overexposed values (notice how in AgX the pavement isn't overexposed while the shadows still retain the details):
This feature is also important for any global illumination upgrades since it will help a lot to achieve properly exposed areas (with more accurate colors in both dark and lit areas). Currently it's nearly impossible to achieve with existing modes. |
Want to comment on this. Originally Troy's original Punchy included a boosting of "CDL Saturation", on top of the original AgX that DID NOT HAVE OUTSET MATRIX!. And afterwards in later edition, we added the ouset matrix to the Base AgX, then in Troy's version, "Punchy" was then completely removed. (Note there we also designed a "rotation" included in the inset/outset matrix, reason will be stated later in this post.) I choose to add back the Punchy look, but since the boosting of "CDL Saturation" has already been replaced by the outset matrix, it's not needed anymore. So in the version I submitted to Blender, the Punchy look is a simple darkening without the boosting of "CDL Saturation". The original Punchy was a simple 1.35 power curve after AgX Base formation (which darkens the image), but due to OCIO's constrain, OCIO Looks are required to be pre-AgX-Base-Formation, so I had to move it to AgX Log pre formation, I tried to use some different curves to achieve in the final image the approximately the same middle grey and roughly the same "black level" as the post formation 1.35 power curve, though of course not going to be completely identical. If the doubling of outset + "CDL Saturation" is what you folks think looks better, then feel free, but I have to stress the importance of checking out those EXR files I linked up there, please make sure after your modifications that those gradients in those EXRs are still smooth as always! Too much post formation chroma boosting can lead to unsmoothness, as some boundary condition might be triggered.
I advise either go make your own rotation matrix by testing against many different challenging EXRs, or use the same one right there, it's for some compensation for Abney Effect etc. (note it's not a fix, it's currently impossible to fix Abney Effect, but the rotated matrix was tested by myself to at least somewhat compensate for it.) (Also note that the matrix was supposed to be used in BT.2020 formation space. I have already mentioned this in the three.js PR page I linked here earlier.) |
Blender Cycles CPU viewport clips negatives. Better view it in compositor. Just click use nodes, drag and drop exr, and shift ctrl left click. Or at least use Cycles GPU viewport. |
I replied to the top comment on that video when it was released. (The one asking for disadvantages.) This is what I said: Khronos is really designed for virtual product photography, and that is where it excels at." I do not think this is a good option for a game engine, but if someone wants to implement it, sure, go ahead. |
Shader code is here and here. Simple to implement in a canvas_item shader to try out. Creator Emmett says:
Having tried it in a project, I wholeheartedly agree that "Khronos [PBR Neutral] doesn't work well with big high exposure areas." However, by boosting the value of startCompression you can reduce or eliminate overblown highlights that come out black at the default value of 0.76. UPDATE: After more playing around, I quite like how PBR Neutral adds highlights and shadows. In this test scene, notice the extra light shaping/sculpting PBR Neutral adds to the face, bow, hands, etc. video.mp41080p60 version on youtube. |
To clarify it normal there’s no news now since we are on feature freeze and Calinou is working on a lot of other stuff rn so it fine if he does not answer, please be patient. |
No problem in this regard, best of luck for the team with the new release |
Thanks for the overview |
I have a WIP Khronos PBR Neutral implementation in https://github.com/Calinou/godot/tree/tonemap-add-pbr-neutral, but I didn't find its look satisfactory enough compared to AgX. I would rather focus on finalizing AgX first as I believe it'll satisfy more use cases. If further control is needed, it may be better to add shadows/midtones/highlights adjustments that work with any tonemapper as opposed to adding even more tonemapping methods. Note that Khronos PBR Neutral advertises itself as a tonemapping method for 3D visualizations rather than games (hence its |
I will agree that |
Correct me if I'm wrong, but it looks like the AgX approximation reintroduces the problem where bright, saturated colors drift significantly towards yellow/cyan/magenta: |
Are those screenshots from this pr? because the current implementation here doesn't look right yet. AgX shouldn't have the six colour problem. Check the implementation in blender for what it should look like. I strongly believe godot's implementation should match blenders' implementation. |
The AgX gradient is from a screenshot in a previous comments ("Eary's version", see comment). The approximation on the right is straight from the shadertoy implementation linked in the iolite-engine article - which is the same code as this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks a lot like bleaching where very bright colors desaturate towards white. It might be another thing entirely, though. |
I had some fun whipping up some more tools for comparing tonemaps. I made these scenes as an extension of @Calinou's demo project. You can download the project here if you'd like to fiddle with them yourself; I find them most useful as interactive tools. All images use Forward+ with whitepoint 6.0 unless otherwise mentioned.
|
37ec580
to
b65fa87
Compare
Rebased and tested again (with no other code changes), it works as expected. |
Took some time today and manually recalculated the inset and outset matrices. They should now be more in line with the fork of AGX that blender uses: const mat3 agx_mat = transpose(mat3(
0.544813, 0.37379614, 0.08139087,
0.14041554, 0.75414325, 0.10544122,
0.0888119, 0.17888511, 0.73230299));
const mat3 agx_mat_inv = transpose(mat3(
1.96489403, -0.85600791, -0.10888612,
-0.29930908, 1.32639189, -0.02708281,
-0.16435644, -0.2382074, 1.40256385)); In my own fork I also switched to a 7th order approximation, but that had very little impact on the final result. In my own test these matrices result in slightly better results, e.g. look at the faces when viewing https://github.com/sobotka/Testing_Imagery/blob/main/red_xmas_rec709.exr. The overall image is still a bit too bright, but setting white to 16 (to match AGX default value) and slightly reducing exposure now results in results that (at least on my monitor) are near identical to blender. |
@Ansraer Thanks 🙂 I've incorporated the suggested matrices: See OP for updated screenshots. Note that ReflectionProbe cubemap filtering has changed since then, so the rough reflections can appear to look different, but this is not due to the tonemapper. The whitepoint issue makes me wonder whether it should just be hardcoded to Comparison between the previous matrices (before) and the ones that are used now (after):
|
b65fa87
to
e8a24fa
Compare
@Ansraer Do you have an implementation that does not use an approximation that we could play with as a reference? This would help us give a second set of eyes on this choice. |
e8a24fa
to
c3647bf
Compare
As discussed in yesterday's rendering meeting, I've amended the PR to hardcode the whitepoint to This is ready to merge on my end. |
I've updated my previous comment with new screenshots from c3647bf ("Before" screenshots are from before the new matrices were integrated with whitepoint set to 16.0.)
Before the new changes were made, I didn't take screenshots of Punchy with a 16.0 whitepoint, so those aren't as good for side by side comparison. |
Been using this in a personal project - would really love to see this get merged into the engine. Finally a tonemapping that looks good! |
@Calinou can someone merge this? IMO: looks good already, can be further improved later. |
It first needs code review from rendering team like clay. |
We could 100% put this in a future PR and merge this into 4.4 |
Thanks to @begla for providing this code under MIT license and helping with the whitepoint configuration 🙂
Testing project: godotengine/godot-demo-projects#857
TODO
Preview
All images use whitepoint 6.0 unless otherwise mentioned.
Footnotes
This demo is currently not designed with Compatibility in mind, so the surface colors are incorrect. Nonetheless, the tonemappers work. ↩