So I pushed several commits to the Vulkan branch incorporating your changes. I'm not done yet, but I wanted to explain them.
You'd probably want to take the
main commit, copy paste over a clean checkout of your branch to see the differences (the next commits are just merges with master).
Your work is helping me A TON. Vulkan is very complex. My approach on Ogre is to think for weeks about what I'm going to do, layout a mental plan of how the components are going to interact, the repercusions on existing code, and then start writing the code.
But Vulkan is so complex I kept getting overwhelmed. Too many things to track down that branch into many potential use case/scenarios we have to consider.
I rewrote VulkanQueue::getCopyEncoder like 3 times because I kept missing use cases and leaving potential hazards. What if someone downloads data from GPU, uploads data to that region, and downloads it again? (without even using it). It would be a stupid usage pattern but it's not impossible. And if it happens, it has to work. Our code doesn't do that (that I'm aware of) but our users could.
And all that just to handle potential race conditions when
copying memory! I didn't even touch code related to
actual rendering yet.
So I rewrote the code to take that into account and then realize it was a good thing I did that because my old version would initiate uploads before the GPU is done using that data. Now I think I finally accounted for everything. And if I didn't then we'll fix those edge cases when we get them. The current code should be robust enough.
Your approach was pretty much the opposite of mine's: Try to get it running ASAP, even it that means leaking 1GB of RAM per second and iterating through all of our buffers every frame to rebuild the descriptors.
Yeah that sucks. But you're coding on Windows with an NVIDIA GPU (correct me if I'm wrong), and I got the same image as you, while working on Linux with AMD's RADV driver. When it comes to Vulkan, that kind of cross-OS, cross-vendor compatibility is a major achievement! Congratulations! (seriously. No sarcasm)
Thus while we can't obviously deploy that kind of brute force (leaks and iterate through all of our buffers every frame) in the final version, having a working version is of tremendous help: I can see what worked, what does not. I can also test other stuff to see if the validation layers will complain.
I no longer get mentally blocked by all the potential paths because I can try them out and probe which approaches look more promising.
Now onto the code itself:
I left out most of the descriptor stuff (shaders/bindings are not yet working in my branch) because the way you're doing it is too brute force. But it is giving me some ideas.
I think that it is clear that I should start with a similar approach of rebuilding everything at first, and later phase it out for a version that can reuse descriptors.
I didn't keep findDepthFormat, so probably the Windows version won't compile, but you may notice that I did check a different version of findSupportedFormat, which avoids VkFormat -> PixelFormatGpu conversion. Thus writing a new findDepthFormat should be a piece of cake
Like I explained in a Github comment, in my experience the original version you wrote is a bad pattern because eventually as new code gets added we fall into the trap of code doing PixelFormatGpu -> VkFormat -> PixelFormatGpu, and this eventually breaks. Either because the conversion isn't lossless or because we end up with two pointers having different PixelFormatGpu, but the same VkFormat or viceversa. Or the user caches getPixelFormat() only to see it later changed, because the Vulkan backend silently changed the Ogre format. This can lead to all sorts of bugs (I asked for a lemon pie, I was handled the fork and then I got an ice cream instead).
I fixed the VaoManager's memory family selection. If the GPU doesn't support non-coherent memory but the user requests BT_DYNAMIC_PERSISTENT, we use coherent memory and pretend it is non-coherent. Same viceversa. That's how it was supposed to work.
The VaoManager is preferring non-cached (i.e. write combined) memory for StagingBuffers even if we request for downloading, which is wrong. Reading from write-combined memory is slow. We'll have to fix this (by adding a new VulkanVaoManager::VboFlag for cached reads and using that one when StagingBuffers for downloads are requested).
I also moved a lot of map, flush, invalidate and unmapping to VulkanDynamicBuffer. It greatly simplified the other classes which were talking to Vulkan directly. Now VulkanDynamicBuffer takes care of rounding up the next nonCoherentAtomSize.
I also added VulkanQueue::getCopyEncoder, although we never yet call any of the end*Encoder() functions (see Metal, look for endBlitEncoder calls and beginRenderEncoder/ComputeEncoder ones).
The beauty of getCopyEncoder is that between 0 and a few memory barriers when getCopyEncoder gets called and then one final memory barrier at the end when endCopyEncoder() is called.
Your approach was to issue two memory barrier for every vkCmdCopy* call we do. This works but it was very suboptimal.
I wrapped some bits that I intend to be temporary under "VULKAN_HOTSHOT_WILL_REMOVE" ifdefs, while I've wrapped around "VULKAN_HOTSHOT_DISABLED" code that is not included but I wanted to keep to see how it works.
There are other classes such as VulkanTextureGpu that I didn't yet fully review.
Tomorrow I will keep working on this and see where that leads us.
Keep up doing the good stuff.
Cheers
