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: do not smoothen position #53264

Conversation

williamd67
Copy link
Contributor

Smoothen position caused positions to move outside their triangle which caused
side-effects as rays from those positions intersected with triangles which
could not be reached from the original triangle.

Smoothen position caused positions to move outside their triangle which caused
side-effects as rays from those positions intersected with triangles which
could not be reached from the original triangle.
@williamd67 williamd67 requested a review from a team as a code owner September 30, 2021 16:35
@Calinou Calinou added this to the 4.0 milestone Sep 30, 2021
@williamd67
Copy link
Contributor Author

This PR fixes the artifact of strange blob that was mentioned in #51638.
Screenshot from 2021-09-30 18-00-44
After removing the smoothening of positions:
Screenshot from 2021-09-30 18-02-51

@JFonS, do you know the situation this smoothening was needed, or do you have a scene that needed it, as I may have broken that case and like to look into it so can fix it (maybe in another way without this side-effect). Thanks.

@JFonS
Copy link
Contributor

JFonS commented Sep 30, 2021

If this section of code is causing artifacts then there's probably something wrong with it, but ideally we should try to keep it.

I did not write the original implementation of the GPU lightmapper, I just worked on some fixes and improvements, but I'm pretty sure this section is here to improve shadow termination. The same article is referenced somewhere in this file, but I believe this section is what Juan referenced while working on this feature. It has a good explanation of the problem and the solution we are (in theory, at least) implementing.

@williamd67
Copy link
Contributor Author

williamd67 commented Sep 30, 2021

Thanks @JFonS for the reference. I will investigate:

  1. if there is an issue with the implementation (or if this a known side-effect)
  2. the reference and determine if we could implement it without side-effects (if 1. seems to be correct)

I propose not to merge this PR till the investigation is concluded.

EDIT: the code is indeed needed for shadow termination so should not be removed. I will close this PR and create a new one with a correct name and implementation.

@williamd67
Copy link
Contributor Author

This PR is replaced by #53322 is the solution of this PR is incorrect.

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.

3 participants