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

Make Vulkan implementation distinguish the user's render pass to its own created render pass in un-docked windows. #3459

Closed
wants to merge 4 commits into from

Conversation

FunMiles
Copy link
Contributor

@FunMiles FunMiles commented Sep 6, 2020

When a new window is created for a GUI component going out of the current window, a fresh render-pass is created that is not likely to be compatible with the user's render pass used when doing the GUI rendering in her/his window.
This PR addresses this issue by associating a pipeline with the created window that is compatible with the new render-pass.

Two more minor commits due to newer versions of Vulkan on Mac not liking the old validation layer. Also the CMake finding/linking with Vulkan is slightly updated to the more modern way.

@FunMiles FunMiles changed the title Docking Make Vulkan implementation distinguish the user's render pass to its own created render pass in un-docked windows. Sep 6, 2020
ocornut pushed a commit that referenced this pull request Sep 7, 2020
@ocornut
Copy link
Owner

ocornut commented Sep 7, 2020

Thank you @FunMiles for the PR!
FYI I have merged the first two commits (the minor ones) into master and made the equivalent changes to the SDL+Vulkan example (see b25756b), so if you ever have to rebase to may remove those two commits.
Looking at the big one now.

@ocornut
Copy link
Owner

ocornut commented Sep 7, 2020

Hello,

As pushed this PR seems to crashes both Vulkan examples, as main.cpp calls SetupVulkanWindow() which via ImGui_ImplVulkan_MakeShaderModules() now relies on the InitInfo which haven't been submitted yet.

(I would also recommend keeping the ImGui_ImplVulkan_InitInfo parameter of all those functions as a pointer as it is currently to minimize noise in the diff.)

Thank you!

@FunMiles
Copy link
Contributor Author

FunMiles commented Sep 7, 2020

Hello,

As pushed this PR seems to crashes both Vulkan examples, as main.cpp calls SetupVulkanWindow() which via ImGui_ImplVulkan_MakeShaderModules() now relies on the InitInfo which haven't been submitted yet.

(I would also recommend keeping the ImGui_ImplVulkan_InitInfo parameter of all those functions as a pointer as it is currently to minimize noise in the diff.)

Thank you!

Sorry about this. It's rooted in my state of confusion last night. I wasn't sure if a line was supposed to be there or not and did a force push. However it seems a bit of a conceptual twist. The example is calling ImGui_ routines that use Vulkan calls before it sets up the ImGui_ImplVulkan_InitInfo which is where the Vulkan Instance is set. I am going to review the logic of this ordering. In my own code, I call ImGui_ImplVulkan_Init upstream, thus there is no issue.

@ocornut
Copy link
Owner

ocornut commented Sep 7, 2020

Btw I'm not too fussy with small breakage in the Vulkan back-ends, people who use Vulkan tends to be quite knowledgable and flexible with those, as long as they are documented (e.g. nice asserts).

@FunMiles
Copy link
Contributor Author

FunMiles commented Sep 7, 2020

I'd rather avoid doing a breakage because of the sdl_vulkan code. I don't know how I can test it.

On the other hand, I am now hitting some issues related global variables. (global RenderPass, PipelineLayout etc...)
Is there going to be a move for Dear ImGui away from globals ? 😄 It's a subject that is also present in trying to have different state for different windows... 😉

@FunMiles
Copy link
Contributor Author

FunMiles commented Sep 7, 2020

I have pushed one more commit that restores the working demo. Hopefully it should not break any other code. It is more robust and allows to keep the call to ImGui_ImplVulkan_InitInfo either early or after creating the window. I've tested it with my code, where I create a full Vulkan state myself and pass it to ImGui_ImplVulkan_InitInfo before other calls. So hopefully it keeps working for this issue: #3455

ocornut pushed a commit that referenced this pull request Sep 8, 2020
…dows from the one created with the user's render-pass. (#3455, #3459)

This is mostly for the benefit of multi-viewports.
ocornut added a commit that referenced this pull request Sep 8, 2020
@ocornut
Copy link
Owner

ocornut commented Sep 8, 2020

Thank you @FunMiles.
The issue is that imgui_impl_vulkan contains code that are both helper to the examples and used by the multi-viewports system, and for the later they are not supposed to depend on the back-end initialization, hence the spaghetti you want through.

I have now merged this with various changes (actually replaced some of your added static with globals, made the changes work in master and then merged in docking). I agree we should move away from globals in eventually. We can probably start by gathering everything in a single struct, one bit of annoyance ihmo was lack of "default value in member variable declaration" which is part of C++11 but we're moving to C++11 this year (might make it one target for 1.80). The refactor of back-ends with perhaps a more explicit context is a separate topic we could tackle more globally.

@ocornut ocornut closed this Sep 8, 2020
ocornut added a commit that referenced this pull request Sep 8, 2020
…ssary pipeline creation for main viewport. Amend 41e2aa2. (#3459)
ocornut added a commit that referenced this pull request Sep 8, 2020
…ssary pipeline creation for main viewport. Amend 41e2aa2. (#3459) + Add ImGui_ImplVulkanH_CreateWindowSwapChain in ImGui_ImplVulkanH_CreateOrResizeWindow().
@ocornut
Copy link
Owner

ocornut commented Sep 8, 2020

Having add another look at this, it seems that there was dead code (unused shader modules), some leaks, and an extraneous pipeline was created for the main window in the example. I fixed those with 770c995

EDIT I broke things with this. See #6325 (comment)

dtrajko added a commit to dtrajko/MoravaEngine that referenced this pull request May 26, 2021
@FunMiles
Copy link
Contributor Author

FunMiles commented Jun 8, 2021

Having add another look at this, it seems that there was dead code (unused shader modules), some leaks, and an extraneous pipeline was created for the main window in the example. I fixed those with 770c995

I had not used this last commit in my running code. However, having decided to upgrade the Vulkan version on my Mac, I was hit with several issues that made me come back. Not creating the pipeline each time caused me issues. So I want to revisit this in a clean way. The pipeline can be lazily created. However, the created pipeline can be shared with other windows. Is there an aversion to using shared_ptr ? Maybe this is a more general question linked with the lack of RAII in the code. If using a shared_ptr, the destructor should take care of destroying the pipeline. And, because of Vulkan's order of destruction requirements, it seems some other mechanism has to be implemented if some other objects on which the pipeline depends need to be destroyed earlier.

ocornut added a commit that referenced this pull request Apr 30, 2024
…indow::Pipeline (#6325, #6305, #7398, #3459, #3253, #3522)

As this is currently unused and misleading. Next commit will add a separate pipeline for secondary viewport.
ocornut pushed a commit that referenced this pull request Apr 30, 2024
…6325, #6305, #7398, #3459, #3253, #3522)

Edited from original commit: moved ImGui_ImplVulkan_CreatePipeline() call from ImGui_ImplVulkanH_CreateOrResizeWindow() to ImGui_ImplVulkan_CreateWindow().
@ocornut
Copy link
Owner

ocornut commented Apr 30, 2024

There was indeed an issue, I regret not spotting it earlier. See resolution here:
#6325 (comment)

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