Skip to content

CreateSwapChain Fix #487

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.

Sign up for GitHub

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

Open
MathewBensonCode wants to merge 1 commit into
base: develop
Choose a base branch
from
+12 −12
Open

CreateSwapChain Fix #487

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 12 additions & 12 deletions ZEngine/ZEngine/Hardwares/VulkanDevice.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1064,22 +1064,12 @@ namespace ZEngine::Hardwares
SwapchainImageHeight = capabilities.currentExtent.height;
}

auto min_image_count = std::clamp(capabilities.minImageCount, capabilities.minImageCount + 1, capabilities.maxImageCount);
auto min_image_count = std::clamp(capabilities.minImageCount, capabilities.minImageCount, capabilities.maxImageCount == 0 ? capabilities.minImageCount + 1 : capabilities.maxImageCount);
VkSwapchainCreateInfoKHR swapchain_create_info = {
.sType = VK_STRUCTURE_TYPE_SWAPCHAIN_CREATE_INFO_KHR, .pNext = nullptr, .surface = Surface, .minImageCount = min_image_count, .imageFormat = SurfaceFormat.format, .imageColorSpace = SurfaceFormat.colorSpace, .imageExtent = VkExtent2D{.width = SwapchainImageWidth, .height = SwapchainImageHeight},
.imageArrayLayers = 1, .imageUsage = VK_IMAGE_USAGE_COLOR_ATTACHMENT_BIT | VK_IMAGE_USAGE_TRANSFER_DST_BIT, .preTransform = capabilities.currentTransform, .compositeAlpha = VK_COMPOSITE_ALPHA_OPAQUE_BIT_KHR, .presentMode = PresentMode, .clipped = VK_TRUE
};

if (SwapchainImageViews.capacity() <= 0)
{
SwapchainImageViews.init(Arena, SwapchainImageCount, SwapchainImageCount);
Copy link
Owner

@JeanPhilippeKernel JeanPhilippeKernel Jan 16, 2026

Choose a reason for hiding this comment

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

init(Arena, count, count) help to pre-allocate (and zero-ed) the needed mem.

it's useful here, in this context because, once the Temp Arena below auto scratch = ZGetScratch(Arena); is called, all operations ( eg : push(...)) will use it and will get cleaned-up after release it.

But data in SwapchainImageViews and SwapchainFramebuffers need to be persistent till we close the engine.

}

if (SwapchainFramebuffers.capacity() <= 0)
{
SwapchainFramebuffers.init(Arena, SwapchainImageCount, SwapchainImageCount);
Copy link
Owner

@JeanPhilippeKernel JeanPhilippeKernel Jan 16, 2026

Choose a reason for hiding this comment

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

Same comment here about the prea-allocate ops before calling the Scratch Arena

}

auto scratch = ZGetScratch(Arena);

Array<uint32_t> family_indice = {};
Expand All @@ -1099,13 +1089,23 @@ namespace ZEngine::Hardwares
uint32_t image_count = 0;
ZENGINE_VALIDATE_ASSERT(vkGetSwapchainImagesKHR(LogicalDevice, SwapchainHandle, &image_count, nullptr) == VK_SUCCESS, "Failed to get Images count from Swapchain")

if (image_count < SwapchainImageCount)
if (image_count != SwapchainImageCount)
{
ZENGINE_CORE_WARN("Max Swapchain image count supported is {}, but requested {}", image_count, SwapchainImageCount);
SwapchainImageCount = image_count;
ZENGINE_CORE_WARN("Swapchain image count has changed from {} to {}", SwapchainImageCount, image_count);
}

if (SwapchainImageViews.capacity() <= 0)
{
SwapchainImageViews.init(Arena, SwapchainImageCount, SwapchainImageCount);
}

if (SwapchainFramebuffers.capacity() <= 0)
{
SwapchainFramebuffers.init(Arena, SwapchainImageCount, SwapchainImageCount);
}

Comment on lines +1099 to +1108
Copy link
Owner

@JeanPhilippeKernel JeanPhilippeKernel Jan 17, 2026

Choose a reason for hiding this comment

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

This will lead to a crash, because it's called under the block of Temp Arena scratch.

and you can't use the allocator Arena here till you release its Temp Arena.

------Arena mem-------|| <-----Temp Arena: Scratch (all allocation here in this zone will be ephemeral)---> || ---Arena mem------

Copy link
Contributor Author

@MathewBensonCode MathewBensonCode Jan 17, 2026

Choose a reason for hiding this comment

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

noted. thanks for the explanation on the allocation. there was an issue on one of the vulkan calls because of the mismatch in the "original" swapchain count and the count after obtaining the image count.

I will try and look at it with the allocation in mind.

Copy link
Owner

@JeanPhilippeKernel JeanPhilippeKernel Jan 17, 2026

Choose a reason for hiding this comment

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

#487 (comment)

The comment here should provide the right fix -- and helps for more clarity

Array<VkImage> SwapchainImages = {};
SwapchainImages.init(scratch.Arena, SwapchainImageCount, SwapchainImageCount);
ZENGINE_VALIDATE_ASSERT(vkGetSwapchainImagesKHR(LogicalDevice, SwapchainHandle, &SwapchainImageCount, SwapchainImages.data()) == VK_SUCCESS, "Failed to get VkImages from Swapchain")
Expand Down
Loading