-
-
Notifications
You must be signed in to change notification settings - Fork 21.5k
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
Add VIEW_INDEX variable in shader so we know which eye/view we're rendering for #48011
Add VIEW_INDEX variable in shader so we know which eye/view we're rendering for #48011
Conversation
// make sure this struct is padded to be a multiple of 16 bytes for webgl | ||
float pad[2]; | ||
float pad[1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually not sure this ends up being a multiple of 16 even before this change. I might be counting wrong but possibly other variables have been added since this was originally set to 2
This will likely be useful to implement godotengine/godot-proposals#437, although we'll want to do this in core since full-screen post-processing shaders have many limitations right now (at least in |
I wonder if we should add some defines to make this a little easier like:
|
Maybe it should be bitwise? Also, #define EYE_NONE 0 // Optional, we can exclude this.
#define EYE_LEFT 1
#define EYE_RIGHT 2
#define EYE_BOTH 3 |
Maybe for future compatibility, but as of now I don't think GPUs support bitwise integer operations in GLES. |
the options are:
There is no both, you are either rendering for the left eye, or right eye, or you're not rendering stereoscopic. So they aren't bits. Happy to add them as defines edit owh and these values have been like this for a long time and are used throughout the system so changing them is a huge breaking change. |
Adding the defines doesn't make them available to Godot shader compiler so they can't be used in code. I've tried implementing this by adding edit never mind, found what I was missing... |
Should we then just use |
I we don't mind loosing the distinction between |
What if the view index was calculated from EYE with |
3e04060
to
0803346
Compare
The problem is that once we go to Godot 4 it actually reflects the layer we're rendering into, it's actually a build in variable in Vulkan, layer 0 for Mono/left eye, layer 1 for right eye, so we would need a separate variable to indicate if we're rendering mono/stereo. In that respect it does make sense to just already bite that bullet and accept that limitation so people get used to Obviously you as a developer know whether you are dealing with stereo scopic or a mono view so if you do need to make that difference you can always just add a uniform. |
I've added a new commit that renames it from EYE to VIEW_INDEX so that this will work comparable to how this will likely end up working in Godot 4. The example shader now looks like this:
We can't tell mono and stereo apart anymore but we're going down that path anyway. If we're happy with this direction I will squash the changes. |
A small extra point of discussion (that very probably doesn't require any changes) - if EYE is not used in the shader, there is no need to send the uniform. Although a lot of uniforms are sent, I don't know if we should have any expectation that they should be cheap, especially not compared to checking a bool on the CPU, and especially on GLES2. There's already an existing mechanism for detecting whether BUILTINs and such are used in the shader, this could potentially be used to store whether this (or several in flags) uniforms are used and only send them in that case. This could be adding up if there are several unused uniforms (e.g. TIME is similar). EYE is just a very obvious case of this - presumably 95%+ of people won't be using stereo rendering. Although it is equally relevant for the other uniforms. Having said that this is more of an issue in the canvas shaders and perhaps something we should look at there, because there are likely to be far more drawcalls there than in 3d. It is probably academic in 3D unless you are using a boatload of drawcalls. It would be an interesting thing to test on desktop and mobile - certainly it wouldn't be worth changing unless it did cause an impact in terms of performance or hardware with limited number of uniforms. |
@lawnjelly totally agree with that, my only remark would be that it may be worth doing a separate PR for that change to encompass other variables too. |
f8be39e
to
8f8c9c2
Compare
Squashed and rebased this, I think this is fine as it is. |
Thanks! |
My question may have a simple solution, but I am unable to figure it out. @BastiaanOlij, could you please help? |
Indeed, OpenXR only shows one eye in preview, the side by side view you see in the screen shot only shows when using the native mobile interface. If you're using SteamVR, there is an option in the steamvr menu to show the output to the headset and that shows both eyes. |
Thank you! |
This was requested a long long time ago, finally got around to adding it for both GLES2 and GLES3.
I will be doing a different implementation with the same end result for Godot 4/Vulkan in the very near future.
This PR introduces a new shader variable called
VIEW_INDEX
that will be :Ergo:
In normal:
In stereo:
Just changing colors like this doesn't look too spectacular but there are various use cases from showing different textures in each eye to create a depth effect or for visual aid applications where a difference in what each eye sees is created on purpose
note For Godot 4 this will tie in with stereoscopic rendering through multiview, current work in progress PR: stereoscopic proposal