OpenXR: Fix ViewportTextures not displaying correct texture (RendererRD)

[BastiaanOlij]

This PR fixes the issue that ViewportTexture wasn't properly being updated when our render target override function was used and thus preventing the correct texture from being displayed.

This has been a regression from the first Godot 4.0 version as this was working correctly in Godot 3.

While a feature that is only used in PCVR, it has been often requested to fix this.

For compatibility the fix was as easy as updating the proxy texture for our viewport, the same approach used in Godot 3.

For our Vulkan backend for some reason we decided early on to work directly on the rendering device. Our renderer thus had no idea what to do with these textures.

Caution

This is a serious breaking change that we need to discuss further in the XR team. There are likely additional changes required.
We may need to investigate alternatives that are less invasive.
As such I've marked this as a milestone for 4.6

Warning

The same issue exists for DX12 and MacOS, the same fix must be applied to these before we can merge this!

Fixes: #100004

[BastiaanOlij]

Note, one option here is to split out the fix for OpenGL/Compatibility in a separate PR. That would be a nice and easy fix for Godot 4.5.

[dsnopek]

Is the breaking change that OpenXRInterface::get_color_texture() will now return a rendering server texture RID, rather than a rendering device texture RID?

If so, I actually think that makes more sense overall - it's always seemed a little weird to me that for OpenGL we return a rendering server texture RID, but not for Vulkan.

We'd also need to update the OpenXRD3D12Extension, right? (UPDATE: Ah, I see you noted that in the description!)

