Add microshadows support#112449
Conversation
8318a82 to
8a3f12a
Compare
|
You dropped you're crown king. |
|
Thank you very much for your contribution! Looks amazing in action |
8a3f12a to
1db8ac7
Compare
|
It seems like this broken the mobile pbr material since when you enable ambient occlusion the texture turns white . Also i got thousands of error messages like this upon opening the project with ao enabled When switching to forward+ the errors and bugs stay Here is a mrp with a pbr material from ambient cg textures https://drive.google.com/file/d/1aC9mlhSppWs-xJSCncHQKdQyjyy5fWWu/view?usp=drivesdk |
|
@Saul2022 Would you mind attaching the error in full? |
1db8ac7 to
e3809c3
Compare
Your problem seems to be related to fp16, so I would like to ask you to try again since I made some adjustments, as I don't have a device that properly supports fp16. |
The Issue is that while i can copy it, upon pasting it here, the browser crashes. |
Tried the latest artifact and i still got the errors , freeze and white texture as before. |
e3809c3 to
4384ffd
Compare
4384ffd to
bab7323
Compare
There was a problem hiding this comment.
Tested locally, it works as expected. The interactions with additional lighting features (emission, back lighting, SSS, transmittance) seem correct.
Testing project: test_microshadows.zip
Preview
AO light effect and SSAO/SSIL are all disabled.
| Renderer | Disabled | Enabled (0.85, default) |
|---|---|---|
| Forward+/Mobile | ![]() |
![]() |
| Compatibility | ![]() |
![]() |
| @@ -1949,6 +1951,7 @@ void fragment() {)"; | |||
| } | |||
|
|
|||
| code += " AO_LIGHT_AFFECT = ao_light_affect;\n"; | |||
| code += " MICRO_SHADOWS = micro_shadows;\n"; | |||
There was a problem hiding this comment.
This is always defined when ambient occlusion is enabled, so there's no way to use ambient occlusion without also having micro shadows enabled in the core shader. This leaves performance on the table if you set Micro Shadows to 0.0.
I suggest adding a boolean Micro Shadows > Enabled property in BaseMaterial3D (true by default), then a Micro Shadows > Strength property (0.85 by default). This way, you can set the first property to false and have no performance cost compared to 4.5.
(We can't use the existing Micro Shadows property alone, as inspector updates when dragging a slider currently don't work well.)
That said, maybe this option is too niche to be toggled on a per-material basis, and should be disabled in the project settings instead (similar to specular occlusion: rendering/reflections/specular_occlusion/enabled).
There was a problem hiding this comment.
Let me see if I understood you correctly:
- You want a new property, micro_shadows_enabled, to be added to ambient occlusion in StandardMaterial3D.
- This property should be set to true by default.
- You want to add a new setting in the project settings to globally disable microshadows.
- This global setting should be set to false by default.
Is this what you are suggesting?
There was a problem hiding this comment.
Yes, that sounds good to me. However, I would make the project setting affirmative (i.e. micro_shadows/enabled that is true by default).
In the future, we should look into adding similar "kill switch" properties for demanding BaseMaterial3D features, so that you can toggle them globally: godotengine/godot-proposals#2455
This is however nontrivial to implement in a way that doesn't require a restart to be effective (so you can use it in graphics settings menus). That said, we can leave this for later for a first implementation.
|
Has there been any progress on this PR? This is something I'd really like to have |
@Calinou made some suggestions that I didn't quite understand, so I asked him for clarification, but he hasn't replied yet. If you're wondering when this PR will be merged, it will likely be in 4.7 because 4.6 is already in feature freeze. However, that’s not my call; it’s up to the rendering team. |
4f4002c to
3dd08b2
Compare
3dd08b2 to
8c902d8
Compare
82a8b96 to
aa9d59e
Compare
aa9d59e to
29dc2c8
Compare
|
Rendering meeting: looks good, especially after good explanation by Kenzie during the meeting. Probably needs someone with an artist view to discuss some of the parameterisation. Also could use more testing feedback esp on mobile. |
May i ask, which kind of test?, since i did try on my s24/25 ultra phone before and it worked well on pbr materials i downloaded from websites like ambient cg. |
|
Here are some of my notes from my testing with meeting feedback: First impressions are excellent! ON
OFF
The materials feel much more grounded with self shadowing. ON
OFF
FeedbackOpt-In vs Opt Out
Global Disable Option
Over-parameterization?
Options in Standard Material limits potential
Needs More Light Type Support
Bent Normals?Microshadows assumes normals align with what comes from ambient occlusion. Bent normals are a cross between surface normals and ambient occlusion specifically made for self shadowing effects. If bent normals are available it seems ideal to use them? #ifdef BENT_NORMAL_MAP_USED
float NdotL = dot(bent_normal_vector, light_direction);
#else
float NdotL = dot(normal, light_direction);
#endif
Meeting Discussions
|
In my experience with this in Godot I also just used some value that seemed "Okay" and ran with it. I've also used microshadows in Unity's HDRP and I never thought about needing to change its strength/opacity after enabling it. |
Wasn't aware Unity had implemented microshadows. Its interesting to see that the opacity is provided as a post process volume setting. Presumably that's because of deferred rendering though. Would be still useful to get artist feedback but another production ready engine feeling it necessary to expose strength control is notable. |
Do you want microshadows to be enabled via a flag?
Unfortunately, I don't think support for other light sources will be possible, as it's a limitation of microshadows themselves. In the proposal, @clayjohn explains why it isn't feasible see godotengine/godot-proposals#12671 (comment).
This is actually very interesting; I never considered using a bent normal as an input. It might make sense to implement it.
Yes, this PR is based on how Unity implements microshadows, as it’s the only engine we have as a reference for this. The intensity parameter is intended to prevent overdarkening, since there are situations where microshadows can cause very intense shadows especially with the Naughty Dog method used for Mobile and Compatibility. |
If possible yes, following a similar convention to specular occlusion and be an opt-out in the shading tab. The microshadow strength should also ideally be in the shading tab.
I am a bit confused by the comment, is it mathematically infeasible (eg the approximation breaks in positional light sources) or just potentially problematic performance wise? The Naughty Dog approximation appears to be just an additional dot product (Huh, seemingly just N*L again? Would that be optimized out?) and a few mults/adds. Perhaps that would be good enough for positional light sources as something to apply to the visibility term? Would be interested to see benchmarking if so.
Perfect! Alright, seems like microshadow strength is worth keeping around then. |
|
@KenzieMac130 I still don't fully understand; could you clarify why implementing it as specular occlusion is preferable to the current implementation? I'd like to get more feedback on this and see if anyone else has a different suggestion. |
Yes, this is me taking off the graphics programmer hat and putting on the UX design hat. Specular occlusion is an existing feature whose goal is similar in principle (small shading improvement that should just be present without user intervention most of the time). Its not about mimicking the underlying implementation of specular occlusion but mimicking how the feature was exposed to the user to maintain UI/UX consistency. (enabled by default on future projects, under the shading tab, global enable in project settings, etc.) |
29dc2c8 to
a9555ed
Compare
|
Ok, I’ve implemented some of the changes proposed by @KenzieMac130:
Please let me know your thoughts on these changes. |
a9555ed to
95931c4
Compare
I'm not sure we should only enable it in new projects, since we didn't do the same for specular occlusion when it was implemented in 4.5. We generally accept slight visual changes as part of minor releases, like we did with the default Glow mode change in 4.6 (paving the way for better HDR support in 4.7). As long as the change is clearly documented in the migration guide and can be disabled after upgrading, I consider it to be acceptable. |
Yeah, I agree that it should be an acceptable small change. Making it only default for new projects was advised by some participants of the last rendering meeting about the multi-bounce and microshadow patches. |
|
I agree, it's weird to only activate microshadows by default for new projects. Besides, if users don't want the effect, it's very easy to turn it off for all materials via the project settings. |
Perfect
Feels very lonely and isolated there. Generally tabs should be used for grouping multiple properties logically, all Micro Shadows will ever have is strength. Makes sense to move it close to it's friend enable/disable button in the shading tab. :)
Looks good on my end. A bit darker but bent normals already does even without micro shadows. The longer/controllable shadow tail length feels like a positive. (I have only done limited testing though.)
Actually maybe that should be reverted? Seems like that is getting pushback. |
Nah, in my opinion, having a slider in the shading tab adds too much noise. The other option is to put it in the ambient occlusion tab alongside its predecessor, |
I agree that it should be moved to the AO tab |
The ambient occlusion tab is only active if ambient occlusion maps on the material are enabled. Materials can also recieve ambient occlusion through SSAO. The problem with putting it in the AO tab is that users are locked out of tweaking the effect unless they have an AO map for the material or temporarilly wish to enable and then disable ambient occlusion maps. This creates a UX papercut. If microshadows where to live in the ambient occlusion tab it should always be accessible to the user, not just when they have an AO map. I would argue that visual noise is not about too many parameters in one place, its about putting unnecessary parameters before the user in a high traffic area. People should rarely go to the shading tab unless they want to tweak a very specific fine-tuned behavior of the shading model outside of the standard PBR pipeline, so it makes sense to expose that fine grain control there IMO. |
Ok, you have a point here.
I still prefer to keep the We need more feedback. |
If you want to go full Bauhaus radio design one could argue the flag to enable/disable microshadows alongside a slider in the standard material is redundant. You could just have one slider called "microshadow" that controls strength and when it hits approximately zero you just add the disable flag behind the scenes. That way the user only has to worry about one parameter. Im not sure thats consistent with the rest of Godot's UX language but it would trim down on noise. Needs feedback but this could be an option. |
Then i think it might be good it we got a Review from someone from the usability team so we can decide what do we want. |
















closes #12671
This PR adds support for microshadows, first described by Naughty Dog in Uncharted 4, as a PBR alternative to Light Affect AO.
For more details, see... #12671.