Workaround crash due to null shader when running XR project with --xr-mode off

[lyuma]

Steps to reproduce:

1. use official openxr test project
2. make sure OpenXR is enabled in project setting; and enable OpenXR Shaders.
3. In Project Settings, enable Advanced Settings, and go to the Run category under Editor, not the Run category under Application. In the text box, type --xr-mode off
4. Press run.
5. Without this patch, it will crash with Nasal Demons and/or Undefined Behavior due to dereference of null pointer. On windows MSVC, this undefined behavior is compiled as an illegal instruction (flow off the end of a function into FF / ??? instructions), but other possibilities may also happen.

This is not a complete fix, since it still prints error due to null shader RID, but at least it won't crash. I was able to play the game with --xr-mode off.
Other people also hit this issue in 4.2 dev

[BastiaanOlij]

Need to have a deeper dive into this when I can, this just fixes the symptom, we need to look into why it can't create the shader, or why it is attempting to create the shader if it shouldn't.

[RandomShaper]

May it be that the placeholder is non-null but dangling? In that case, the function could either explicitly check for validity (pre) or, well, fail on null (post) as this PR does.

[BastiaanOlij]

Interesting point @RandomShaper , indeed the usage of get_or_null here suggest that it's expected the shader may not (yet) exist, but the code continuing as if it exists is then a problem.

I don't know a lot about the intended implementation of placeholder shaders, but if it is intended that this could be NULL, instead of erroring out we should probably skip over the code that uses the shader.

[RandomShaper]

I've delved a bit more and I think it should indeed be an error on the side of the caller. This function can be called either to create a shader from scratch or to fill a soul-less (placeholder) shader previously instantiated. In the former case, I don't think shader can be null ever. In the latter case, if the caller messed up enough to provide a dangling RID, that's indeed its fault. So:

• It would be a bit more expressive to check the error only for the case of placeholder (or simply add a little comment about why it may be null).
• This patch is fine, but the root issue (why the engine does an invalid call in the first place) is still to be investigated, isn't it?

[BastiaanOlij]

@RandomShaper agreed, I think it's valid to record the error when the shader is null as it can but shouldn't happen. Then we can chase down the real cause and fix that as well in a separate PR.

[BastiaanOlij]

Approving this as per insight/discussion with @RandomShaper (see above).

This change makes sense to make, shader can but shouldn't be null, thus the ERR_FAIL_NULL_V makes sense.

[akien-mga]

Thanks!