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

Various fixes for transmittance effect #93448

Merged
merged 1 commit into from
Sep 3, 2024

Conversation

clayjohn
Copy link
Member

Fixes: #88083

Fixes: #73946

Use correct shadow sampling for omni and spot lights. We changed how shadow maps were sampled in #51335, but we forgot to update the code for transmittance, so it was sampling from the totally wrong position. This resulted in the bug reported in #88083

Disable transmittance if shadows are disabled. Transmittance requires reading from the shadow map. It used to be calculated at the same time as shadows but was moved in #45023. It has been broken ever since. I noticed this while working on something unrelated.

Correct DirectionalLight transmittance bias to match shadow bias (its still pretty sensitive though). The calculation of bias was updated in #51025 but it wasn't updated for Transmittance. The result is that the default bias is way too large. Users need to manually adjust bias to a tiny amount for it to work at all.

before
Screenshot from 2024-06-21 17-05-43

after
Screenshot from 2024-06-21 17-06-07

Use correct shadow sampling for omni and spot lights

Disable transmittance if shadows are disabled

Correct DirectionalLight transmittance bias to match shadow bias (its still pretty sensitive though)
@clayjohn clayjohn added bug topic:rendering cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Jun 22, 2024
@clayjohn clayjohn added this to the 4.4 milestone Jun 22, 2024
@clayjohn clayjohn requested a review from a team as a code owner June 22, 2024 01:23
Copy link
Contributor

@BastiaanOlij BastiaanOlij left a comment

Choose a reason for hiding this comment

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

LGTM, I'm not too familiar with the transmittance code so purely judging on code style.

Only question I have seeing you mention in your description that transmittance requires shadows to be enabled, why you're not doing a #if defined(LIGHT_TRANSMITTANCE_USED) && !defined(SHADOWS_DISABLED) and skip over the code entirely?

@clayjohn
Copy link
Member Author

clayjohn commented Sep 3, 2024

LGTM, I'm not too familiar with the transmittance code so purely judging on code style.

Only question I have seeing you mention in your description that transmittance requires shadows to be enabled, why you're not doing a #if defined(LIGHT_TRANSMITTANCE_USED) && !defined(SHADOWS_DISABLED) and skip over the code entirely?

Just because the first couple variables are still needed for the light compute function:

#ifdef LIGHT_TRANSMITTANCE_USED
	float transmittance_z = transmittance_depth; //no transmittance by default
	transmittance_color.a *= light_attenuation;
#ifndef SHADOWS_DISABLED

So the defines are slightly separated, but in practice it should be fine

@akien-mga akien-mga merged commit 667778c into godotengine:master Sep 3, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

Cherry-picked for 4.3.1.

@akien-mga akien-mga removed the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Sep 16, 2024
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.

Subsurface Scattering Transmittance visual glitches SSS Transmission visual bug
3 participants