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

GPULightmapper: better algorithm to generate rays for indirect lighting #54919

Conversation

williamd67
Copy link
Contributor

Previous algorithm used an algorithm to generate rays that was not completely
random. This caused artifacts when large lighmap textures were used.

The new algorithm creates better rays and by that prevents artifacts.

This fixes #53770. Without denoising and limited quality there is more noise, yet with denoising the quality is better than before.

@williamd67 williamd67 requested a review from a team as a code owner November 12, 2021 14:29
@williamd67 williamd67 force-pushed the GPULightmapper-improve-noise-to-prevent-artifacts branch from c055be9 to fa17532 Compare November 12, 2021 14:44
@Calinou Calinou added this to the 4.0 milestone Nov 12, 2021
@akien-mga
Copy link
Member

FYI, you authored the commit with your $dayjob identity (see email in https://patch-diff.githubusercontent.com/raw/godotengine/godot/pull/54919.patch). If this wasn't intended, you can change it with git commit --amend --reset-author after editing your identity settings for the Godot repo.

@williamd67 williamd67 force-pushed the GPULightmapper-improve-noise-to-prevent-artifacts branch 2 times, most recently from 0a3c38f to b773ee0 Compare November 12, 2021 17:06
@williamd67
Copy link
Contributor Author

Thanks I fixed it. I did something incorrectly so I had to force-push it twice 😞.

@jcostello
Copy link
Contributor

I can confirm that this fix the issue on #53770.

@clayjohn
Copy link
Member

clayjohn commented Nov 13, 2021

Is it worth also replacing the other places vogel_hemisphere is used with hemisphere_ray_direction? We probably should if we can. At the very least quick_hash should be removed entirely.

@williamd67 williamd67 force-pushed the GPULightmapper-improve-noise-to-prevent-artifacts branch from b773ee0 to befde70 Compare November 14, 2021 10:23
@williamd67
Copy link
Contributor Author

williamd67 commented Nov 14, 2021

Good point. I checked the usage of vogel_hemisphere and it seems to be used in the same way as for indirect lighting. I am not familiar yet with the implementation of probes, still I think that replacing vogel_hemisphere with the new algorithm is an improvement.

I removed vogel_hemisphere and quick_hash.

@JFonS
Copy link
Contributor

JFonS commented Nov 16, 2021

I was going over the code and I noticed we are not taking into account the angle between the sample ray directions and the surface normal when integrating the incoming light.

This bit here:

light_average += light;

should be changed to light_average += light * dot(ray_dir, normal);, assuming both vectors are normalized and in the same hemisphere.

Another option would be to do importance sampling and, instead of adding the previously mentioned factor, use a cosine weighted sample distribution. That should give the same results and reduce noise, especially at higher sample counts.

In any case, it would be good to go over the current implementation and check that the incident angle factor is accounted for in all cases, either explicitly or by using a cosine weigthed distribution. The only exception I can think of ar light probes, which don't have a surface normal and simply store the amount of light reaching them, the scene shader will take care of the rest.

@williamd67
Copy link
Contributor Author

williamd67 commented Nov 17, 2021

@JFonS Good point and I verified it. As far as I understand, does the function hemisphere_ray_direction already generate cosine weighted directions. See per example slide 47 of this presentation or slide 41 of this presentation.

I will add a comment that this function generates 'cosine weighted directions' or maybe better I will change the name of the function. I renamed it to generate_hemisphere_cosine_weighted_direction.

For direct light the incident factor is part of the attenuation calculation.

@williamd67 williamd67 force-pushed the GPULightmapper-improve-noise-to-prevent-artifacts branch from befde70 to f88f257 Compare November 18, 2021 08:08
@JFonS
Copy link
Contributor

JFonS commented Nov 18, 2021

@williamd67 right, I was looking at the current implementation and not your PR for some reason. The new implementation looks good, yeah :)

My last nitpick has to do with lightprobes. They are only proxies that will relay the stored light to a real surface, so they only need to know about how much light reaches them. There is no incidence factor so the sampling distribution should be uniform.

Previous algorithm used an algorithm to generate rays that was not completely
random. This caused artifacts when large lighmap textures were used.

The new algorithm creates better rays and by that prevents artifacts.
@williamd67 williamd67 force-pushed the GPULightmapper-improve-noise-to-prevent-artifacts branch from f88f257 to 2ee77f6 Compare November 19, 2021 14:30
@williamd67
Copy link
Contributor Author

@JFonS I introduced a second function generate_hemisphere_uniform_direction which is used for lightprobes.

Copy link
Contributor

@JFonS JFonS left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@JFonS JFonS merged commit ea8d0de into godotengine:master Nov 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vulkan: LightmapGI indirect bounces with a lot of artifacts
6 participants