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

Restrict sampler hint validation to only screen texture hints #94902

Merged
merged 1 commit into from
Jul 30, 2024

Conversation

clayjohn
Copy link
Member

Fixes: #94897

This chunk of code was supposed to ban mixing textures with incompatible hints, but in reality what it did was stop the function from accepting any two textures with different hints. Most hints are compatibility with one another. In practice, we only care if the hint is a screen texture type (color, depth, or normal-roughness) as those are the only ones with custom code-gen.

CC @BastiaanOlij

@clayjohn clayjohn added this to the 4.3 milestone Jul 29, 2024
@clayjohn clayjohn requested a review from a team as a code owner July 29, 2024 05:34
@BastiaanOlij
Copy link
Contributor

BastiaanOlij commented Jul 29, 2024

If I read this right, if I call a texture function with a screen_texture first, and then call it with a depth_texture next, it will error out because the hints aren't the same, even though both would be compatible as far as reading from the texture goes. We're trying to prevent reading from screen_texture and then reading from albedo_texture because the first could have layers and thus alter the code to add ViewIndex to the UV coords, while the second does not.

Should the check be something like:

bool ShaderLanguage::_can_have_layers(ShaderNode::Uniform::Hint p_hint) {
	if (p_hint == ShaderNode::Uniform::HINT_SCREEN_TEXTURE ||
			p_hint == ShaderNode::Uniform::HINT_NORMAL_ROUGHNESS_TEXTURE ||
			p_hint == ShaderNode::Uniform::HINT_DEPTH_TEXTURE) {
		return true;
	}
	return false;
}

...

if (arg->tex_argument_filter == p_filter && arg->tex_argument_repeat == p_repeat && arg->can_have_layers == _can_have_layers(p_hint)) {

...

arg->can_have_layers = _can_have_layers(p_hint);

Or is there more going on that make screen, depth and normal textures incompatible?

@clayjohn
Copy link
Member Author

Or is there more going on that make screen, depth and normal textures incompatible?

There is more. Layers is only one factor that makes them incompatible. For the color texture, we have the luminance_multiplier that needs to be inserted when using an RD backend. For the normal_roughness texture, we have to scale it when using the Forward+ backend.

@BastiaanOlij
Copy link
Contributor

Or is there more going on that make screen, depth and normal textures incompatible?

There is more. Layers is only one factor that makes them incompatible. For the color texture, we have the luminance_multiplier that needs to be inserted when using an RD backend. For the normal_roughness texture, we have to scale it when using the Forward+ backend.

Ok cool, than I think what you're doing here seems sound. It's a shame we can't automatically create variants of the same function when we detect the input texture has such requirements. Though that might be something worth thinking about for a future improvement

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, and resolves the issue we had with triplannar UVs in XR tools

@akien-mga akien-mga merged commit 52f2290 into godotengine:master Jul 30, 2024
18 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.

Triplanar UVs are broken
3 participants