I think we may be able to prevent breaking binary compatibility for XRInterfaceExtension by having a virtual compatibility method that takes the rendering device texture and wraps it up in a rendering server texture? That would need some experimentation to see if it would work. (Not that there's many XRInterfaceExtensions in the wild - the main one being the OpenVR extension.)

[dsnopek]

I think color_rd (or rd_color?) would be a clearer name:

Suggested change

	RID texture;
	RID color_rd;

[BastiaanOlij]

I'm tempted to do this just to make things clearer, however the names mirror the names of the internal texture RIDs if you're not overriding so a name change may add confusion. Not sure

[BastiaanOlij]

Is the breaking change that OpenXRInterface::get_color_texture() will now return a rendering server texture RID, rather than a rendering device texture RID?

If so, I actually think that makes more sense overall - it's always seemed a little weird to me that for OpenGL we return a rendering server texture RID, but not for Vulkan.

Yes, in hindsight it should have always worked this way. But changing it now, especially as we have some extensions doing things with swapchains, could cause issues.

It's possible the way I've done things hides the changes well enough that it will keep on working just fine (though there may be a few places I've missed).

The other part I'm not sure of is that this creates extra texture views and I don't know if that has any costs.

[BastiaanOlij]

I think we may be able to prevent breaking binary compatibility for XRInterfaceExtension by having a virtual compatibility method that takes the rendering device texture and wraps it up in a rendering server texture? That would need some experimentation to see if it would work. (Not that there's many XRInterfaceExtensions in the wild - the main one being the OpenVR extension.)

Things is, it's always just been RIDs we're pushing around so most of the extensions are none the wiser. Yes we now need to sent through texture RIDs instead of texture_rd RIDs, but as we now expose the texture RIDs through our swapchain wrapper class, that part should be transparent.

This is why it wasn't a problem that compatibility already used texture RIDs without causing any problems.

However, if extensions perform additional actions here that assume we have a texture_rd RID that things fall a part. Or if I've forgotten some place that is still expecting texture_rd RIDs.

[BastiaanOlij]

Btw @dsnopek , I'm probably going to split this PR in two, one purely for the changes to compatibility (seeing they are straight forward) and then changing this one to only handle the RendererRD changes.

[BastiaanOlij]

Extracted changes for compatibility renderer into #110002

[BastiaanOlij]

I added the functionality in for Metal and DX12 but this is as yet mostly untested so not ready for merging yet.

@mbucchia I ran into a weird error on DX12 with this when we're attempting to create a slice on the textures. Do you mind having a quick test and see if I did something dumb or if we're missing something?

[dsnopek]

I think we may be able to prevent breaking binary compatibility for XRInterfaceExtension by having a virtual compatibility method that takes the rendering device texture and wraps it up in a rendering server texture? That would need some experimentation to see if it would work. (Not that there's many XRInterfaceExtensions in the wild - the main one being the OpenVR extension.)

Things is, it's always just been RIDs we're pushing around so most of the extensions are none the wiser. Yes we now need to sent through texture RIDs instead of texture_rd RIDs, but as we now expose the texture RIDs through our swapchain wrapper class, that part should be transparent.

Actually, my idea with using a compatibility method won't work, because the method hash won't change. But I still think we could potentially come up with a way to minimize the compatibility breakage.

Something like (untested):

RID XRInterfaceExtension::get_color_texture() {
	RID texture;
	GDVIRTUAL_CALL(_get_color_texture, texture);

#ifndef DISABLE_DEPRECATED
	RenderingDevice *rd = RenderingDevice::get_singleton();
	if (texture.is_valid()  && rd) {
		// @todo We should cache the RS texture that is made, and not remake it if we get the same RD texture	
		if (rd->texture_is_valid(texture)) {
			RID texture_rd = texture;
			texture = texture_storage->texture_allocate();
			texture_storage->texture_rd_initialize(texture, texture_rd);
		}
	}
#endif
	return texture;
}

But, yeah, it's certainly possible that an extension is doing something advanced that could break. However, if we can test all the GDExtensions we're aware of, and it works, we'll probably have covered 99% of possible use cases.

FYI, I took a quick peek at godot_openvr, godot_arcore and TiltFiveGodot4. I don't think godot_openvr or godot_arcore will be affected at all - they appear not to be using the texture override stuff, but the blits returned by _post_draw_viewport(). TiltFiveGodot4 does use the texture overrides, but I suspect it would be OK - would need some testing.

[mbucchia]

@mbucchia I ran into a weird error on DX12 with this when we're attempting to create a slice on the textures. Do you mind having a quick test and see if I did something dumb or if we're missing something?

Any specific repro steps? Just the xr template?
Edit: Do I use the "spectator demo" on the linked issue?

[BastiaanOlij]

@dsnopek the problem is that we don't have a proper lifecycle for the created texture object. It should be freed when the RD texture is freed but... That said, I like the general idea of your fix, checking if the RID is RD texture RID and if so, wrapping it in a texture RD object.. Yeah...

@mbucchia yeah just test with the spectator demo: godotengine/godot-demo-projects#1144
I'm going to give it another try myself, the more I think about it, the more convinced I am I just made a mistake somewhere.

[BastiaanOlij]

@mbucchia I've chased the value to this point in the code: https://github.com/godotengine/godot/blob/master/drivers/d3d12/rendering_device_driver_d3d12.cpp#L1491

RDD::TextureID RenderingDeviceDriverD3D12::_texture_create_shared_from_slice(TextureID p_original_texture, const TextureView &p_view, TextureSliceType p_slice_type, uint32_t p_layer, uint32_t p_layers, uint32_t p_mipmap, uint32_t p_mipmaps) {
	TextureInfo *owner_tex_info = (TextureInfo *)p_original_texture.id;
#ifdef DEBUG_ENABLED
	ERR_FAIL_COND_V(!owner_tex_info->owner_info.allocation, TextureID());
#endif

I think because this texture is created within OpenXR and we use texture_create_from_extension to then create our texture structure around it, we don't have information about it's creation.
This is a little beyond me to know whether we can solve this differently and make _texture_create_shared_from_slice work with a texture that's been created from outside our sphere of influence.
I'm actually surprised we haven't run into this yet because we create a number of slices on this texture. There must be something different in what we're adding here compared to what we're doing elsewhere when rendering a frame.