Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Merged by Bors] - Revamp Bloom #6677
[Merged by Bors] - Revamp Bloom #6677
Changes from 135 commits
425c7ae
1dd39be
7704e12
bb465b5
82e0f2f
fa4fccb
d7a1ae7
9a94a40
bb06589
892fb5e
eb27945
2e3f428
dabf32a
5fd4116
38df5ad
7de822b
cff860e
e58aeac
af760a7
1719571
d6b642a
3f4ecb8
4398515
d172215
5fbdc01
4b63ea3
aea919a
0e481ff
47f2e3d
d4f491c
e64900e
193932f
8972fd3
ec53b37
4daff1b
5fb4806
2ab6225
bf8e301
a574388
85af8f9
1dc0d41
774f008
2f35c92
896ee46
9f1c6cc
79bc5b9
ec3c569
9bef787
f849c4e
9298bc6
3452b32
c74f301
3d0a912
9d5c544
dc257d4
df53631
ad1f11c
3f51432
8971a88
8a0d1d1
dac7680
82a553d
4ff46bf
0214e39
1cbe98a
ebe2cff
3e8d9fa
c0a7f2f
cc57711
9377e14
12e4b10
75f59f8
a495344
38d7c45
4a2587b
c515fc5
c7faf67
f87bf01
895d626
b2dba3a
1536709
8ae71e6
7a321f3
677b758
d4b61ac
f315d78
aa2a597
a297ce9
c2955b3
09ae245
a96ad6e
870435c
a187c4b
95ad4d7
c041374
6dd011b
e1485f1
48fedbf
1c11c9d
c296a45
860effc
f99a027
6d4ee2b
e8fb506
3339655
fe5aa43
70b398e
62c885c
7b7db8a
ddfe45e
6cb8b0c
adedc79
8981638
13d131c
93d9042
86372b1
80b7ce2
b7e0951
9b1cdad
98060e8
24af9c6
26a503b
63630e8
c624b06
61391d4
129f334
7a02aae
e047f41
0e2d92f
f480488
c7d722a
c28ad59
5c40029
bcc6994
6cb4c99
e72f201
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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'm not sure what luminance mapping to use here, but this is based on Rec.709. Apparently the values in
tonemapping_luminance
are based on the Rec.709 colour primaries. The article says that if RGB need to be mapped into the range, then the max colour component should be used. I'm not sure this is correct.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'm not sure either.
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.
The implementation is faithful to the source material. Input signal is in linearized sRGB and luma is a weighted sum or sRGB components. We convert our image to sRGB and perform the luminance function (which is the same as luma but with gamma-corrected input). I don't know why this works considering we are trying to process physical light and avoid any human-based measurements like luminance and, even more so, luma, but this function seems to do the trick somehow. I am pretty positive this will lead to artifacts if the input color space changes (for example to ACES 2065-1) but at the moment it works. I think the approach the original developers took here was somewhat similar to the one described in this video (15:47 - 16:15)
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.
:)
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.
For clarity, and because I can't resolve these discussions, I consider this discussion resolved well enough for now.
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.
The article says:
I am not convinced the implementation in this PR is a valid simplification of that. For one, I don't think calculating the weight of a sum of samples will be the same as summing the weights of samples, when the formula for the weight is
1 / (1 + luma)
.(1 / a) + (1 / b) = (b / (a * b)) + (a / (a * b)) = (a + b) / (a * b)
.I would say that I think to be correct, the firefly reduction would need to be applied to each sample individually. But then I realised that each sample is a bilinear interpolation of samples around it, so it is already in some sense a weighted-average value.
So, what is the goal here? To reduce fireflies. The PR considers each group, which are groups of four samples in various configurations at least 2 pixels apart, and then tries to eliminate fireflies in the average of those four. It seems incorrect regardless.
I'm not yet sure what a correct and/or acceptable solution is.
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.
It seems I missed part of the slides that discuss this approach. It was intentional to apply the firefly reduction to the averaged groups of 4 samples, so this is OK.
We should add a reference for it though. Had it been there I'd have looked at it. :)
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.
The firefly reduction method used in this PR is a well tested one that is extensively described and, as far as I see, correctly implemented one. There is another article (Bonus material 2: Karis average) that shows code very similar to what we have
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.
Thanks for that reference. We should probably add a link to it too.
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 see now there is a link at the top of the file.
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.
Clarity: I'm happy to consider this resolved.
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 feel like the full filter weights should be applied after the luminance averaging. So each 4x4 box filter just needs the 4 samples averaging (i.e. / 4 or * 0.25), then the luminance adjustment is applied, then the 13-tap weighting is applied.
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.
Hmm, this thinks otherwise: https://learnopengl.com/Guest-Articles/2022/Phys.-Based-Bloom But maybe they did it wrong?
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.
Asking Jimenez: https://twitter.com/swainrob/status/1624767458958516225
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.
Clarity: given this matches the learnopengl source material, I'm happy to consider this resolved, pending feedback from Jimenez.