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

Dx12 vulkan texture management #2697

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kudaba
Copy link
Contributor

@kudaba kudaba commented Jul 28, 2019

I hope you don't mind that I put a few related changes into a single request:

  1. This is a replacement for Add a callback for customizing how textures are set during rendering #2528
  2. Apply texture callback changes to vulkan backend and implement descriptor set management (Vulkan: switching between images for rendering #914).
  3. Found an edge case where if the only thing passed to imgui was render callbacks, they wouldn't get called when using the vulkan backend.

For more details about the descriptor management: From what I can tell, resetting the descriptor pool and building the descriptor sets every frame is actually fairly cheap. The only costly bit would be any time the pool has to grow, but I don't think it will cause any issues for most normal use cases.

…g rendering.

DX12: Only generate the Font texture if it doesn't already exist. Only set texture if it actually changes.
These changes can enable better integration to existing rendereres by ensuring that all ImTextureID objects are the same type as defined by the user.
* Pool allocates linearly and resets each frame.
* If pool limit is reached a new pool will be allocated at double the size up to 1024 then it increments in steps of 1024
* Add usage of SetTextureFn, otherwise assumes TextureId is a VkImageView
* Users of SetTextureFn can set textures using ImGui_ImplVulkan_SetTexture
@kudaba kudaba force-pushed the dx12_vulkan_texture_management branch from 892020e to 4fb09ae Compare September 17, 2019 05:14
@kudaba
Copy link
Contributor Author

kudaba commented Sep 17, 2019

Hey, I was wondering if this can get some attention. I have two follow up changes, 5376e58 and a4a3a1d which contain fixes for viewports, but are also based on these changes. I'd combine them into one PR but those two are viewport specific where the 3 here can go into the master branch.

@ocornut
Copy link
Owner

ocornut commented Apr 26, 2020

Hello,

I realize this is not the full merge you've been waiting for, but I just noticed that the 1st commit (fix minor Vulkan edge case) could be merged so it's merged now: 73c30aa

It's possible this will now conflicts, you might drop the first commit and then rebase if you have time.

(FYI we're working on another thing which overlaps this topic so this may be dragged in toward the full solution.)

Thanks again!

@kudaba
Copy link
Contributor Author

kudaba commented May 9, 2020

Hi, thanks for looking back at this. unfortunately other things have caused me to go for a custom render backend so I'm handling this all internally now.

@kudaba kudaba closed this May 9, 2020
@kudaba kudaba deleted the dx12_vulkan_texture_management branch May 9, 2020 05:28
@ocornut
Copy link
Owner

ocornut commented May 9, 2020

Err? This is useful reference for dear imgui, I wish you didn’t delete that branch.

@kudaba kudaba restored the dx12_vulkan_texture_management branch May 9, 2020 22:47
@kudaba
Copy link
Contributor Author

kudaba commented May 9, 2020

Oh, sorry, I thought github would do better at keeping it alive, I managed to find it locally to resurrect it.

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

2 participants