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

PBR Roughness is broken in 4.0 beta #69476

Closed
fracteed opened this issue Dec 2, 2022 · 25 comments
Closed

PBR Roughness is broken in 4.0 beta #69476

fracteed opened this issue Dec 2, 2022 · 25 comments

Comments

@fracteed
Copy link

fracteed commented Dec 2, 2022

Godot version

4.0 beta 7

System information

Windows 10 960m

Issue description

There seems to be several issues currently with roughness in spatial materials in 4.0.
The first one that I observed was that a full black albedo material could have no reflections, either from the environment (hdri or procedural sky) or lights, regardless of the roughness amount. Albedo color should not effect roughness.

This is how it looks in 3.5 with a procedural sky which is correct:

roughness_issue_3-5_env_clip2.mp4

This is how it looks in 4.0 with metallic showing nothing, but non-metallic is ok:

roughness_issue_4_env_clip.mp4

In 3.5 with just an omni:

roughness_issue_3-5_light_clip.mp4

In 4.0 with an omni:

roughness_issue_4_light_clip.mp4

A most likely related issue is the roughness in general looks much less rough than it should.

Here is a comparison using a hdri and a metallic material in 3.5 and 4.0:

godot_metallic

And for reference in Blender and Substance Painter. As can be seen 3.5 is very close, but 4.0 is not rough enough:

blender_substance_metallic
Here are the same comparisons using a dielectirc material for all 4 apps:

godot_dielectric

blender_substance_dielectric

@clayjohn this is probably up your alley! Afaik these issues have been in 4.0 for a while.

Steps to reproduce

  • Create a sphere
  • Give ir a material with pure black albedo and add a light and/or a sky to the scene
  • No reflections will be seen on the sphere

Minimal reproduction project

hdri_roughness.zip

@mrjustaguy
Copy link
Contributor

This is the Mobile renderer?

@fracteed
Copy link
Author

fracteed commented Dec 2, 2022

No, it is the Vulkan forward renderer. Haven't tried the others, but am guessing they have the same issue.

@clayjohn
Copy link
Member

clayjohn commented Dec 2, 2022

Looking into these now. The metallic/albedo issue doesn't look like a bug to me. An albedo value of (0,0,0) should eliminate specular reflections in a purely metallic material. You can observe the same behaviour in Blender for example.

Screenshot (286)

The roughness indeed looks off, so I will take a closer look and see if it is something that we can fix

edit: I can confirm that Godot appears to use a different roughness mapping than blender and Unreal (I checked both). This likely comes from #58418 where I remapped the high quality filtering to use non-perceptual roughness to match with the real time filtering option

@fracteed
Copy link
Author

fracteed commented Dec 2, 2022

@clayjohn That is not correct in blender. Not sure how you are getting that result, but the pure black will kill reflections in the facing but not all of the grazing angle reflections ...as can be seen in these videos. They all have the same hdri, with material as full metallic, roughness of 0, ramping up from full black to full white in the albedo:

Eevee:

blender_eevee0000-0145.mp4

Cycles path tracer (which is ground truth in PBR):

blender_cycles0000-0134.mp4

Substance Painter:

substance_metallic0000-0157.mp4

And godot 4 , which looks like the facing and grazing angles are both going up and down at the same rate:

gofot_40000-0231.mp4

@clayjohn
Copy link
Member

clayjohn commented Dec 2, 2022

@fracteed All I did was turn metallic to 1.0 and base color to black:

Screenshot (302)

Unreal Engine is the exact same. With Metallic at 1.0 and albedo at black, there are no reflections

Screenshot (300)

I can't see your settings in blender above, but for substance you can see that substance differentiates between base color and Metallic color and metallic color is set to white. That is likely where the reflections are coming from. In game engines we usually simplify and make albedo turn into "metallic color" based on the metallic value. Perhaps in Blender you are also using a BRDF

@fracteed
Copy link
Author

fracteed commented Dec 2, 2022

@clayjohn Here is the blend file. And showing in blender changing the metallic slider from full to no metallic. The grazing reflections are not really changing, only the facing. Just using the default principled BSDF.

eevee_metallic_slider0000-0202.mp4

roughness_metallic_black.zip

@clayjohn
Copy link
Member

clayjohn commented Dec 3, 2022

@fracteed Opening that blend file is giving me the same results I got before. I'm downloading the latest version of blender now to see if the difference is caused by my blender being out of date

@fracteed
Copy link
Author

fracteed commented Dec 3, 2022

@clayjohn That was done in blender 3.3. If you want to send me the blend file you are using I can look at it.

Just tried in Unity, as I don't have Unreal installed. Not sure how to put in the hdri, so just using the default sky. Same material with 0 roughness and scrubbing between metallic range of 0 -1 and base color of 0 -1. Getting the same result as I would expect.

unity_black_material30000-0381.mp4

@fracteed
Copy link
Author

fracteed commented Dec 3, 2022

@clayjohn and lastly, here it is shown in Substance Designer. You can see from the node graph what is going on. Black base color, black for roughness and I am scrubbing the metallic from full black to white range. As can be seen the grazing reflections stay unchanged:

