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

Allow imgui to render into a multisampled Vulkan framebuffer #2706

Closed
wants to merge 3 commits into from

Conversation

vilya
Copy link
Contributor

@vilya vilya commented Jul 31, 2019

ImGui always sets the MSAA level to 1 sample, disabling it. This means you get errors if you try to render with ImGui's into a multisampled framebuffer in Vulkan, because Vulkan requires you to specify the correct multisampling level at pipeline creation time.

This change adds a new member to the ImGui_ImplVulkan_InitInfo struct which users of ImGui can set to the correct multisample level for their target frame buffer. The zero value (which you'll get by default if the struct has been zero-initialised) is not a valid MSAA level in Vulkan, so we detect that case and use VK_SAMPLE_COUNT_1_BIT instead at setup time. Unless you've explicitly set the MSAASamples to something, the default behaviour should be exactly as it was previously, i.e. multisampling will be disabled.

This fixes issue #2705

Feedback very much appreciated!

vilya added 2 commits July 31, 2019 19:50
ImGui always sets the MSAA level to 1 sample, disabling it. This means you get errors if you try to render with ImGui's into a multisampled framebuffer in Vulkan, because  Vulkan requires you to specify the correct multisampling level at pipeline creation time.

This change adds a new member to the `ImGui_ImplVulkan_InitInfo` struct which users of ImGui can set to the correct multisample level for their target frame buffer. The zero value (which you'll get by default if the struct has been zero-initialised) is not a valid MSAA level in Vulkan, so we detect that case and use `VK_SAMPLE_COUNT_1_BIT` instead at setup time. Unless you've explicitly set the MSAASamples to something, the default behaviour should be exactly as it was previously, i.e. multisampling will be disabled.
}
else {
ms_info.rasterizationSamples = VK_SAMPLE_COUNT_1_BIT;
}
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks!
Might want to remove those braces, update local changelog at the top of imgui_impl_vulkan.cpp and docs/Changelog.
I'll do those changes if you need.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, I'll remove those braces. I didn't notice that it wasn't in line with ImGui style. Sorry!

Would text like this be ok for the changelog in imgui_impl_vulkan.cpp?

//  2019-07-31: Vulkan: Added support for specifying multisample count. Set ImGui_ImplVulkan_InitInfo::MSAASamples to one of the VkSampleCountFlagBits values to use, default is non-multisampled as before.

And would it be the same text, in the version 1.73 section, for docs/Changelog?

Copy link
Owner

Choose a reason for hiding this comment

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

Yes that's good, thanks!

Copy link
Owner

Choose a reason for hiding this comment

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

(I'm actually doing a bit of a rollback and releasing a hotfix 1.72b to fix a navigation issue now, so your patch may be in it or not.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK great, I've added those changes to the PR. Thanks!

- Add changelog comments to both imgui_impl_vulkan.cpp and docs/CHANGELOG.txt describing the multisample change.
- Remove unnecessary braces around single line if & else statements, to match ImGui coding style.
@ocornut
Copy link
Owner

ocornut commented Aug 1, 2019

Merged, thank you!

@ocornut ocornut closed this Aug 1, 2019
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.

2 participants