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 shader_cache_dir_valid check to _save_to_cache #87096

Merged
merged 1 commit into from
Jan 15, 2024

Conversation

jsjtxietian
Copy link
Contributor

Fixes #87079

Alternatively one could argue that we should add the check to the call site instead of inside the function, but I think add a protection inside it is harmless and prevent future mistakes.

@jsjtxietian jsjtxietian requested a review from a team as a code owner January 12, 2024 03:19
@akien-mga akien-mga added bug topic:rendering cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Jan 12, 2024
@akien-mga akien-mga added this to the 4.3 milestone Jan 12, 2024
@akien-mga
Copy link
Member

akien-mga commented Jan 12, 2024

Alternatively one could argue that we should add the check to the call site instead of inside the function, but I think add a protection inside it is harmless and prevent future mistakes.

I think it might need to do both.

Usually we would use ERR_FAIL_COND if the contract of a function is not respected. Here, I think _save_to_cache should never be called if there's no valid shader cache dir, so it should raise an error.

And then the call site where it's not properly doing the check should be changed. _save_to_cache is called in three files, and two of them properly check:

if (shader_cache_dir_valid) {
	_save_to_cache(p_version);
}

But that check is missing in shader_gles3.h:

Version::Specialization *spec = version->variants[p_variant].lookup_ptr(p_specialization);
if (!spec) {
	if (false) {
		// Queue load this specialization and use defaults in the meantime (TODO)

		spec = version->variants[p_variant].lookup_ptr(specialization_default_mask);
	} else {
		// Compile on the spot
		Version::Specialization s;
		_compile_specialization(s, p_variant, version, p_specialization);
		version->variants[p_variant].insert(p_specialization, s);
		spec = version->variants[p_variant].lookup_ptr(p_specialization);
		_save_to_cache(version);
	}

@jsjtxietian
Copy link
Contributor Author

But that check is missing in shader_gles3.h

Yes I noticed that too, you are right, it need to do both, an if guard at the call site and ERR_FAIL_COND in the function .

@akien-mga akien-mga merged commit bf7e198 into godotengine:master Jan 15, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@jsjtxietian jsjtxietian deleted the save-to-cache branch January 15, 2024 12:44
@akien-mga
Copy link
Member

Cherry-picked for 4.2.2.

@akien-mga akien-mga removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Mar 10, 2024
@SinaZK
Copy link

SinaZK commented Jul 11, 2024

Could we see this on 4.1.5 too? We have a released mobile game using 4.1 and we need to get rid of many annoying errors when shader cache is off in the project settings.

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.

Disabling shader cache gives _save_to_cache error in editor
3 participants