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

Add half-pixel offset to lightmapper rasterization. #81872

Merged

Conversation

DarioSamo
Copy link
Contributor

@DarioSamo DarioSamo commented Sep 18, 2023

Add half-pixel offset to lightmapper to fix issues where the ray would be generated from the wrong spot corresponding to the pixel and causing light leaks. Fixes Issue #69126.

As said in the issue and code comments, this is required so the lightmapper doesn't spawn rays from the very edge of the faces. It looks a bit hacky at first but there's a pretty solid reason behind it and as far as I can tell it fixes leaks in most scenarios I could find. Basically, by using this offset, the interpolated position the pixel shader receives will never be perfectly aligned at the edges of the faces but right on the middle of the pixel, which is what is usually desired.

Denoiser is recommended to be off when testing, as the current denoiser can cause light leaking on its own that doesn't exist in the noisy output due to the blurring.

Before
268679870-9d884417-2fc9-4389-bf2f-cd250e4c0414

After
268679903-96c80adf-a348-4f42-9ca3-09849a7caf2f

Before
image

After
image

(Lighting differences might be due to exposure adjustment or just less light leaking into the scene, but it's to be expected)

Testing out the fix in more scenes would be appreciated.

Bugsquad edit:

Add half-pixel offset to lightmapper to fix issues where the ray would be generated from the wrong spot corresponding to the pixel and causing light leaks. Fixes Issue godotengine#69126.
@DarioSamo DarioSamo requested a review from a team as a code owner September 18, 2023 15:10
@Calinou Calinou added this to the 4.2 milestone Sep 18, 2023
@Calinou Calinou added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Sep 18, 2023
@bitsawer
Copy link
Member

Might also help with #63437 in case someone wants to test it.

@Calinou
Copy link
Member

Calinou commented Sep 18, 2023

Before After (this PR)
Screenshot_20230918_232446 Screenshot_20230918_232917

Testing project: test_lightmap_preview_bake_4.x_1.zip

Before After (this PR)
Screenshot_20230918_233633 Screenshot_20230918_233753

Testing project: test_lightmap_one_sided.zip

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Great work!

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.

Looks great! The changes make sense. I haven't tested locally, but I trust that the testing from other reviewers was adequate.

@akien-mga akien-mga merged commit 2c125bf into godotengine:master Sep 20, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Sep 21, 2023
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.2.

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