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 trace_ray() function in the lightmapper missing hits with large triangles. #83040

Merged
merged 1 commit into from
Oct 9, 2023

Conversation

DarioSamo
Copy link
Contributor

@DarioSamo DarioSamo commented Oct 9, 2023

The DDA traversal had a conceptual error where it did an early termination of the search if it hit a triangle, but it didn't check if the hit position was inside the bounds of the cell being traversed. This can aid to fix light leaks such as the ones found in issue #75440.

I don't have much in the way of good comparison pics as the effectiveness of this fix is better observed with another change I'll introduce in my indirect bounces PR, which is the possibility of tracing lights on each bounce. In that scenario there's leaks that are impossible to fix otherwise that this PR will address.

What I did notice however is an increase in baking times as expected.

Unreal Sun Temple:
Before: Done baking lightmaps in 00:04:20.
After: Done baking lightmaps in 00:04:41.

Sponza:
Before: Done baking lightmaps in 00:00:56.
After: Done baking lightmaps in 00:01:02.

That's around ~10% slower bake times for what might not be a very noticeable fix right now, but it's hard to deny the fact the algorithm itself is wrong and can cause errors. The slower bake times are easily explained by the fact it's not doing early termination anymore. The fact the difference shows up means there was a significant amount of cases where the light mapper was not finding the actual closest hit but something further away.

So in short, there might be not be easy to find scenarios where we can confirm this fixes something right now, but it'd be for the best for the code to not work incorrectly.

…riangles.

The DDA traversal had a conceptual error where it did an early termination of the search if it hit a triangle, but it didn't check if the hit position was inside the bounds of the cell being traversed. This can aid to fix light leaks such as the ones found in issue godotengine#75440.
Copy link
Member

@lawnjelly lawnjelly left a comment

Choose a reason for hiding this comment

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

I had several problems of this type in my DDA ray tracer. I'm not sure I used this exact solution, but this seems simple and worth a try.

@DarioSamo
Copy link
Contributor Author

DarioSamo commented Oct 9, 2023

I've discussed it with @reduz and we have some interest in attempting to do a BVH acceleration structure on the GPU instead, as the DDA currently scales pretty poorly with scenes with higher triangle density (that Unreal scene for example takes way longer despite being less pixels and rays than Sponza). Whether that'll be any faster remains to be seen.

@akien-mga akien-mga changed the title Fix trace_ray() function in the lightmapper missing hits with large triangles. Fix trace_ray() function in the lightmapper missing hits with large triangles. Oct 9, 2023
@akien-mga akien-mga merged commit e0ea86f into godotengine:master Oct 9, 2023
15 checks passed
@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.

4 participants