-
-
Notifications
You must be signed in to change notification settings - Fork 21.3k
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
Implement LightmapGI shadowmasks #85653
base: master
Are you sure you want to change the base?
Implement LightmapGI shadowmasks #85653
Conversation
22a959a
to
065eb51
Compare
Since this is salvaged from a previous PR, unless it doesn't contain any of their code, please add them as co-author |
065eb51
to
f347004
Compare
Did I do it correctly? Sorry, I'm still new to github. |
83baccd
to
bb0763c
Compare
I think it's ready for review now. I'm still not certain whether the documentation is clear enough, or if the seams are noticeable enough to require blending them, though. |
servers/rendering/renderer_rd/shaders/forward_clustered/scene_forward_clustered.glsl
Outdated
Show resolved
Hide resolved
Reimplementing support for bicubic sampling could resolve this. If this doesn't suffice, we could add an option that uses
This is a clever idea I hadn't thought of before. It makes a lot of sense if you need sharp distant shadows but don't need indirect lighting data to be very precise 🙂 That said, in practice, you don't want distant shadows to be too precise as you run the risk of having aliasing visible in the distance (lightmaps and shadowmasks aren't mipmapped). |
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.
Tested locally, it mostly works as expected (I tested Forward+ and Mobile).
Testing project: test_lightmapgi_mobile.zip
The way shadowmask interacts with real-time shadows looks strange. I don't recall this happening with previous PRs, but maybe I started noticing this issue just now. This is most noticeable with Directional Soft Shadow Filter Quality set to Ultra, but it occurs with any setting:
I'd expect the final shadow color to be min(shadowmask, real_time_shadowmap)
. Right now, it seems to be real_time_shadowmap > 0.0 ? real_time_shadowmap : shadowmask
. This issue affects both Forward+ and Mobile.
If you set Directional Soft Shadow Filter Quality to Hard, notice how shadowed area transitions look much sharper than intended:
Regarding fixing seams, I'd say it's worth the effort as right now, seams can be quite noticeable in the distance:
Real-time shadow | Shadowmask |
---|---|
bb0763c
to
c1a73f2
Compare
c1a73f2
to
a2208ad
Compare
OIDN works correctly now, I think this PR is finally ready for review |
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.
Tested locally, it works as expected.
Some remaining issues I noticed:
-
When you set the shadowmask mode to None and bake lightmaps again, the shadowmask source texture isn't removed from the filesystem (unlike what the description implies).
-
In Compatibility, DirectionalLight3D's Blend Splits doesn't work correctly with the last split when the LightmapGI shadowmask mode is Overlay:
It works well in Forward+ and Mobile.
- With all rendering methods, dynamic object shadows are not rendered (even up close) when using the Only shadowmask mode, even though I'd expect them to appear:
Replace | Only |
---|---|
In comparison, this works with the Replace and Overlay shadowmask modes.
- During a recent rendering meeting, it was suggested that the shadowmask texel scale should be adjustable separately from the lightmap texel scale, so that your shadowmask can be more detailed than the lightmap. This can be important if you only bake indirect light, as indirect light data is low-frequency but the shadowmask can be higher-frequency, especially if using the Only shadowmask mode.
- I'd probably add Shadowmask Texel Scale property below Shadowmask Mode that is only shown when Shadowmask Mode is set to a value other than None. This value would be relative to Texel Scale, with
1.0
being the same resolution,2.0
being double resolution on each axis and so on.
- I'd probably add Shadowmask Texel Scale property below Shadowmask Mode that is only shown when Shadowmask Mode is set to a value other than None. This value would be relative to Texel Scale, with
I'm not sure whether this should be the correct behavior, I intended the docs to imply that the reference to the texture would be removed so it doesn't get loaded. Though it may be better from the user's perspective to delete it, since it will get overwritten anyway on subsequent bakes.
I'm not certain how to approach this, currently all real-time shadows are ignored in the shader when an object has an 'Only' shadowmask. One way would be to only disable shadow casting for static objects with a shadowmask, but that would mean they wouldn't also cast shadows onto dynamic ones. The shadows of static and dynamic objects are combined, so it's not possible to tell which ones are cast by the dynamic ones. Edit: #77683 could help resolve this. |
This is not what I would expect for a mode called "Only". I think it can be added as a new mode after the merge, maybe something like "static + dynamic". This is a big PR and is in a good shape, there’s no need to keep overthinking it. New features can always be added later. Please, consider merge before we miss the window of opportunity ^_^ |
Seconding @SpockBauru. Not having shadow masks in any form currently makes Godot's baked shadows impractical for a lot of projects, forcing those projects to accept the performance cost of real-time shadows for static geometry only in order to have acceptable shadows for dynamic geometry. This feature is currently a major blocker for optimizing performance in 3D games and supporting lower-end platforms with Godot without sacrificing shadows entirely. In my opinion, getting this merged in at least a basically usable form ought to be a very high priority. |
Eh, correct me if I'm wrong, but wouldn't you still need to render static objects in the shadow map for dynamic objects to be able to get statically cast shadows? |
I've also initially thought about this, but we can't separate the two because they render into the same shadow map AFAIK. This means that if static objects both receive shadows and use static shadowmap, they could potentially look wrong due to getting both dynamic and static shadows. This is why most engines have the Only mode not cast dyanmic shadows onto static objects, and Replace mode for when you need that to work where static shadows are only rendered outside of dynamic shadow's range. |
c44a9cf
to
385b322
Compare
Rebased, hopefully I didn't accidentally break something. Update: I did |
Updated my MRP to the latest windows fragment: I'm getting flickering issues while in movement in Forward+ rendering. This does not happens on Mobile or Compatibility: shadowmask_flickering.mp4Edit:
|
I see no flickering on my setup. This PR is a huge improvement in terms of quality and performance. I can switch to short distance (i.e. 50m) and cheap orthogonal shadows, and still the entire scene is properly lit. Thank you! |
385b322
to
5a64809
Compare
Both issues should now be resolved |
This comment was marked as resolved.
This comment was marked as resolved.
Thank you, now Forward+ works and Mobile seems to be working. But I'm facing a weird issue on the Compatibility renderer, it took a while to find the issue but here it is: On Compatibility renderer when I set a Spotlight to "Dynamic" the shadowmask vanishes and is replaced by the shadowmap, even when Shadowmask is set to "Only". When I look away from the light the shadowmask reapears. Rebaking doesn't help the issue. shadowmask_compatibility.mp4
Not sure If I understand the issue, maybe you are referring to setting the In LightmapGI, when the mesh GI Mode is set to Dynamic it will be lit by the Lightmap Probes (that little ballls). Those are like "little lights" that illuminates dynamic objects. The Shadowmasks are not baked on Dynamic objects. |
Thanks! That was the cause. I completely forgot about GI mode. My apologies! |
Note that this option doesn't affect shadowmask baking due to #98684. |
Oh, thanks for pointing this out! |
5a64809
to
14a7472
Compare
Shadowmasks with vertex shading now work correctly. There is only one remaining issue that is not related to this PR: with the Mobile renderer, lightmaps don't render at all on certain scenes. |
14a7472
to
7635222
Compare
Point/omni lights and shadowmasks should now work correctly on Compatibility. Therefore, I think this PR is now complete |
Co-authored-by: dearthdev <[email protected]>
7635222
to
8a29c4a
Compare
Tested my MRP on Forward+, Mobile and Compatibility renderers. Shadowmasks is working as intended without any visual issues. Also baked the LightmapGI on each renderer and everything just works \o/ I didn't find any issue related to this PR and believe that is ready to merge :D Shadowmask_Compatibility2.mp4After mergeAfter merging, here are some thoughts about what I believe should be prioritized for LightmapGI overall: After merge LightmapGI Blocking bug I believe this bug should be prioritized. After merge todo feature After merge to be discussed |
#99367 should fix the mobile version |
Salvages #77284
Closes: godotengine/godot-proposals#2354
Adds support for baked shadowmasks to LightmapGI. The shadowmasks are saved as compressed images and applied on top of the dynamic shadowmaps.
Progress:
To consider:
To do in the future (important):
Compress grayscale textures as RGTC_R,Edit: doneImplement and use bicubic sampling (See Implement bicubic sampling for lightmaps #89919)Edit: done,shadowmask_resolution_ratio
property to scale shadowmasks independently from lightmaps.