subs_designer_metallic0000-0318.mp4

@clayjohn
Copy link
Member

clayjohn commented Dec 3, 2022

@fracteed Thanks for following up. With Blender 3.3 I am now getting the same results you are. I have also taken a closer look at Godot 3.5 and am convinced that you are right, the old behaviour is more "correct". I'll take a look at the current PBR stuff and see if I can spot what regressed.

Edit: Naturally this is going to be more complex than it at first seems. The regression comes from #63587 which was implemented in response to godotengine/godot-proposals#4818 Which was an extension of #40753. The trouble is #40753 was reporting as a bug, the exact behaviour you are asking to be implemented. In that Issue the user insists that all PBR renderers extinguish the specular reflection if the base color is dropped to solid black.

@fracteed
Copy link
Author

fracteed commented Dec 3, 2022

@clayjohn good to hear, as I had run out of PBR renderers to demo it on :)

@fracteed
Copy link
Author

fracteed commented Dec 3, 2022

@clayjohn and just on a related note. Is it a known issue that hdri's look so bad in the background in 4.0? If you look at the hdri in this issue in 3.5 it is fine, but in 4.0 it is distorted. I can file a separate issue if this is not already known.

@clayjohn
Copy link
Member

clayjohn commented Dec 3, 2022

@clayjohn and just on a related note. Is it a known issue that hdri's look so bad in the background in 4.0? If you look at the hdri in this issue in 3.5 it is fine, but in 4.0 it is distorted. I can file a separate issue if this is not already known.

@fracteed Kind of, the HDRI compressor is really broken. I couldn't even open your scene on my device, I had to manually edit the .import file to disable VRAM compression. With VRAM compression disabled the background looks fine

Edit: Looks like it has already been reported #67776

@clayjohn
Copy link
Member

clayjohn commented Dec 3, 2022

Now that I have refreshed myself on the issue, I am put in a difficult place. Very technically speaking, an albedo value of (0,0,0) is not physically plausible for a metallic object. Metals can only have an albedo between 0.66 and 1.0 see chart here. Anything outside that range is no longer physically based.

In #40753 I mentioned the above fact in support of the 3.x behaviour (the behaviour that you want restored).

However, I later added #63587 as I was increasingly seeing something like #63587 implemented in other engines and was finally swayed by godotengine/godot-proposals#4818. At the time, it made sense to me that an object with an albedo of (0,0,0) would absorb all incoming light. Technically speaking that is what should happen under our PBR definition of albedo despite the fact that an albedo of (0,0,0) does not exist in the real world. Also, at the time it appeared that all other PBR renderers had the same behaviour.

Now the story is a bit different. Eevee has flipped and is now back to our old behaviour. Unity and Substance use the old behaviour. Unreal, Filament, and Frostbite all use the same behaviour that we now use in Godot 4.0. So it is totally unclear to me which we should go with.

Filament has by far the best discussion/justification for why they use their current behaviour (they cite frostbite in doing so) https://google.github.io/filament/Filament.html#lighting/occlusion/specularocclusion. I can't find any discussion with respect to Blender/Eevee as to why they switched

@fracteed
Copy link
Author

fracteed commented Dec 3, 2022

@fracteed Thanks for following up. With Blender 3.3 I am now getting the same results you are. I have also taken a closer look at Godot 3.5 and am convinced that you are right, the old behaviour is more "correct". I'll take a look at the current PBR stuff and see if I can spot what regressed.

Edit: Naturally this is going to be more complex than it at first seems. The regression comes from #63587 which was implemented in response to godotengine/godot-proposals#4818 Which was an extension of #40753. The trouble is #40753 was reporting as a bug, the exact behaviour you are asking to be implemented. In that Issue the user insists that all PBR renderers extinguish the specular reflection if the base color is dropped to solid black.

Oh wow, I hadn't seen that "issue" before. I really think that this user was off base and doesn't really understand how pbr should work. Of course it is all approximations, and a user needs to find clever ways around the specular and reflection problems. There is a lot confusion in general in the blender and godot community with the default 0.5 specular value, as you no doubt know!

The gold standard for a realtime renderer should be comparing it with a path tracer. For instance Cycles and Eevee are designed to be as close as possible to each other. Everyone from studios to indies use the substance tools and these match up almost perfectly with Blender and Unity/Unreal. Godot 4 should definitely be trying to match Blender as closely as possible, which will also mean compliance with the Substance. Godot 3.5 was very close, though it still looked slightly off in the grazing angle, but it was much more acceptable than what is happening in 4.0.

@fracteed
Copy link
Author

fracteed commented Dec 3, 2022

@clayjohn ah, sorry I was typing before I read your comment. I did not realise that Filament and Unreal were different is this respect. Interesting but annoying! Funnily enough I use pure black metallic materials in my game which is why I noticed this change from 3.5.

Personally I think 4.0 should match Blender and Substance in this regard as that is what 99% of users will be using, but I understand this is a problem if you are basing the pbr code on Filament.

@fracteed
Copy link
Author

fracteed commented Dec 3, 2022

@clayjohn It would also be interesting to know if the path tracer in Unreal also uses this behaviour as well as their realtime engine? I have used tons of offline path tracers for rendering and never seen this behavior. Even though I understand that it may be more physically accurate since there are not any close to black metals materials in the real world.

Ideally you should stick to the 16-235 range for realistic materials I agree. As I work in motion graphics, I tend to like bending the rules with pbr when appropriate and prefer the "old" method which gives more control. This also would apply to npr style games as I am currently making. If I wanted a full black material, then I would just use an unshaded material.

@fracteed
Copy link
Author

fracteed commented Dec 3, 2022

@clayjohn and just for curiosity, I did look up some photos of real world black metal spheres. They are actually very reflective, and I can't even see the facing angle reflections disappear (which happens in the pbr approximation)

This is black tungsten carbide:
Carbide-Ball

And electroplated black (titanium) stainless steel (I couldn't download the photos):
https://metalsphere.com/product/black-stainless-steel-gazing-ball/

Much closer to the blender/substance look and what all path tracers that I have used do. Maybe it is something that should be put up for the community to vote on?

@clayjohn
Copy link
Member

clayjohn commented Dec 3, 2022

@clayjohn It would also be interesting to know if the path tracer in Unreal also uses this behaviour as well as their realtime engine? I have used tons of offline path tracers for rendering and never seen this behavior. Even though I understand that it may be more physically accurate since there are not any close to black metals materials in the real world.

I just checked Unreal engine's path tracer (in version 4.26) and it is the same behaviour as the realtime engine. Albedo (0,0,0) is solid black. As soon as Albedo is (1,1,1) there are reflections.

@clayjohn
Copy link
Member

clayjohn commented Dec 3, 2022

Well, I have an idea that may make everyone happy enough. To me, the most important part of #63587 was that it allows users to treat a specular of less than 0.02 as a signal to disable specular reflections with dielectric materials. This is standard behaviour in other PBR engines. As a side-effect of doing so is that reflections are also effectively disabled when using a black albedo with metals.

I can modify the code so that it only has an impact on dielectric materials. This matches the behaviour in Eevee which allows using specular to scale reflections on dielectrics, but does not eliminate reflections in metals when albedo is (0,0,0).

The only holdback I have is that the user who reported #40753 explicitly claims that metals with an albedo of (0,0,0) should have no reflections whatsoever. So at least one person desires that behaviour.

I will make a draft PR for testing and discussion. Let me know if you need access to a binary to test or if you can compile it yourself.

@fracteed
Copy link
Author

fracteed commented Dec 3, 2022

@clayjohn thanks for all your attention on this matter! It is interesting that Unreal went in that direction. I guess at the end of the day most Godot users will author in Blender and/or Substance so will expect them to look as close as possible. I do realise that the matter I am talking about is a somewhat corner case, but I still think a realtime pbr renderer should be as close as possible to an offline path tracer (the Unreal path tracer being the exception).

Out of interest I did a quick render test in Blender between eevee and cycle. The first 2 spheres are metallic (roughness of 0 and 1 respectively). The last 2 spheres are dielectric (roughness of 0 and 1). What I am seeing in the Unreal/Filament style is what I would expect for a full black, full roughness material. So with the Blender method you can get both, just using the roughness.

Cycles:
cycles_comparison

Eevee:
eevee_comparison

I personally rarely touch the 0.5 spec default except maybe for ice and water or skin. Are you proposing that the godot method will look the same as eevee unless you actually change the specular from 0.5? Yes, I think I can download directly from the github autobuild if a PR is put up. I haven't compiled it myself in a long time.

@clayjohn
Copy link
Member

clayjohn commented Dec 3, 2022

I personally rarely touch the 0.5 spec default except maybe for ice and water or skin. Are you proposing that the godot method will look the same as eevee unless you actually change the specular from 0.5?

More or less yes. Right now Godot ignores specular for metallic materials (for the purposes of IBL reflection anyway). While in Eevee a specular value of 0 removes the reflection regardless of whether the material is a dieletric or metal. My proposed middle ground would leave the specular behavior the same in Godot, but would change it so that an albedo of (0,0,0) would not remove reflections in metals.

@fracteed
Copy link
Author

fracteed commented Dec 3, 2022

Cheers, that sounds good. It is also the lights in the current method that don't show up at all for a full black/zero roughness metallic material (unlike 3.5). Will it also make lights visible if on the default 0.5 specular value?

I am happy to leave feedback when you are able to put up a PR (assuming there is a github autobuild)

@clayjohn
Copy link
Member

clayjohn commented Dec 3, 2022

Will it also make lights visible if on the default 0.5 specular value?

Yep, lights will have the same behaviour as IBL. Totally dark in the center, but will reflect at glancing angles

@akien-mga
Copy link
Member

Fixed by #69514 and #69522.

Repository owner moved this from To Assess to Done in 4.x Priority Issues Dec 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

5 participants