From 683e171fd18143cb677fd3d4604ecb78502a2d9d Mon Sep 17 00:00:00 2001 From: Kelsi Date: Mon, 30 Mar 2026 14:24:41 -0700 Subject: [PATCH] fix: VkTexture move/destroy ownsSampler_ flag, extract finalizeSampler - Fix move constructor and move assignment: set other.ownsSampler_ to false after transfer (was incorrectly set to true, leaving moved-from object claiming ownership of a null sampler) - Fix destroy(): reset ownsSampler_ to false after clearing sampler handle (was set to true, inconsistent with null handle state) - Extract finalizeSampler() from 3 duplicated cache-or-create blocks in createSampler() overloads and createShadowSampler() (-24 lines) - Add SPIR-V alignment why-comment in vk_shader.cpp --- include/rendering/vk_texture.hpp | 2 + src/rendering/vk_shader.cpp | 1 + src/rendering/vk_texture.cpp | 75 ++++++++++---------------------- 3 files changed, 27 insertions(+), 51 deletions(-) diff --git a/include/rendering/vk_texture.hpp b/include/rendering/vk_texture.hpp index 51c57db8..fb0e01f1 100644 --- a/include/rendering/vk_texture.hpp +++ b/include/rendering/vk_texture.hpp @@ -68,6 +68,8 @@ public: private: void generateMipmaps(VkContext& ctx, VkFormat format, uint32_t width, uint32_t height); + // Shared sampler finalization: prefer the global cache, fall back to direct creation + bool finalizeSampler(VkDevice device, const VkSamplerCreateInfo& samplerInfo); AllocatedImage image_{}; VkSampler sampler_ = VK_NULL_HANDLE; diff --git a/src/rendering/vk_shader.cpp b/src/rendering/vk_shader.cpp index 5ebc7a08..e20a2dc0 100644 --- a/src/rendering/vk_shader.cpp +++ b/src/rendering/vk_shader.cpp @@ -32,6 +32,7 @@ bool VkShaderModule::loadFromFile(VkDevice device, const std::string& path) { } size_t fileSize = static_cast(file.tellg()); + // SPIR-V is a stream of 32-bit words — file size must be a multiple of 4 if (fileSize == 0 || fileSize % 4 != 0) { LOG_ERROR("Invalid SPIR-V file size (", fileSize, "): ", path); return false; diff --git a/src/rendering/vk_texture.cpp b/src/rendering/vk_texture.cpp index 6ef1abac..c11b5921 100644 --- a/src/rendering/vk_texture.cpp +++ b/src/rendering/vk_texture.cpp @@ -17,7 +17,8 @@ VkTexture::VkTexture(VkTexture&& other) noexcept ownsSampler_(other.ownsSampler_) { other.image_ = {}; other.sampler_ = VK_NULL_HANDLE; - other.ownsSampler_ = true; + // Source no longer owns the sampler — ownership transferred to this instance + other.ownsSampler_ = false; } VkTexture& VkTexture::operator=(VkTexture&& other) noexcept { @@ -28,7 +29,7 @@ VkTexture& VkTexture::operator=(VkTexture&& other) noexcept { ownsSampler_ = other.ownsSampler_; other.image_ = {}; other.sampler_ = VK_NULL_HANDLE; - other.ownsSampler_ = true; + other.ownsSampler_ = false; } return *this; } @@ -196,6 +197,23 @@ bool VkTexture::createDepth(VkContext& ctx, uint32_t width, uint32_t height, VkF return true; } +// Shared sampler finalization: try the global cache first (avoids duplicate Vulkan +// sampler objects), fall back to direct creation if no VkContext is available. +bool VkTexture::finalizeSampler(VkDevice device, const VkSamplerCreateInfo& samplerInfo) { + auto* ctx = VkContext::globalInstance(); + if (ctx) { + sampler_ = ctx->getOrCreateSampler(samplerInfo); + ownsSampler_ = false; + return sampler_ != VK_NULL_HANDLE; + } + if (vkCreateSampler(device, &samplerInfo, nullptr, &sampler_) != VK_SUCCESS) { + LOG_ERROR("Failed to create texture sampler"); + return false; + } + ownsSampler_ = true; + return true; +} + bool VkTexture::createSampler(VkDevice device, VkFilter minFilter, VkFilter magFilter, VkSamplerAddressMode addressMode, float maxAnisotropy) @@ -217,22 +235,7 @@ bool VkTexture::createSampler(VkDevice device, samplerInfo.mipLodBias = 0.0f; samplerInfo.minLod = 0.0f; samplerInfo.maxLod = static_cast(mipLevels_); - - // Use sampler cache if VkContext is available. - auto* ctx = VkContext::globalInstance(); - if (ctx) { - sampler_ = ctx->getOrCreateSampler(samplerInfo); - ownsSampler_ = false; - return sampler_ != VK_NULL_HANDLE; - } - - // Fallback: no VkContext (shouldn't happen in normal use). - if (vkCreateSampler(device, &samplerInfo, nullptr, &sampler_) != VK_SUCCESS) { - LOG_ERROR("Failed to create texture sampler"); - return false; - } - ownsSampler_ = true; - return true; + return finalizeSampler(device, samplerInfo); } bool VkTexture::createSampler(VkDevice device, @@ -258,22 +261,7 @@ bool VkTexture::createSampler(VkDevice device, samplerInfo.mipLodBias = 0.0f; samplerInfo.minLod = 0.0f; samplerInfo.maxLod = static_cast(mipLevels_); - - // Use sampler cache if VkContext is available. - auto* ctx = VkContext::globalInstance(); - if (ctx) { - sampler_ = ctx->getOrCreateSampler(samplerInfo); - ownsSampler_ = false; - return sampler_ != VK_NULL_HANDLE; - } - - // Fallback: no VkContext (shouldn't happen in normal use). - if (vkCreateSampler(device, &samplerInfo, nullptr, &sampler_) != VK_SUCCESS) { - LOG_ERROR("Failed to create texture sampler"); - return false; - } - ownsSampler_ = true; - return true; + return finalizeSampler(device, samplerInfo); } bool VkTexture::createShadowSampler(VkDevice device) { @@ -290,22 +278,7 @@ bool VkTexture::createShadowSampler(VkDevice device) { samplerInfo.mipmapMode = VK_SAMPLER_MIPMAP_MODE_NEAREST; samplerInfo.minLod = 0.0f; samplerInfo.maxLod = 1.0f; - - // Use sampler cache if VkContext is available. - auto* ctx = VkContext::globalInstance(); - if (ctx) { - sampler_ = ctx->getOrCreateSampler(samplerInfo); - ownsSampler_ = false; - return sampler_ != VK_NULL_HANDLE; - } - - // Fallback: no VkContext (shouldn't happen in normal use). - if (vkCreateSampler(device, &samplerInfo, nullptr, &sampler_) != VK_SUCCESS) { - LOG_ERROR("Failed to create shadow sampler"); - return false; - } - ownsSampler_ = true; - return true; + return finalizeSampler(device, samplerInfo); } void VkTexture::destroy(VkDevice device, VmaAllocator allocator) { @@ -313,7 +286,7 @@ void VkTexture::destroy(VkDevice device, VmaAllocator allocator) { vkDestroySampler(device, sampler_, nullptr); } sampler_ = VK_NULL_HANDLE; - ownsSampler_ = true; + ownsSampler_ = false; destroyImage(device, allocator, image_); }