Merge pull request 'fix(device): preferred mask + required-only fallback for GetMemoryType (#59)' (#87) from claude/issue-59 into master

implementations/Crafter.Graphics-Device.cpp

@@ -811,17 +811,32 @@ void Device::Initialize() {
     }
 }
 
-std::uint32_t Device::GetMemoryType(uint32_t typeBits, VkMemoryPropertyFlags properties) {
+std::uint32_t Device::GetMemoryType(uint32_t typeBits, VkMemoryPropertyFlags required, VkMemoryPropertyFlags preferred) {
+	// First pass: honour the preferred bits on top of the required ones.
+	// Second pass (when nothing matched, or no preferences were given): drop
+	// the preferences and match the required bits alone. Vulkan orders the
+	// memory types so first-superset-match is generally optimal, so a plain
+	// linear scan suffices for each pass. We only throw when even `required`
+	// cannot be satisfied — i.e. no valid memory exists for the allocation.
+	for (VkMemoryPropertyFlags wanted : {required | preferred, required})
+	{
+		uint32_t bits = typeBits;
 		for (uint32_t i = 0; i < memoryProperties.memoryTypeCount; i++)
 		{
-		if ((typeBits & 1) == 1)
+			if ((bits & 1) == 1)
 			{
-			if ((memoryProperties.memoryTypes[i].propertyFlags & properties) == properties)
+				if ((memoryProperties.memoryTypes[i].propertyFlags & wanted) == wanted)
 				{
 					return i;
 				}
 			}
-		typeBits >>= 1;
+			bits >>= 1;
+		}
+
+		// No preferences to fall back from — the second pass would be identical.
+		if (preferred == 0) {
+			break;
+		}
 	}
 
 	throw std::runtime_error("Could not find a matching memory type");

interfaces/Crafter.Graphics-DescriptorHeapVulkan.cppm

@@ -83,9 +83,14 @@ export namespace Crafter {
             samplerFreelist.clear();
             samplerFreelist.reserve(samplers);
 
+            // The heaps are written by the CPU every frame (HOST_VISIBLE is
+            // mandatory) and read by the GPU, so DEVICE_LOCAL is a perf
+            // preference, not a requirement: devices without a host-visible
+            // device-local heap (no resizable BAR) get a host-visible-only
+            // allocation instead of a hard allocation failure.
             for(std::uint8_t i = 0; i < Window::numFrames; i++) {
-                resourceHeap[i].Resize(VK_BUFFER_USAGE_SHADER_DEVICE_ADDRESS_BIT | VK_BUFFER_USAGE_DESCRIPTOR_HEAP_BIT_EXT, VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT | VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT, resourceSize);
+                resourceHeap[i].Resize(VK_BUFFER_USAGE_SHADER_DEVICE_ADDRESS_BIT | VK_BUFFER_USAGE_DESCRIPTOR_HEAP_BIT_EXT, VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT, resourceSize, VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT);
-                samplerHeap[i].Resize(VK_BUFFER_USAGE_SHADER_DEVICE_ADDRESS_BIT | VK_BUFFER_USAGE_DESCRIPTOR_HEAP_BIT_EXT, VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT | VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT, samplerSize);
+                samplerHeap[i].Resize(VK_BUFFER_USAGE_SHADER_DEVICE_ADDRESS_BIT | VK_BUFFER_USAGE_DESCRIPTOR_HEAP_BIT_EXT, VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT, samplerSize, VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT);
             }
         }
 

interfaces/Crafter.Graphics-Device.cppm

@@ -199,7 +199,12 @@ export namespace Crafter {
         // ComputeShader read the offset off the pipeline they record.
 
         static void CheckVkResult(VkResult result);
-        static std::uint32_t GetMemoryType(std::uint32_t typeBits, VkMemoryPropertyFlags properties);
+        // Selects a memory type index from typeBits that satisfies `required`.
+        // When `preferred` bits are also given, a type satisfying both is
+        // chosen first; if none exists we fall back to required-only rather
+        // than throwing. Throws only when even `required` cannot be met (no
+        // valid memory exists for the allocation).
+        static std::uint32_t GetMemoryType(std::uint32_t typeBits, VkMemoryPropertyFlags required, VkMemoryPropertyFlags preferred = 0);
 
         // ─── Wayland key repeat ────────────────────────────────────────
         // TickKeyRepeats fires onRawKeyDown / onRawKeyHold / onTextInput on

interfaces/Crafter.Graphics-VulkanBuffer.cppm

@@ -55,7 +55,12 @@ namespace Crafter {
     class VulkanBuffer : public VulkanBufferBase, public VulkanBufferMappedConditional<T, Mapped> {
     public:
         VulkanBuffer() = default;
-        void Create(VkBufferUsageFlags2 usageFlags, VkMemoryPropertyFlags memoryPropertyFlags, std::uint32_t count) {
+        // `preferredPropertyFlags` are best-effort: the allocation prefers a
+        // memory type satisfying them on top of `memoryPropertyFlags`, but
+        // falls back to the required flags alone rather than failing. Use it
+        // for perf hints (e.g. DEVICE_LOCAL on a mapped buffer) that aren't
+        // available on every device — see Device::GetMemoryType.
+        void Create(VkBufferUsageFlags2 usageFlags, VkMemoryPropertyFlags memoryPropertyFlags, std::uint32_t count, VkMemoryPropertyFlags preferredPropertyFlags = 0) {
             size = count * sizeof(T);
 
             // Carry usage in the maintenance5 flags2 chain so 64-bit bits
@@ -77,7 +82,7 @@ namespace Crafter {
             VkMemoryAllocateInfo memAlloc {
                 .sType = VK_STRUCTURE_TYPE_MEMORY_ALLOCATE_INFO,
                 .allocationSize = memReqs.size,
-                .memoryTypeIndex = Device::GetMemoryType(memReqs.memoryTypeBits, memoryPropertyFlags)
+                .memoryTypeIndex = Device::GetMemoryType(memReqs.memoryTypeBits, memoryPropertyFlags, preferredPropertyFlags)
             };
 
             VkMemoryAllocateFlagsInfoKHR allocFlagsInfo {
@@ -109,11 +114,11 @@ namespace Crafter {
             buffer = VK_NULL_HANDLE;
         }
 
-        void Resize(VkBufferUsageFlags2 usageFlags, VkMemoryPropertyFlags memoryPropertyFlags, std::uint32_t count) {
+        void Resize(VkBufferUsageFlags2 usageFlags, VkMemoryPropertyFlags memoryPropertyFlags, std::uint32_t count, VkMemoryPropertyFlags preferredPropertyFlags = 0) {
             if(buffer != VK_NULL_HANDLE) {
                Clear();
             }
-            Create(usageFlags, memoryPropertyFlags, count);
+            Create(usageFlags, memoryPropertyFlags, count, preferredPropertyFlags);
         }
 
         void Copy(VkCommandBuffer cmd, VulkanBuffer& dst) {

project.cpp

@@ -384,6 +384,35 @@ extern "C" Configuration CrafterBuildProject(std::span<const std::string_view> a
         textImpls.emplace_back("tests/ShapeTextCache/main");
         xc.GetInterfacesAndImplementations(ifaces, textImpls);
         cfg.tests.push_back(std::move(textTest));
+
+        // Issue #59: Device::GetMemoryType gained a `preferred` mask and a
+        // required-only fallback so callers combining a mandatory flag with a
+        // perf-only one (HOST_VISIBLE | DEVICE_LOCAL descriptor heaps) no
+        // longer throw on devices without a host-visible device-local heap.
+        // The selection is pure CPU logic over Device::memoryProperties, so
+        // this test installs synthetic memory layouts and drives it directly —
+        // no GPU device needed at runtime.
+        Test memTest;
+        Configuration& mc = memTest.config;
+        mc.path                 = cfg.path;
+        mc.name                 = "MemoryTypeFallback";
+        mc.outputName           = "MemoryTypeFallback";
+        mc.type                 = ConfigurationType::Executable;
+        mc.target               = cfg.target;
+        mc.march                = cfg.march;
+        mc.mtune                = cfg.mtune;
+        mc.debug                = cfg.debug;
+        mc.sysroot              = cfg.sysroot;
+        mc.dependencies         = cfg.dependencies;
+        mc.externalDependencies = cfg.externalDependencies;
+        mc.compileFlags         = cfg.compileFlags;
+        mc.linkFlags            = cfg.linkFlags;
+        mc.defines              = cfg.defines;
+        mc.cFiles               = cfg.cFiles;
+        std::vector<fs::path> memImpls(impls.begin(), impls.end());
+        memImpls.emplace_back("tests/MemoryTypeFallback/main");
+        mc.GetInterfacesAndImplementations(ifaces, memImpls);
+        cfg.tests.push_back(std::move(memTest));
     }
 
     return cfg;

tests/MemoryTypeFallback/main.cpp

@@ -0,0 +1,146 @@
+/*
+Crafter®.Graphics
+Copyright (C) 2026 Catcrafts®
+catcrafts.net
+
+This library is free software; you can redistribute it and/or
+modify it under the terms of the GNU Lesser General Public
+License version 3.0 as published by the Free Software Foundation;
+
+This library is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+Lesser General Public License for more details.
+
+You should have received a copy of the GNU Lesser General Public
+License along with this library; if not, write to the Free Software
+Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301 USA
+*/
+
+// Regression test for issue #59: Device::GetMemoryType used to be a bare
+// first-superset match that threw whenever no memory type satisfied the
+// requested property flags. Callers that combine a mandatory flag with a
+// perf-only one (e.g. HOST_VISIBLE | DEVICE_LOCAL for the descriptor heaps)
+// would then crash at allocation time on any device without a host-visible
+// device-local heap (no resizable BAR).
+//
+// GetMemoryType now takes a `preferred` mask distinct from `required`:
+//   - prefers a type satisfying required+preferred,
+//   - falls back to a required-only match when the preference is unavailable,
+//   - still throws only when even `required` cannot be met.
+//
+// The selection is pure CPU logic over Device::memoryProperties, so this test
+// drives it directly with synthetic memory layouts — no Vulkan device needed.
+
+#include <cstdlib>
+#include "vulkan/vulkan.h"
+
+import Crafter.Graphics;
+import std;
+using namespace Crafter;
+
+namespace {
+
+int failures = 0;
+
+void Check(bool ok, std::string_view what) {
+    std::println("{}  {}", ok ? "PASS" : "FAIL", what);
+    if (!ok) ++failures;
+}
+
+// Install a synthetic memory layout: one VkMemoryType per entry, in order.
+void SetMemoryTypes(std::initializer_list<VkMemoryPropertyFlags> flags) {
+    Device::memoryProperties = {};
+    Device::memoryProperties.memoryTypeCount = static_cast<std::uint32_t>(flags.size());
+    std::uint32_t i = 0;
+    for (VkMemoryPropertyFlags f : flags) {
+        Device::memoryProperties.memoryTypes[i].propertyFlags = f;
+        Device::memoryProperties.memoryTypes[i].heapIndex = 0;
+        ++i;
+    }
+}
+
+constexpr auto DEVICE_LOCAL  = VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT;
+constexpr auto HOST_VISIBLE  = VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT;
+constexpr auto HOST_COHERENT = VK_MEMORY_PROPERTY_HOST_COHERENT_BIT;
+constexpr auto HOST_CACHED   = VK_MEMORY_PROPERTY_HOST_CACHED_BIT;
+
+// Call GetMemoryType, reporting whether it threw rather than returning.
+bool Threw(std::uint32_t typeBits, VkMemoryPropertyFlags required, VkMemoryPropertyFlags preferred) {
+    try {
+        Device::GetMemoryType(typeBits, required, preferred);
+        return false;
+    } catch (const std::runtime_error&) {
+        return true;
+    }
+}
+
+} // namespace
+
+int main() {
+    constexpr std::uint32_t kAll = ~0u;
+
+    // --- required-only: first superset match wins -------------------------
+    {
+        SetMemoryTypes({DEVICE_LOCAL, HOST_VISIBLE, HOST_VISIBLE | DEVICE_LOCAL});
+        Check(Device::GetMemoryType(kAll, HOST_VISIBLE) == 1,
+              "required-only picks the first type satisfying the required bits");
+        Check(Device::GetMemoryType(kAll, DEVICE_LOCAL) == 0,
+              "required-only honours superset ordering for a different mask");
+    }
+
+    // --- typeBits gates which types are eligible --------------------------
+    {
+        SetMemoryTypes({HOST_VISIBLE, HOST_VISIBLE, HOST_VISIBLE});
+        // Only bit 2 is set -> type index 2 is the sole candidate.
+        Check(Device::GetMemoryType(0b100, HOST_VISIBLE) == 2,
+              "typeBits mask excludes types the allocation can't use");
+    }
+
+    // --- preferred bits are honoured when available -----------------------
+    {
+        SetMemoryTypes({HOST_VISIBLE, HOST_VISIBLE | HOST_COHERENT, HOST_VISIBLE | DEVICE_LOCAL});
+        Check(Device::GetMemoryType(kAll, HOST_VISIBLE, DEVICE_LOCAL) == 2,
+              "preferred DEVICE_LOCAL selects the host-visible device-local type");
+        Check(Device::GetMemoryType(kAll, HOST_VISIBLE, HOST_COHERENT) == 1,
+              "preferred HOST_COHERENT selects the host-visible coherent type");
+    }
+
+    // --- preferred unavailable: fall back to required-only ----------------
+    {
+        // No host-visible type is also device-local (the no-resizable-BAR case).
+        SetMemoryTypes({DEVICE_LOCAL, HOST_VISIBLE | HOST_COHERENT});
+        Check(Device::GetMemoryType(kAll, HOST_VISIBLE, DEVICE_LOCAL) == 1,
+              "unsatisfiable preference falls back to a required-only match instead of throwing");
+        Check(!Threw(kAll, HOST_VISIBLE, DEVICE_LOCAL),
+              "the fallback path does not throw when required is still satisfiable");
+    }
+
+    // --- preferred is satisfiable only via the second-best type -----------
+    {
+        // First host-visible type lacks the preference; a later one has it.
+        SetMemoryTypes({HOST_VISIBLE, HOST_VISIBLE | DEVICE_LOCAL});
+        Check(Device::GetMemoryType(kAll, HOST_VISIBLE, DEVICE_LOCAL) == 1,
+              "preference outranks ordering: skips an earlier required-only type");
+    }
+
+    // --- required unsatisfiable still throws ------------------------------
+    {
+        SetMemoryTypes({DEVICE_LOCAL, HOST_VISIBLE | HOST_COHERENT});
+        Check(Threw(kAll, HOST_CACHED, 0),
+              "no type satisfies the required bits -> throws (no valid memory)");
+        Check(Threw(kAll, HOST_CACHED, DEVICE_LOCAL),
+              "unsatisfiable required throws even when a preference is given");
+        // typeBits that masks out the only matching type is also unsatisfiable.
+        SetMemoryTypes({HOST_VISIBLE});
+        Check(Threw(0b10, HOST_VISIBLE, 0),
+              "typeBits excluding every match -> throws");
+    }
+
+    if (failures != 0) {
+        std::println("{} check(s) failed", failures);
+        return EXIT_FAILURE;
+    }
+    std::println("all checks passed");
+    return EXIT_SUCCESS;
+}