Convert final HDR output to RGBA8

[BastiaanOlij]

Note I haven't tested this in VR yet to see if it indeed has the desired effect. Hope to do this tonight.

This PR adds a new setting to the viewport that if enabled adds an extra RGBA8 buffer to our viewport which will hold the final result of our rendering. We need this mainly because OpenVR does not accept 16 bits per pixel framebuffers when rendering in VR. There may however be other uses for this.

As 3D rendering still happens in HDR buffers and it is only the final tonemapping stage that now outputs into the 8bits per pixel buffer there is no performance penalty in this approach, only a memory penalty.
2D rendering will happen directly into the RGBA8 buffer when this is enabled in most scenarios.

The setting is ignored if 3D is turned off on the viewport or if HDR is turned off on the viewport.

This should solve the banding issues we've experienced on OpenVR when turning off HDR.

[BastiaanOlij]

Wow, just tested it out after coming home. Works like a charm! Finally OpenVR works as it should with this option turned on :)

[aaronfranke]

OpenVR does not accept 16 bits per pixel framebuffers when rendering in VR.

Can you clarify, 16 bits per pixel or per channel? 16 bits per pixel is a very low color depth.

[BastiaanOlij]

@aaronfranke , Godots HDR rendering is done at 16bits per color channel, so 64bits per pixel, my bad for writing that down wrong :)

OpenVR requires an 8bits per color channel, so 32bits per pixel buffer. I don't know why this restriction is there as normally OpenGL would handle this side of things so I'm guessing they did some sort of optimization in their lens shaders that don't play nice with the higher precision color buffers.

As at the stage of output the precision is no longer as important doing a conversion right at the end as we're applying tone mapping anyway seems like a good way of doing things.

[BastiaanOlij]

Latest check-in is a small fix that prevented the preview output to screen when the main viewport was used.

[fire]

I was able to test it and it didn't break anything. The fix stopped the colour banding issues in my Sponza test map.

[BastiaanOlij]

@akien-mga this should probably be discussed soon in a PR meeting to make a final decision on it so it can either be cancelled or merged instead of lingering around.

Last time I talked to @reduz about this his main concern, and I agree to a degree, is that this is a solution for one very specific use case that does not add anything for anyone else and in a way it's thus engine bloat.

The flip side of this argument is that there are only two alternatives:

1. Valve needs to fix this issue upstream but in the past half year have been unresponsive on the issue so I don't think that is a realistic option even though it's definitely the best option: Committing GL_RGBA16F texture ValveSoftware/openvr#737
2. We "solve" this in the ARVR interface which we have done in this PR: RGBA16F HDR to RGBA8 conversion to make OpenVR happy GodotVR/godot_openvr#30
   However this PR has two major problems. The first is that it crashes when Godot is set to multi threading, I believe because we don't have access to the right OpenGL context with a GDNative plugin. And the second is that the conversion itself at this stage introduces a fairly expensive and unnecessary action. Seeing how performance critical VR is I'm not really a fan of going down this route.

[vnen]

From today's meeting:

<reduz>  for hdr to rgba8
<reduz>  i dont know
<reduz>  viewport code sucks because it needs so many buffers in so many formats
<reduz>  i dont like the idea of adding yet another one
<reduz>  i understand why it's needed though
<reduz>  let me discuss this more with mux
<reduz>  in Vulkan this will hopefully no longer be a problem because all the buffers will share memory

[BastiaanOlij]

Thanks @vnen for the update :)

[BastiaanOlij]

Just rebased this.

[akien-mga]

Based on the lack of answer on ValveSoftware/openvr#438, but also the fact that Valve devs hardly ever comment on any openvr issue nor ever merged any PR, I guess we can give up on getting any change made to OpenVR itself.

So we'll have to hack it around ourselves if we want to support their tech...

[BastiaanOlij]

@akien-mga I've rebased this in case we do decide to merge it.

[BastiaanOlij]

Just another rebase, was testing out something :)

[akien-mga]

Discussed again with @reduz, and he's still not really in favour of merging this.

16:54 <reduz> Akien: I dont want to add hack in there, given its code going away as soon as 3.1 is out
16:55 <reduz> Akien: I also still think for now could be done in the side of the VR driver

So I'll keep this PR open for VR people to cherry-pick themselves if needed, but just stating in clear that it likely won't be merged.

[BastiaanOlij]

@akien-mga , @reduz is right, it can be done on the driver side and has been done:
GodotVR/godot_openvr#30

I'm not keen on merging that either because:
A) it introduces an extra copy which is really wasteful and unwanted in an environment where performance is already maxed out
B) it crashes when you turn on multi tasking. I don't know why but I think it has to do with the OpenGL context, I could really use Reduz' help in looking at this. My Oculus driver suffers from the same issue

[BastiaanOlij]

Just read the openHMD driver also suffers this issue:
GodotVR/godot_openhmd#27

Could really use some help in properly implementing doing any further GL stuff on the GDNative side that doesn't involve crashing when multitasking is turned on. Could be as simple as a strategically placed mutex or doing more initialisation in the plugin

[hpvb]

If this code goes away in 3.1, and it's optional I think that's a good reason for adding it to 3.1 so OpenHMD and OpenVR developers can at least get going. I've marked this for 3.1 for that reason. I don't want to make that decision by myself though so @akien-mga? :)

[fire]

Note that @BastiaanOlij has implemented a workaround so it's strictly not necessary. There may be performance gains, but not required to get the feature working. GodotVR/godot_openvr#30

[BastiaanOlij]

@hpvb the issue with OpenHMD is around the multitasking issue. It's caused by Godot initializing all OpenGL related stuff as part of the render thread but the plugins creating a few shaders and such in the main thread. OpenGL does not like that very much.

I've solved that one now for both OpenVR and Oculus in a pretty ugly way by postponing the initialisation in the plugin until the first frame is rendered, still need to figure out a similar solution for OpenHMD.

So that part of it all will go away.

This RGBA16F buffer not being supported by OpenVR however remains a crappy thing. Like @fire mentioned, I have a 'fix' but its far more wasteful then what we're doing in this PR.

[aaronfranke]

What is the chance of OpenVR adding support for RGBA16F? Wouldn't that be better?

[BastiaanOlij]

@aaronfranke yes it would be, I wager there is about a 1% chance of them doing so. There has been no response whatsoever on the issue that was raised.

[BastiaanOlij]

I'm closing this PR. Just pretty much finished #27527 which covers a broad spectrum of requirements for VR SDKs and having the SDK supply the texture into which we render. This should allow us to solve the HDR issue has well.