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: skip smoothen positions for flat triangles #53322

Conversation

williamd67
Copy link
Contributor

Smoothening positions for flat, non-smoothened, triangles is unnecessary and
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.

This is solved by skipping smoothening of positions for flat triangles.
A triangle is determined to be flat as its vertex normals are equal.

This PR replaces #53264 which will be closed.
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-43

@jcostello
Copy link
Contributor

@williamd67 The issue seems to be fixed. How ever in the Test.tscn scene there is a weird thing when baking in diferent qualities. Not sure if its related with the fix

Low

image

Medium

image

High

image

Ultra
image

@williamd67
Copy link
Contributor Author

@jcostello I have an idea what causes this. If so, it should not happen with a single bounce. In that case it is also not related to this fix.

With the introduction of ignoring back faces, I also added an 'ActiveRay' counter to ensure that only rays that actively contribute to the light of a texel count. In the bounces after the first, the rays that reach the sky (no hit) are not counted. This may cause this effect as a single ray that hits the lighted wall may add a lot of light to this texel (all other rays reach the sky and are not counted). I expect that due to the higher quality more rays are traced and there is a higher chance that one may hit the lighted wall. The only thing I cannot explain is why medium is lighter than high. Yet it may be caused by a ray under a specific angle that is only traced in medium and not in high.

BTW: if this is the case there will be an easy fix (increment 'ActiveRay' counter for rays that reach the sky in all bounces). I will test this when I have some time and create a new PR.

@jcostello
Copy link
Contributor

Very nice. Thank you for the hard work. I enjoy testing your PRs

@jcostello
Copy link
Contributor

Since this resolve the original issue and @williamd67 confirms that my comment is not related with the fix. I think this would be ready for merge once the code is reviewed

@akien-mga akien-mga requested a review from JFonS October 5, 2021 13:57
@JFonS
Copy link
Contributor

JFonS commented Oct 6, 2021

Since it seems to be a precision issue, I'm a bit concerned with using any(notEqual(norm_a, norm_b)) to compare the normal vectors. The same issue will most likely also happen with normals that are very similar but not exactly the same, so I would use dot(norm_a, norm_b) < 0.99 instead. The comparison value can be adjusted, but 0.99 corresponds to ~8 degrees between vectors, which seems reasonable to me. Very flat faces won't have many shadow termination issues anyway, so we can leave a safe margin.

@williamd67
Copy link
Contributor Author

Good point, I will update this PR. And as the flatness of a triangle is independent of the fragment, I moved the code to the vertex-shader.

Smoothening positions for flat, non-smoothened, triangles is unnecessary and
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.

This is solved by skipping smoothening of positions for flat triangles.
A triangle is determined to be flas as its vertex normals are equal.
@williamd67 williamd67 force-pushed the GPULightmapper-skip-smoothen-positions-flat-triangle branch from adc51bf to 9e58b02 Compare October 12, 2021 16:24
@akien-mga akien-mga merged commit 2eeb9a8 into godotengine:master Oct 13, 2021
@akien-mga
Copy link
Member

Thanks!

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.

5 participants