Skip to content
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

Disable multiview shader versions when xr is disabled #63829

Merged
merged 1 commit into from
Feb 1, 2023

Conversation

clayjohn
Copy link
Member

@clayjohn clayjohn commented Aug 2, 2022

Partially addresses: #63217

This change disables colour passes that include Multiview when XR is disabled. This avoids compiling multiview variants of shaders when not using XR. Accordingly, non-XR users will not have shader compilation failures because their shader fails to compile for XR.

This PR does not fix- #63217 As the same problem will still arise for XR-users. When XR is enabled, the shader will still crash when using the NORMAL_ROUGHNESS_TEXTURE

@clayjohn clayjohn requested a review from a team as a code owner August 2, 2022 14:01
@fire
Copy link
Member

fire commented Aug 2, 2022

This breaks the usecase of multiview for looking glass displays, where in theory it can be accelerated for its 30ish multiviews.

@clayjohn
Copy link
Member Author

clayjohn commented Aug 2, 2022

This breaks the usecase of multiview for looking glass displays, where in theory it can be accelerated for its 30ish multiviews.

I'm not sure what you mean. Already we only support Multiview when XR is enabled, this just partially completes that by avoiding compiling shaders that will never be used.

@lyuma
Copy link
Contributor

lyuma commented Aug 2, 2022

@fire I think that usecase can be addressed by having XR enabled in the renderer, but not necessarily using OpenXR. Note that other parts of godot already assume multiview is only used if XR is on.

@reduz
Copy link
Member

reduz commented Aug 21, 2022

I want to do this a bit different, lets talk about it during the week.

@clayjohn
Copy link
Member Author

Closing as we will be going with a different (more complex, but much better) approach.

@clayjohn clayjohn closed this Oct 31, 2022
@clayjohn clayjohn removed this from the 4.0 milestone Oct 31, 2022
@BastiaanOlij
Copy link
Contributor

I'd like to re-open this PR.

Seeing we originally implemented this solution from day 1 when multiview was introduced, it got broken at some point when we added all these new modes, and now we decided not to fix this months ago because of a change that Reduz suggests (a good change, don't get me wrong) that probably won't see the light of day until many months from now, possibly not until we get to it in 4.1 or 4.2, we have a problem with users now running into problems in 4.0.

Else I suggest we remove the logic that disables multiview shaders all together, if we've broken it and don't intent to fix it, then lets remove it so people who are doing XR don't keep falling into the pitfall of having to enable multiview shaders.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed in the rendering channel, we agreed to merge this for now in 4.0.

@akien-mga akien-mga merged commit 7990b76 into godotengine:master Feb 1, 2023
@akien-mga
Copy link
Member

Thanks!

@clayjohn clayjohn deleted the VULKAN-disable-multiview branch July 18, 2023 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants