Forbid passing multiview sampler to the custom function in shaders

[Chaosus]

My attempt to fix #72290, if @BastiaanOlij has another idea of how to fix it that would be cool, and I will close this PR.

[clayjohn]

My feeling is that this might be necessary until we figure out a better solution. It would be really nice to automatically detect XR and non-XR usage, but I have a feeling that it will be too complex for now

[BastiaanOlij]

This does feel like a very ugly hack around the problem. One thing we could do is to check actions.check_multiview_samplers and only apply this restriction if that is true, and improve the code where we set actions.check_multiview_samplers to true to only do this if multiview shaders are actually used (right now a project setting).

Also could we change the check that if we detect a function being called using a parameter we've deemed needing multiview support, that we tell the user to create a sampler2Darray version of the function being called, if it doesn't exist?

[BastiaanOlij]

My feeling is that this might be necessary until we figure out a better solution. It would be really nice to automatically detect XR and non-XR usage, but I have a feeling that it will be too complex for now

Keep in mind that in XR we're always dealing with a mixed environment. Plus someone adding a function like the OP did, that function might be used for both normal textures and for textures that require multiview access in the same shader.

[Chaosus]

It's not a hack - I would call it a plug until we found a way to handle it properly.

[BastiaanOlij]

Just to repeat this here as it should be part of this fix, as i mentioned in the issue, we can make this smarter so that our restriction only applies if multiview is enabled. This requires a few small changes:

1. We need to merge the fix Clay did months ago but which was closed because we wanted a better implementation that never eventuated and probably won't until much much later: Disable multiview shader versions when xr is disabled #63829

2. We need to change the code in GLES3::MaterialStorage, SceneShaderForwardClustered and SceneShaderForwardMobile from actions.check_multiview_samplers = true; to actions.check_multiview_samplers = RendererCompositorRD::singleton->is_xr_enabled();

3. We need to check actions.check_multiview_samplers in this fix and only apply this fix if it is true

With these changes our restriction will only be needed, and only apply, if we're compiling multiview shaders.

[YuriSizov]

@clayjohn @BastiaanOlij You seem to have agreed in the issue that this is a good short-term solution (on top of Clay's own old PR, which has since been merged). What changes, if any, are expected from @Chaosus now? Or should we bump it?

[clayjohn]

@Chaosus I agree with Bastiaan's proposed improvements to this PR. Together they will make this limitation only impact users who are working on XR.

For reference, these are the two changes needed:

We need to change the code in GLES3::MaterialStorage, SceneShaderForwardClustered and SceneShaderForwardMobile from actions.check_multiview_samplers = true; to actions.check_multiview_samplers = RendererCompositorRD::singleton->is_xr_enabled();

We need to check actions.check_multiview_samplers in this fix and only apply this fix if it is true

[BastiaanOlij]

Sounds like clay covered it. If we can make those small changes, this should be ready to go.

[Chaosus]

@BastiaanOlij, @clayjohn OK, I think I've done it, please recheck.

[BastiaanOlij]

Shader changes look good. Just a little nitpick on the singleton change, though this is my personal opinion on the subject, I don't know what the official stance on this is but I've always added separate singleton members to subclasses to ensure we're accessing a pointer that will remain null when a different subclass is instantiated.

[Chaosus]

@BastiaanOlij OK, I made some changes, check again.

[BastiaanOlij]

Seeing get_singleton is now on the base class we could replace RendererCompositorRD::get_singleton() and RendererCompositorGLES3::get_singleton() with RendererCompositor::get_singleton(). But I feel I'm really taking nitpicking to the next level now :)

[BastiaanOlij]

Weird, I thought this PR would only have impact on the 3D renderer, not on the 2D renderer, but I could be wrong

[akien-mga]

I'll try to do more testing, it might have been an unrelated bug present in master too.

[akien-mga]

Yeah my segfault was unrelated to this PR, sorry for the false alarm. It happens in the master branch too and is related to a simple plugin I got from an unrelated MRP.

So this should be good to go. But I already started the build for 4.0 RC 3, so please don't merge for now, we'll see in the production team how to time this.

[akien-mga]

Thanks!