Skip to content
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

Allow black metallic materials to reflect IBL #69522

Merged
merged 1 commit into from
Dec 3, 2022

Conversation

clayjohn
Copy link
Member

@clayjohn clayjohn commented Dec 3, 2022

Fixes the other half of #69476

@fracteed and I have been discussing this change at length today. This PR restores a behaviour that was present in Godot 3.x and was only removed this year in #63587 (in response to #40753 and godotengine/godot-proposals#4818).

Background

In Godot 3.x there was no way to disable specular reflections. Many users requested the ability, often pointing out that in other engines specular reflections can be disabled in dieletric materials by setting specular to 0.0 (in Godot this only adjusted the specular lobe of lights). In response, I implemented #63587 which relies on a hack that Filament calls "Pre-baked Specular Occlusion". The idea is basically that no materially can have a specular value of less than 0.02, so we can treat a specular value of less than 0.02 as a signal to shut off specular reflections entirely. In other words, we can treat a specular of less than 0.02 as a form of baked occlusion. This hack is used in Filament, Unreal Engine, and Frostbite to my knowledge.

Trouble arises when we consider metallic materials. For metals, their specular value comes directly from their albedo, the specular property isn't used. Therefore, metals with an albedo of (0,0,0) were subject to this change as well. In effect any metal with an albedo of (0,0,0) became a perfectly black absorbing material.

This created two inconsistencies:

  1. Metals aren't supposed to absorb light, so it is odd that they would be absorbing light in this case but only with an albedo of exactly (0,0,0)
  2. Dielectrics which do absorb light still have a reflection when their albedo is (0,0,0), they only totally lose their reflection when specular is also 0.0

Further, as pointed out in #69476 it can be desirable to have a metal material with an albedo of (0,0,0) still have some fresnel reflectance, but this was made impossible by #63587

Finally, the reason I have had difficulty with this is that we have two knowledgeable users at odds over what the behaviour should be.

In godotengine/godot-proposals#4818 @mindinsomnia specifically pointed out that albedo metals should have no reflection in order to match the behaviour in Unreal Engine, unity, and Eevee (note, it only actually matches the behaviour in Unreal Engine).

In #69476 @fracteed has requested that the behaviour be changed back (only for black albedo metals) in order to be consistent with Eevee, Substance, and Unity.

Part of what makes this tricky is that Eevee used to disable reflections for black albedo metals. But in newer versions has changed to the behaviour in this PR where specular reflections are partially enabled.

Underpinning all this discussion is the fact that metals with an albedo of less than 0.66 don't exist in nature, so there is no solid PBR theory underpinning the difference between the two behaviours (which is why I consistently refer to this and #63587 as hacks)

Implementation

The specular occlusion trick works by expanding the 0.02 range of the specular parameter to 0-1 (essentially a multiplication by 50) then clamping to the 0-1 range. We exploit that by replacing the 0 with metallic, so the range becomes metallic-1. For metallic materials, this nullifies specular occlusion entirely, but allows it to gradually fade in if metallic is used to blend between a metal and dialectric. In theory this should not create any extra shader instructions and so should be essentially free.

Reasoning

The reason I think this change is worth making is twofold:

  1. It enables a material behaviour that cannot be created any other way (contrast that to the current behaviour which allows for many ways to get a totally black non-specular material)
  2. It makes sense to enforce the fact that metals do not absorb light (previously I explained that anytime albedo is (0,0,0) it should be assumed that all light is absorbed, but that doesn't actually make sense for metallic materials which do not absorb light)

The following examples use:
metallic = 1.0
specular = 0.5
albedo = (0,0,0)
roughness = 0.0

Before:
Screenshot (308)

After
Screenshot (307)
Note that the center is dark, but the reflection increases strength as it approaches the edge.

@clayjohn clayjohn added this to the 4.0 milestone Dec 3, 2022
@clayjohn clayjohn requested a review from a team as a code owner December 3, 2022 04:24
@fracteed
Copy link

fracteed commented Dec 3, 2022

@clayjohn thanks, I thoroughly tested it with lights and several environments, and it works as expected. Thanks for your very prompt fixes for these issues! I really need both of your new PR's from today to do a direct comparison between Blender/Substance and Godot, but it all looks good at this point.

Probably, not the place to ask, but it keeps bugging me. Why has the "Material" slot in 3.5 been renamed to "Surface Material Override" in 4.0? We now have slots for "Surface Material Override" and "Material Override". It seems like a good way to confuse a new user as to which one to use, when the simpler name of "Material" made it obvious.

@mrjustaguy
Copy link
Contributor

Material is a Mesh property. Material Override is a Mesh Instance Property that overrides Mesh Property.

@clayjohn
Copy link
Member Author

clayjohn commented Dec 3, 2022

Probably, not the place to ask, but it keeps bugging me. Why has the "Material" slot in 3.5 been renamed to "Surface Material Override" in 4.0? We now have slots for "Surface Material Override" and "Material Override". It seems like a good way to confuse a new user as to which one to use, when the simpler name of "Material" made it obvious.

We renamed it because new users often didn't realize that they were overriding the material set on the mesh itself on a per-MeshInstance basis. New users were setting it expecting it to apply to all MeshInstances using the same mesh, or alternatively, they would set a material on the mesh's surface material, and then later try to retrieve the material from the MeshInstance's surface material and were often confused why that didn't work. In the end, renaming the material override in the MeshInstance made the most sense.

@mindinsomnia
Copy link

Greatly appreciate your patience with my feedbacks & requests @clayjohn

These changes along with the previous changes are great in my opinion.

@akien-mga akien-mga merged commit daf168f into godotengine:master Dec 3, 2022
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants