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

Toggling XR/Shaders/Enabled caused errors spam when using WorldEnvironment #84841

Closed
jonSP12 opened this issue Nov 13, 2023 · 7 comments · Fixed by #84883
Closed

Toggling XR/Shaders/Enabled caused errors spam when using WorldEnvironment #84841

jonSP12 opened this issue Nov 13, 2023 · 7 comments · Fixed by #84883
Assignees
Milestone

Comments

@jonSP12
Copy link

jonSP12 commented Nov 13, 2023

Godot version

4.2 beta 5

System information

windows 10

Issue description

when choosing the option in the editor
project settings / XR - shaders / enable on , off
It works the frist time,
when choosing back to off ( default ) the entire screen stays black

Steps to reproduce

  • add a colorRect with a shader on
  • add node WorldEnvironment, choose new enviorement, canvas
  • go into project settings / XR - shaders / turn it on ( restart )
  • now turn it off
  • delete WorldEnvironment node, to clear the black screen, add WorldEnvironment again / canvas
  • the error presists in memory condition shader null()

XR_option01

XR_option00

XR_option02

Minimal reproduction project

XR_shaderOption.zip

@akien-mga akien-mga changed the title editor shader Option XR / shader on, off conflict with world enviorement Toggling XR/Shaders/Enabled caused errors spam when using WorldEnvironment Nov 13, 2023
@BastiaanOlij
Copy link
Contributor

Still investigating but the root cause seems to be that our shader cache is getting confused and not triggering a recompile and it snowballs from there.

Shader 'CopyToFbShaderRD' (group 0) SHA256: 88d9bbe69d7b49b42396bd6a374bd1ab31f986ad2eec697f60898639293bf8b6
ERROR: Condition "binsize < sizeof(uint32_t) * 3 + sizeof(RenderingDeviceVulkanShaderBinaryData)" is true. Returning: RID()
   at: RenderingDeviceVulkan::shader_create_from_bytecode (drivers\vulkan\rendering_device_vulkan.cpp:4872)

@BastiaanOlij
Copy link
Contributor

BastiaanOlij commented Nov 14, 2023

@clayjohn I'm gonne need your help with this one. I think I have a good picture of whats gone wrong and what part of the fix is, but I'm still in the dark of the root cause.

So when you first run this project cleanly with XR off, we build our shader cache, all good. We're dealing with the code starting with CopyEffects::CopyEffects, specifically initialising the variants for copy_to_fb.

There are 6 variants for this shader:

copy_modes.push_back("\n"); // COPY_TO_FB_COPY
copy_modes.push_back("\n#define MODE_PANORAMA_TO_DP\n"); // COPY_TO_FB_COPY_PANORAMA_TO_DP
copy_modes.push_back("\n#define MODE_TWO_SOURCES\n"); // COPY_TO_FB_COPY2
copy_modes.push_back("\n#define MULTIVIEW\n"); // COPY_TO_FB_MULTIVIEW
copy_modes.push_back("\n#define MULTIVIEW\n#define MODE_TWO_SOURCES\n"); // COPY_TO_FB_MULTIVIEW_WITH_DEPTH
copy_modes.push_back("\n#define MODE_SET_COLOR\n"); // COPY_TO_FB_SET_COLOR

Variants COPY_TO_FB_MULTIVIEW and COPY_TO_FB_MULTIVIEW_WITH_DEPTH are disabled so not stored.

When we turn XR shaders on, those variants are enabled. Now when we restart and reload it gets interesting.
It attempts to load all variants from cache first, when it gets to COPY_TO_FB_SET_COLOR the binsize error is triggered. The data is just 4 bytes and seems wrong.

So here I'm wondering if the two disabled variants are causing an issue in how the cache is saved?

Anyway, it goes worse from there. We're now in ShaderRD::_load_from_cache and at the end we find this code:

RID shader = RD::get_singleton()->shader_create_from_bytecode(p_version->variant_data[variant_id], p_version->variants[variant_id]);
if (shader.is_null()) {
	for (uint32_t j = 0; j < i; j++) {
		int variant_free_id = group_to_variant_map[p_group][j];
		RD::get_singleton()->free(p_version->variants[variant_free_id]);
	}
	ERR_FAIL_COND_V(shader.is_null(), false);
}

So for this variant shader_create_from_bytecode fails and returns a null RID and we go into the false.
Here it now frees all shaders loaded and compiled up till now (so including all the successfully loaded ones).

It gives two errors when trying to free the COPY_TO_FB_MULTIVIEW and COPY_TO_FB_MULTIVIEW_WITH_DEPTH seeing they weren't in the cache, not a big deal but annoying.
It then gives the first shader.is_null() error on the ERR_FAIL_COND_V, can live with that as well.

However, after this, everything snowballs. It now tries to compile these variants (as they weren't in cache) and fails. My guess here is that this is because the code changed with the new shader groups, that our code expects placeholder variants?

Anyway couple of questions to sort out here:

  • Why is it failing on COPY_TO_FB_SET_COLOR? Is this because of the disabled variants preceding it? Will test this hypothesis in a minute.
  • Why are we destroying all variants on failure, even the ones loaded correctly?
  • Should we indeed be recreating placeholder variants or doing something else here?

I'm not familiar enough with the caching code so treading carefully here :)

(haven't fully run through the scenario with XR off yet, but I'm guessing that once the cache gets messed up by the above, things just get worse)

@BastiaanOlij
Copy link
Contributor

@jonSP12 it's not a full fix but it at least solves the immediate problem. If you could test if #84883 fixes this issue for you we may be able to include it in 4.2 RC1 (no promises).

@jonSP12
Copy link
Author

jonSP12 commented Nov 14, 2023

Did it came ou in 4.2 beta 6 ?
I can't try it without downloaing it... But if you manage to remove the black screen with XR shaders turned off, should be working the same in all pc's...

@BastiaanOlij
Copy link
Contributor

Ah no, it needs to be tested before it can be merged and become part of a build. If you want, you can go to the PR, select the "Checks" tab, and you will find a dropdown on the right hand side of the page called "Artifacts". From this dropdown you can download a copy of Godot that includes this fix and test it.

@jonSP12
Copy link
Author

jonSP12 commented Nov 15, 2023

Its working, i removed the enviorement several times, and pressed the XR shader on and off, didnt got any errors.

It seems the options in rendering / Anti aliasing need the enviorement node to work, they add an extra blend / or blur ?! to the pixels when seen from affar.
The effect is still there and working, after trying XR shader ON or OFF and restart
.
env03
.
env01
.
env02

@akien-mga akien-mga added this to the 4.2 milestone Nov 15, 2023
@akien-mga
Copy link
Member

Fixed by #84883.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants