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 shader uniforms has null as default value #95626

Merged
merged 1 commit into from
Jan 31, 2025

Conversation

Chaosus
Copy link
Member

@Chaosus Chaosus commented Aug 16, 2024

@Chaosus Chaosus requested a review from a team as a code owner August 16, 2024 09:22
@Chaosus Chaosus force-pushed the shader_fix_default_params branch from 7e2b344 to 5368dff Compare August 16, 2024 09:25
@Chaosus Chaosus force-pushed the shader_fix_default_params branch from 5368dff to 469c536 Compare August 16, 2024 12:11
@Chaosus Chaosus requested a review from a team as a code owner August 16, 2024 12:11
@AThousandShips AThousandShips added this to the 4.4 milestone Aug 16, 2024
@Chaosus Chaosus force-pushed the shader_fix_default_params branch from 469c536 to 55fccdf Compare August 16, 2024 12:57
@Chaosus Chaosus force-pushed the shader_fix_default_params branch from 55fccdf to 285a9e5 Compare September 17, 2024 10:33
Comment on lines +4506 to +4532
case ShaderLanguage::TYPE_BOOL:
if (array_size > 0) {
PackedInt32Array array;
for (int i = 0; i < array_size; i++) {
array.push_back(false);
}
value = Variant(array);
} else {
VariantInitializer<bool>::init(&value);
VariantDefaultInitializer<bool>::init(&value);
}
break;
Copy link
Member

@akien-mga akien-mga Jan 30, 2025

Choose a reason for hiding this comment

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

TL;DR: I looked into how to improve this code myself but in the end I gave up.

I was still wondering if there isn't a simpler way to do this. There must exist a pre-existing method in the codebase to construct a Variant with a given type and default initialize it.

From a quick look, I found:

void Variant::construct(Variant::Type p_type, Variant &base, const Variant **p_args, int p_argcount, Callable::CallError &r_error);

Which I think you would use like this:

Callable::CallError ce;
Variant value;
Variant::construct(variant_type, value, nullptr, 0, ce);

This would allow to simplify the cases quite a bit, like (untested):

switch (p_type) {
	case ShaderLanguage::TYPE_BOOL:
		if (array_size > 0) {
			type = Variant::PACKED_INT32_ARRAY;
		} else {
			type = Variant::BOOL;
		}
		break;
	case ShaderLanguage::TYPE_BVEC2:
		if (array_size > 0) {
			type = Variant::PACKED_INT32_ARRAY;
			array_size *= 2;
		} else {
			type = Variant::INT;
		}
		break;
	...
	}
}

Callable::CallError ce;
Variant value;
Variant::construct(variant_type, value, nullptr, 0, ce);

if (array_size > 0) {
	// Figure out a way to resize the array in Variant and `zero()` it.
}

But I finally noticed that this is pretty much adapted from ShaderLanguage::constant_value_to_variant and so keeping a consistent format might be worth it. The type conversions are also less trivial than I thought with types not natively compatible with Variant like bvec2.

And so at this point I gave up on trying to make this better :P

@akien-mga
Copy link
Member

Would be worth a rebase before merging as this is fairly old.

@Chaosus Chaosus force-pushed the shader_fix_default_params branch from 285a9e5 to 570e59d Compare January 31, 2025 04:36
@Chaosus
Copy link
Member Author

Chaosus commented Jan 31, 2025

@akien-mga Done

@Repiteo Repiteo merged commit 1586c56 into godotengine:master Jan 31, 2025
19 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Jan 31, 2025

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.

Shader uniforms has null as default value
4 participants