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

Added SDL3 Vulkan Example with docking + fixed a multi-viewport bug with SDL3 #8085

Closed

Conversation

SuperRonan
Copy link

Wrote on top of #8084 and #8061 (although it is independant of #8061).

About the bug:
When creating a secondary viewport's SDL_Window, it inherits some flags from the main window.
But not all flags should be inherited (not the fullscreen one in particular).
In the SDL2 impl, it used to only inherit the HighDPI flag, so I changed the SDL3 impl so that it only inherits the HighDPI flag.

I noticed the problem in my engine when I migrated from SDL2 to SDL3 recently. With my main window in borderless fullscreen, and the secondary ImGui viewports on my second monitor, it would crash when creating a new ImGui window (like when openning a combo or a color picker). The new secondary viewport would attempt to open fullscreen, conflicting with the main window. It ended up messing with the main window's VkSurface (VkSwapchain out of date and maximum extent of the surface to 0).

…ction in the fragment shader

API changes:
- Added ImGui_ImplVulkan_ReCreateMainPipeline(...) to explicitly re-create the main window pipeline (when some of its properties are changed).
- ImGui_ImplVulkan_CreateMainPipeline(...) does not implicitly use ImGui_ImplVulkan_InitInfo::PipelineRenderingCreateInfo, but a function parameter.
- The main window pipeline is created only if possible during ImGui_ImplVulkan_Init(...) (if a render pass or rendering info are given), else it should be created with ImGui_ImplVulkan_ReCreateMainPipeline(...) before rendering)
Concerning color correction:
- The default behavior of imgui_impl_vulkan does not change (no color correction is applied).
- A color correction method is decided at pipeline compile time (through a vulkan specialization constant)
- Color correction parameters can either be set at pipeline compile time (static mode), or through push_constant (dynamic mode, default)
- A gamma correction mode is implemented (and an extra alpha gamma correction).
- New color modes can easily be added.
Concerning multi view ports:
- Secondary viewports surface format and present mode can now be controlled by the application view ImGui_ImplVulkan_RequestSecondaryViewportsChanges(...)
- Secondary viewports can also use color correction if they use the application requested format.
- Each viewport now has its own VkRenderPass and VkPipeline (cached in the backed data). This fits to the fact that each viewport might have a different format (the previous code was making this (quite reasonable) assumption).
- Recycle some viewport resource on swapchain recreation if possible (CommandPool, CommandBuffer, Fence, Semaphores) rather than recreating them every time.
@SuperRonan
Copy link
Author

I made a branch on my fork with only the commit that fixes the SDL3 bug, maybe I should do a PR of it separately?

@SuperRonan
Copy link
Author

I made a standalone PR for fix: #8098

@ocornut
Copy link
Owner

ocornut commented Oct 24, 2024

Thanks Ronan for those PR.

Please see my comment #8041 (comment)

I am unlikely to be able to look at any PR with multiple unrelated changes, it's a little too difficult for Vulkan backend. The current state of this is unfortunately "100% won't merge". If I cannot review, I cannot merge :( But you can use interface rebase to squash and recreate suitable history.

  • unrelated features should be in separate PR unless there is a strong justification/link between them.
  • every commit should be standalone and well justified. I won't be able to review a series of intermediary commits with names such as "not working yet", "fixed compilation warnings".
  • the dual master/docking thing makes thing more difficult which is unfortunately. I think it'll be easier both for you and me to open a single PR for docking (close the master one), but with commit history intentionally crafted so that some commits are expected to be picked in master (minor obvious conflicts I can fix), and some only in docking after picking the earlier ones. This way we discuss/update in a single location.

Thank you!

@ocornut
Copy link
Owner

ocornut commented Oct 24, 2024

I have pushed a SDL3+Vulkan example ccb6646 which I've recreated by copying example_sdl2_vulkan then manually looking at the diff between example_sdl2_opengl3 and example_sdl3_opengl3, and applying a few other fixes. I'm merging this in docking too.

This likely to unfortunately conflicts with your two similar commits.

I suggest that you:

  • close the master versions of PR to keep the docking ones.
  • rebase them over latest docking (3 commits should be removed)
  • then you can look at what's left and clean up or split the commits accordingly.

@SuperRonan
Copy link
Author

It looks like you integrated the changes in 646df39, so I can close this PR.

@SuperRonan SuperRonan closed this Oct 24, 2024
@ocornut
Copy link
Owner

ocornut commented Oct 29, 2024

It looks like you integrated the changes in 646df39, so I can close this PR.

I honestly don't know if everything from the PR was integrated since it had many things.

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