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

Fix OpenXR sample count #84099

Merged
merged 1 commit into from Nov 8, 2023

Conversation

BastiaanOlij
Copy link
Contributor

In OpenXR we were creating our swapchain with the suggested sample count. This however does not work as expected as Godot will render 3D content into separate buffers, which in Vulkan can be MSAA buffers, and then blit the result into our swapchain image which is expecting a buffer with sample count = 1.

While most XR runtimes suggest a sample count of 1 and this does not lead to issues, we had reports on some hardware where this was causing issues.

For the future it would be worth investigating if we can make this a setting. On the compatibility renderer, where we do not render into separate buffers, setting the sample count to the MSAA level we want will save us a pass compared to Godots build in MSAA solution (which we are adding to the compatibility renderer). This we should handle separately to this PR however and will probably be considered new functionality for Godot 4.3.

@BastiaanOlij BastiaanOlij added this to the 4.2 milestone Oct 28, 2023
@BastiaanOlij BastiaanOlij self-assigned this Oct 28, 2023
@m4gr3d
Copy link
Contributor

m4gr3d commented Oct 28, 2023

While most XR runtimes suggest a sample count of 1 and this does not lead to issues, we had reports on some hardware where this was causing issues.

By this do you mean some runtimes were suggesting a sample count > 1 and that was leading to issues?

On the compatibility renderer, where we do not render into separate buffers, setting the sample count to the MSAA level we want will save us a pass compared to Godots build in MSAA solution (which we are adding to the compatibility renderer).

So this change would introduce a regression for the compatibility renderer?

@BastiaanOlij
Copy link
Contributor Author

BastiaanOlij commented Oct 29, 2023

By this do you mean some runtimes were suggesting a sample count > 1 and that was leading to issues?

Yes, HTC reported a problem with this, they seem to suggest a sample count of 2. Makes sense for a scratch build game where you render just to the buffer generated and want to adhere to the suggested size, but less so to more complex renderers like Godot uses.

So this change would introduce a regression for the compatibility renderer?

Potentially, but it is not expected behaviour to have MSAA work when MSAA isn't enabled. I have been experimenting with making this an option however that would mean a new feature, and that would mean this can't be fixed until 4.3.
Right now we have a broken renderer in 4.2 if sample count > 1 and we're on the Vulkan renderer.

(Also it's likely that enabling this feature will break various effects that assume we are rendering to a normal buffer).

@YuriSizov
Copy link
Contributor

So, do we have a consensus that this is an overall good (at least for now)? 🙃

@BastiaanOlij
Copy link
Contributor Author

So, do we have a consensus that this is an overall good (at least for now)? 🙃

@m4gr3d could you let us know what you think of my reaction, then hopefully we can merge this :)

@m4gr3d
Copy link
Contributor

m4gr3d commented Nov 7, 2023

So, do we have a consensus that this is an overall good (at least for now)? 🙃

@m4gr3d could you let us know what you think of my reaction, then hopefully we can merge this :)

Sorry for the delay; the rational sounds good so I'm okay to proceed with the current approach.

I have been experimenting with making this an option however that would mean a new feature, and that would mean this can't be fixed until 4.3.

@BastiaanOlij Can you add this to the XR Roadmap so we can track it for Godot 4.3.

@akien-mga akien-mga merged commit 84e16de into godotengine:master Nov 8, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@BastiaanOlij BastiaanOlij deleted the fix_openxr_samplecount branch November 19, 2023 23:41
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.

None yet

4 participants