Disable all sources of ambient light when ambient_light_disabled render mode is used
#92213
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…nder mode is used
Calinou
approved these changes
Jun 25, 2024
There was a problem hiding this comment.
Tested locally with all rendering methods (rebased on top of master 25ff130), it works as expected. The rationale makes sense to me.
Testing project: test_ambient_disabled.zip
| Before | After (this PR) |
|---|---|
 |
 |
|
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes: #83593
In #66107 we agreed to reinstate the
ambient_light_disabledrender mode and use it to disable all ambient light contributions. However, the actual code only disabled diffuse ambient light and not specular ambient light. This led to it doing essentially nothing on metallic materials and running the full ambient light calculations for all materials.I noticed this issue while debugging performance in a 3D scene with @RPicster. A transparent object was doing the full VoxelGI calculation in the forward pass, even when ambient light was disabled.
This change will technically change behaviour as it will disable specular ambient light in addition to diffuse. IMO this is an acceptable change in behaviour because the previous behaviour was unexpected, bad for performance, and ultimately the result of a bug. That being said, I am tagging it for 4.4 as we are too late in the 4.3 dev cycle to introduce a change in behaviour.