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

Ensure optional CopyEffects variants are loaded last. #84883

Merged
merged 1 commit into from Nov 15, 2023

Conversation

BastiaanOlij
Copy link
Contributor

@BastiaanOlij BastiaanOlij commented Nov 14, 2023

This is a quick fix for #84841

There is an issue with our shader cache when there are disabled variants, it does not seem to deal with these properly (though this seems to be a recently introduced issue, possibly due to shader groups being added in 4.2).
For the most part this isn't a problem as the missing variants end up being recompiled and the cache fixes itself. The exception are our copy effects where a new copy type was added after the optional variants.
There is a more serious bug in the shader cache that causes all variants to be deleted (even the ones loaded properly) but in such a way that the Godot can no longer compile new versions.

This PR works around the problem by moving this new variant before the optional variants. This fixes the immediate problem.

We will need to fix the cache logic at some point but that will be a 4.3 fix.

Bugsquad edit:

@BastiaanOlij
Copy link
Contributor Author

Just for future reference, we'll need to investigate this further. But I think the problem is that disabled variants are simply not added to the cache at all. So now it's trying to load the COPY_TO_FB_SET_COLOR variant as if it's the COPY_TO_FB_MULTIVIEW.
We'll probably need to save something into the cache to indicate that slow contains a disabled variant, so that COPY_TO_FB_SET_COLOR is saved in the correct slot.

@akien-mga akien-mga added the bug label Nov 14, 2023
@akien-mga akien-mga changed the title Ensure optional variants are loaded last. Ensure optional CopyEffects variants are loaded last. Nov 15, 2023
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks good to me.

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.

Seems fine to me. As-is, I think this is a very low risk change.

That said, I think it is a band-aid to an underlying problem with how we deal with disabling/enabling XR variants. So we should probably revisit this after 4.2 releases

@akien-mga akien-mga merged commit 00177d7 into godotengine:master Nov 15, 2023
15 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.

Toggling XR/Shaders/Enabled caused errors spam when using WorldEnvironment
3 participants