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

Fix VoxelGI static light pairing #81124

Merged
merged 1 commit into from
Oct 5, 2023

Conversation

bitsawer
Copy link
Member

I tweaked the MRP project from the first issue a bit to make things easier to see (disabling default environment light, ramping up indirect light, changing static light type and position etc.).

Updated Test Project:
voxelgi-static-lights-bug.zip

Looks like when using lights with static bake mode, the lights are sometimes unpaired from the VoxelGI and not properly paired back. As VoxeGI does support static lights and in optimal conditions they do currently stay paired, I think the way static light are not sometimes paired back is the bug behind those issues. Note that when using VoxelGI and static lights you still need to re-bake VoxelGI if you move static lights around if you want the indirect lighting to stay accurate.

It looks like the issue is with pairing code that unpairs static lights from VoxelGI when updated but doesn't pair them back. Static lights are paired when created, but then lose their pairing after certain node tree transform or visibility change. How and when this happens also depends on node tree structure, which makes this especially nasty. By this I mean the behavior is different depending if the light is "above" or "below" the VoxelGI in the tree sibling node hierarchy. This seems to be caused by transform or visibility notification ordering during scene tree changes, which can invalidate pairing. So depending on order, if the light pairing is invalidated first and then VoxelGI things work out because updating VoxelGI pairs the lights again later. But if it happens the other way around the light stays unpaired until VoxelGI is updated (by baking, toggling its visibility etc.)

We can dig more into the test project now. When you open it without this PR, the VoxelGI indirect light is probably broken for the static spotlight. You can fix this by toggling VoxelGI node visibility off and on again or baking it again, this is where that static light gets paired again to the VoxelGI. Navigating to another scene tab and back also breaks this pairing again (because transform/visibility notification and update unpairs the light). NOTE: You need to save, close and then reopen the scene tab after re-ordering nodes to see the effects. If you try to play the project indirect light is also broken (this is what the workarounds in both projects attempted to fix). For example the first project has this:

await get_tree().create_timer(0.05).timeout

to set the data again and the second issue uses toggling visibility to force the light pairing. Interestingly 0.05 timeout value doesn't work for me, I have to use at least 0.1 timeout value on my machine.

You can experiment by reordering the SpotLight3D so that it is lower than VoxelGI in the sibling hierarchy. Simply doing this fixes the play mode indirect lighting for me (without this PR). Also note that changing order also changes the behavior when navigating to another scene tab and back, but you have to close the tab completely after re-ordering the node.

After this PR, static lights stay paired to VoxelGI even when the tree gets modified (by transform or visibility updates). Still, I'm not all that familiar with the pairing code so feedback is appreciated.

There are also somewhat similar possible pairing/update issues with LightmapGI (like #77356 and #77167 and good discussion in #77089).

@bitsawer bitsawer added this to the 4.2 milestone Aug 29, 2023
@bitsawer bitsawer requested a review from a team as a code owner August 29, 2023 12:31
@clayjohn
Copy link
Member

clayjohn commented Sep 6, 2023

As far as I can tell with this change static lights and dynamic lights will behave the same with VoxelGI. Or am I missing something? It seems that VoxelGI does not distinguish between light types.

@bitsawer
Copy link
Member Author

bitsawer commented Sep 6, 2023

I have been wondering the same thing and I suspect this fix might not be fully correct. The current Voxel/static light pairing behavior is currently so weird it's hard to say should be the correct result. For example, currently in master VoxelGI does seem to pair the static light in the same way as dynamic light when created, but then loses this pairing very easily. This PR keeps the pairing "alive", but I wonder how the pairing should work with baked voxel static lights in the first place.

I have been playing with the MRP and only once I managed to see what I think could be the correct behavior from a baked, static voxel light. That is, when a baked static light is moved the baked light "stays behind", still illuminating the meshes in the old position until re-baked. However, I haven't managed to repeat this behavior.

I'll try to play with this some more and see if I can figure out anything useful before throwing in the towel.

@clayjohn
Copy link
Member

clayjohn commented Sep 6, 2023

I have been wondering the same thing and I suspect this fix might not be fully correct. The current Voxel/static light pairing behavior is currently so weird it's hard to say should be the correct result. For example, currently in master VoxelGI does seem to pair the static light in the same way as dynamic light when created, but then loses this pairing very easily. This PR keeps the pairing "alive", but I wonder how the pairing should work with baked voxel static lights in the first place.

I have been playing with the MRP and only once I managed to see what I think could be the correct behavior from a baked, static voxel light. That is, when a baked static light is moved the baked light "stays behind", still illuminating the meshes in the old position until re-baked. However, I haven't managed to repeat this behavior.

I'll try to play with this some more and see if I can figure out anything useful before throwing in the towel.

I have a feeling that static lights maybe should just never pair with VoxelGI. And they should only be baked (which I think is handled completely on the scene side of things)

@bitsawer
Copy link
Member Author

I have been doing some experiments and reading through the code and I'm strongly starting to feel like this is a correct fix after all.

First, simply not pairing static light doesn't work because all lights are needed when the voxel code does its light calculation and it gets them from paired lights. Also, ignore my previous comment about once seeing a static light "staying behind" when moving it, that effect happens when you accidentally move the VoxelGI node instead of the light node. Duh. I can repeat it now and its not related to this.

It looks like VoxelGI does not process any light nodes at all during the bake, only geometry and their materials. It also supports dynamic geometry and handles it differently from static geometry. But it looks like VoxelGI treats all light nodes as dynamic and there doesn't really seem to be any code for handling them differently. The documentation states otherwise, but maybe it was a planned feature that was never added:

https://docs.godotengine.org/en/latest/classes/class_voxelgi.html in bake():

Note: GeometryInstance3Ds and Light3Ds must be fully ready before bake is called. If you are procedurally creating those and some meshes or lights are missing from your baked VoxelGI, use call_deferred("bake") instead of calling bake directly.

Interestingly, that paragraph was explicitly added in #58291, but it looks like the baking of static Light3Ds's was never implemented. I also believed that static lights needed to be baked when using VoxelGI, but it looks like that is not needed. Ping at @Calinou if you have any extra info about this.

I went through most of the VoxelGI related code in voxel_gi.cpp, voxelizer.cpp, renderer_scene_cull.cpp, gi.cpp and related shader files and I think this is the correct way to pair both static and dynamic lights. I also made several PR's fixing some related issues that I noticed during this adventure. In the best case, this PR is correct and fixes static lights. In the worst case, this PR simply makes broken VoxelGI static lights into functional dynamic lights. Other than fixing the issues, there doesn't seem to be any performance or any other impact. Still, having another person check my conclusion would be useful in case I missed something obvious.

@Calinou
Copy link
Member

Calinou commented Sep 14, 2023

Interestingly, that paragraph was explicitly added in #58291, but it looks like the baking of static Light3Ds's was never implemented. I also believed that static lights needed to be baked when using VoxelGI, but it looks like that is not needed. Ping at @Calinou if you have any extra info about this.

I distinctly remember being able to bake static lights in VoxelGI, but maybe this works with SDFGI (where baking occurs when the camera moves enough).

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation. Indeed I can't seem to find any distinction between static and dynamic lights in the VoxelGI code. It seems that all lights are somewhat dynamic

@clayjohn clayjohn added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Oct 5, 2023
@akien-mga akien-mga merged commit f2ba8ec into godotengine:master Oct 5, 2023
@akien-mga
Copy link
Member

Thanks!

@bitsawer bitsawer deleted the fix_voxelgi_static_lights branch October 6, 2023 07:40
@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Oct 24, 2023
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.3.

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