-
-
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
LightmapGI: Implement shadowmask for DirectionalLight #77284
Conversation
f9b9cdd
to
52b06fc
Compare
I suggest enabling it by default, considering it increases quality a lot for not much gain in file size. It can also indirectly improve performance since you can use a lower Shadow Max Distance property than you otherwise could use with shadowmasking disabled. |
I was conflicted on this. My major concern was people having dynamic shadows disappear in the distance while baked shadows stayed and them thinking it was a bug. If they enable shadowmasking they're more likely to be aware of its pros and cons. I'll change it to enabled by default if you think that's better. |
As long as we document the default behavior in the Using LightmapGI documentation, it should be fine 🙂 |
Alright, I'll see if I can get to that in the new few days. |
It should be on by default, as devs should probably notice how the shadows disappear some 80-100m away from the camera using default settings, and you don't automatically bake lightmaps, so it's likely most people will understand that the shadows are Baked at that point |
Tested locally, it works. This is a pretty good start! Testing project: test_shadowmask.zip Some comments:
This should be able to work if possible, so that you can use shadowmasking in an "always shadowmask" fashion to improve performance. This means that static objects would also sample shadows from the shadow mask, but dynamic objects would still be able to cast shadows.
This may indicate a bug in the lightmapper itself, which we could work around by making nearly-white areas be considered fully white before saving the shadowmask. |
Right now, shadows in the shadowmask look like this when up close and Angular Distance is I'd expect them to look like this, matching the Shadow Blur property defined in DirectionalLight3D: For the shadowmask, we could fake this using a post-process blur filter, but this should preferably be implemented in the lightmapper itself to also affect shadows when using the Static bake mode (which bakes both direct and indirect light). So I'm not asking you to do that in this PR – it's just a reminder for the future 🙂 |
Thank you for providing clarification. I was wondering if applying the denoiser to the shadowmask would potentially mitigate some of the aliasing on sharp shadows, thereby reducing visual objections until the addition of shadow blurring. I'll test it and find out. |
I managed to fix the other issues, but I'm having trouble getting OIDN to denoise the shadowmask. According to the OIDN documentation, the RTLightmap filter cannot be used on LDR images, so I tried using the RT filter instead, but it still didn't work. |
703f177
to
b692b09
Compare
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 (rebased against e7d2e49), it works.
I'm a little confused about how this would work from the user perspective. If we are baking a shadowmask, isn't that a little bit contradictory to the "dynamic" bake mode? I feel like it will be very confusing for users to have baked light in the distance, but dynamic light up close. What is the recommended use for this? Right now it sounds like the following:
Is the goal to provide a way for users to have dynamic shadows up close, but baked shadows in the distance? |
Maybe, but we are already baking indirect light in this case anyway. It's expected that you cannot rotate a light after baking without the indirect lighting looking wrong. Color changes will be mostly OK, so you could still simulate a thunderstorm by brightening the sun temporarily for instance (this is not possible with fully static lights). As long as this is clearly documented, I think it's fine from an user perspective. We could communicate the need to rebake lightmaps after modifying light nodes better (like Unreal's infamous
This is pretty much how it works. Lightmaps are not intended to be used with day/night cycles anyway. If we wanted to support day/night cycles with lightmaps, we'd need to implement support for baking multiple sets of lightmaps that can be blended together. This would result in much longer bake times and larger file sizes, so it's probably not a good fit for indie games. Many AAAs do something akin to this, albeit with baked GI probes as opposed to lightmaps.
Yes 🙂 This is a very common need in games not only to improve quality, but also to improve performance (as you don't have to extend your shadow cascades as far away to get good shadows for static objects). Most polished games use a solution of this kind, and both Unity and Unreal support this use case. In fact, we could even support similar shadowmasking with VoxelGI and SDFGI in the long term, so that distant objects can cast static shadows without having to extend the cascades very far away. |
@Calinou The workflow issue is that we are requiring users to explicitly enable a setting on the light saying that it should not be baked and thus can move freely. Then, behind the scenes, we are baking the light and expecting the user not to move the light. With this PR the bake mode settings become Accordingly, the user loses the option to have the light included dynamically in GI, but not in lightmaps |
I guess this is an issue in situations where you use both SDFGI/VoxelGI and LightmapGI in the same scene. That said, even if you baked without a shadowmask, you wouldn't want to rotate the DirectionalLight too much to prevent the lightmapped areas from having wrong indirect lighting.
This is already the current expectation when using dynamic GI with lightmaps. Only subtle changes can be made without it looking too wrong. I believe Unity uses similar terminology for its "indirect light only" GI with lightmaps, but I don't remember it exactly. Of course, with VoxelGI and SDFGI, it's fine to perform significant changes to lighting after baking. |
Maybe the solution could be to add an option like unity or unreal engine one https://docs.unity3d.com/Manual/LightMode-Mixed.html And a blog about it uses. https://forum.unity.com/threads/what-is-the-purpose-of-mixed-lights.1099234/ , it may be possible i am confusing with dynamic light mode, i am no expert in this subject. |
I don't really understand the use of this much. BC6H (the texture format used for lightmaps in general) does not support transparency, so this would be very limited in how it could be used. Even the fallback (RGBE) does not either. So this PR only would work for LDR lightmapping, which is very useless unless you are targeting the GLES3 renderer somewhat, then it would be useless to have in the RD renderer. I do not think this feature is worth adding for the little benefit it brings. |
Okay, so I checked and this PR can't go through. The idea at the time was to eventually integrate the one Matias Goldberg did for Betsy in the lightmap pipeline (code should be quite easy to merge) and use that instead for BC6H compression at high performance. But this never happened and we forgot. That has to be done at some point, because using lightmaps uncompressed is extremely inefficient and we have to fix it, and by the time we fix it (as described above) this PR will no longer work because this format does not support transparency. Hence it will need to be rejected. |
@reduz can that be set as a priority? Its almost imposible to work with lightamps in large levels like the TPSdemo without compression. |
Regarding this, I suggest moving the shadowmask to a separate texture that uses the L8 format. This way, the original lightmap doesn't have an alpha channel and it can be VRAM-compressed with any method. The downside of this approach is that it generates an additional file on the filesystem and adds some complexity, but I think we need to do it in the long run if we want to support all rendering methods well. This was the approach I originally envisioned for my PR, but I didn't get it to work, so I settled on an alpha channel hack back then. |
Another possibility for this PR is to compress the shadowmask in a separate texture (that outputs to a grayscale PNG (so it gets compressed as DXT1 on PC and ETC1 on mobile). |
@jcostello This has to be done, but we are low on rendering contributors and the person working on the lightmapper moved on to other jobs lastyear. I can provide some guidance of what to do and where if anyone is interested. |
I've pushed a rebased version here (which I've tested again and can confirm still works): https://github.com/Calinou/godot/tree/shadowmask The new denoiser behaves well with the shadowmask; it doesn't affect the shadowmask in any way, which allows them to remain sharp. (If we want to smooth it out, it's probably best to use DirectionalLight3D's Angular Distance property, or have a separate filter as mentioned above.) Remaining work to do:
@DearthDev Are you available to look into this? If not, let us know and I'll mark this PR as salvageable so another contributor can complete it. |
Yeah, I won't be able to work on this anymore, go ahead and mark it salvageable or whatever. Sorry. |
|
Implements shadowmasking as proposed in godotengine/godot-proposals#2354.
See also #68702, #51330
Shadowmasking can be toggled on and off in the lightmap settings and is disabled by default.
Directional lights with their bake mode set to dynamic will bake their shadows into the alpha channel of the lightmap. Then, dynamic shadows will be used up close and baked shadows will be use outside the light's shadow max distance. If more than one directional light with its baked mode set to dynamic exists, then the shadowmask will only be present in areas where their shadows overlap. If no directional light with its baked mode set to dynamic exists, shadowmasking will be disabled and a warning printed saying such.
Note: #55867 has made testing with the mobile backend cumbersome.