Merge pull request #615 from Devsh-Graphics-Programming/examples_rewrite

Bugfixes

Author: devshgraphicsprogramming

CMakeLists.txt

@@ -22,7 +22,7 @@ if(MSVC)
 	if(NBL_SANITIZE_ADDRESS)
 		set(CMAKE_MSVC_DEBUG_INFORMATION_FORMAT "$<$<CONFIG:Debug,RelWithDebInfo>:ProgramDatabase>")
 	else()
-		set(CMAKE_MSVC_DEBUG_INFORMATION_FORMAT "$<$<CONFIG:Debug,RelWithDebInfo>:EditAndContinue>")
+		set(CMAKE_MSVC_DEBUG_INFORMATION_FORMAT "$<$<CONFIG:Debug>:EditAndContinue>$<$<CONFIG:RelWithDebInfo>:ProgramDatabase>")
 	endif()
 endif()
 

cmake/common.cmake

@@ -231,17 +231,9 @@ macro(nbl_create_ext_library_project EXT_NAME LIB_HEADERS LIB_SOURCES LIB_INCLUD
 	project(${LIB_NAME})
 
 	add_library(${LIB_NAME} ${LIB_SOURCES})
-	get_target_property(_NBL_NABLA_TARGET_BINARY_DIR_ Nabla BINARY_DIR)
 
-	# TODO: correct those bugs, use generator expressions
 	target_include_directories(${LIB_NAME}
-		PUBLIC ${_NBL_NABLA_TARGET_BINARY_DIR_}/build/import
-		PUBLIC ${CMAKE_BINARY_DIR}/include/nbl/config/debug
-		PUBLIC ${CMAKE_BINARY_DIR}/include/nbl/config/release
-		PUBLIC ${CMAKE_BINARY_DIR}/include/nbl/config/relwithdebinfo
-		PUBLIC ${CMAKE_SOURCE_DIR}/include
-		PUBLIC ${CMAKE_SOURCE_DIR}/src
-		PUBLIC ${CMAKE_SOURCE_DIR}/source/Nabla
+		PUBLIC $<TARGET_PROPERTY:Nabla,INCLUDE_DIRECTORIES>
 		PRIVATE ${LIB_INCLUDES}
 	)
 

examples_tests

@@ -27,27 +27,23 @@ namespace nbl::asset
 class ICPUBuffer : public asset::IBuffer, public asset::IAsset
 {
     protected:
-        virtual ~ICPUBuffer()
-        {
-            this->convertToDummyObject();
-        }
-
         //! Non-allocating constructor for CCustormAllocatorCPUBuffer derivative
         ICPUBuffer(size_t sizeInBytes, void* dat) : asset::IBuffer({ dat ? sizeInBytes : 0,EUF_TRANSFER_DST_BIT }), data(dat) {}
+
     public:
         //! Constructor. TODO: remove, alloc can fail, should be a static create method instead!
         /** @param sizeInBytes Size in bytes. If `dat` argument is present, it denotes size of data pointed by `dat`, otherwise - size of data to be allocated.
         */
         ICPUBuffer(size_t sizeInBytes) : asset::IBuffer({0,EUF_TRANSFER_DST_BIT})
         {
             data = _NBL_ALIGNED_MALLOC(sizeInBytes,_NBL_SIMD_ALIGNMENT);
-            if (!data)
+            if (!data) // FIXME: cannot fail like that, need factory `create` methods
                 return;
 
             m_creationParams.size = sizeInBytes;
         }
 
-        core::smart_refctd_ptr<IAsset> clone(uint32_t = ~0u) const override
+        core::smart_refctd_ptr<IAsset> clone(uint32_t = ~0u) const override final
         {
             auto cp = core::make_smart_refctd_ptr<ICPUBuffer>(m_creationParams.size);
             clone_common(cp.get());
@@ -56,33 +52,29 @@ class ICPUBuffer : public asset::IBuffer, public asset::IAsset
             return cp;
         }
 
-        virtual void convertToDummyObject(uint32_t referenceLevelsBelowToConvert = 0u) override
+        void convertToDummyObject(uint32_t referenceLevelsBelowToConvert = 0u) override final
         {
             if (!canBeConvertedToDummy())
                 return;
             convertToDummyObject_common(referenceLevelsBelowToConvert);
-
-            if (data)
-                _NBL_ALIGNED_FREE(data);
-            data = nullptr;
-            m_creationParams.size = 0ull;
+            freeData();
             isDummyObjectForCacheAliasing = true;
         }
 
         _NBL_STATIC_INLINE_CONSTEXPR auto AssetType = ET_BUFFER;
-        inline IAsset::E_TYPE getAssetType() const override { return AssetType; }
+        inline IAsset::E_TYPE getAssetType() const override final { return AssetType; }
 
-        virtual size_t conservativeSizeEstimate() const override { return getSize(); }
+        size_t conservativeSizeEstimate() const override final { return getSize(); }
 
         //! Returns pointer to data.
-        virtual const void* getPointer() const {return data;}
-        virtual void* getPointer() 
+        const void* getPointer() const {return data;}
+        void* getPointer() 
         { 
             assert(!isImmutable_debug());
             return data;
         }
 
-        bool canBeRestoredFrom(const IAsset* _other) const override
+        bool canBeRestoredFrom(const IAsset* _other) const override final
         {
             auto* other = static_cast<const ICPUBuffer*>(_other);
             if (m_creationParams.size != other->m_creationParams.size)
@@ -108,14 +100,27 @@ class ICPUBuffer : public asset::IBuffer, public asset::IAsset
         }
 
     protected:
-        void restoreFromDummy_impl(IAsset* _other, uint32_t _levelsBelow) override
+        void restoreFromDummy_impl(IAsset* _other, uint32_t _levelsBelow) override final
         {
             auto* other = static_cast<ICPUBuffer*>(_other);
 
+            // NO THIS IS A NIGHTMARE!
+            // FIXME: ONLY SWAP FOR COMPATIBLE ALLOCATORS! OTHERWISE MEMCPY!
             if (willBeRestoredFrom(_other))
                 std::swap(data, other->data);
         }
 
+        // REMEMBER TO CALL FROM DTOR!
+        // TODO: idea, make the `ICPUBuffer` an ADT, and use the default allocator CCPUBuffer instead for consistency
+        // TODO: idea make a macro for overriding all `delete` operators of a class to enforce a finalizer that runs in reverse order to destructors (to allow polymorphic cleanups)
+        virtual void freeData()
+        {
+            if (data)
+                _NBL_ALIGNED_FREE(data);
+            data = nullptr;
+            m_creationParams.size = 0ull;
+        }
+
         void* data;
 };
 
@@ -140,42 +145,37 @@ using CDummyCPUBuffer = CCustomAllocatorCPUBuffer<core::null_allocator<uint8_t>,
     */
 
 template<typename Allocator>
-class CCustomAllocatorCPUBuffer<Allocator, true> : public ICPUBuffer
+class CCustomAllocatorCPUBuffer<Allocator,true> : public ICPUBuffer
 {
         static_assert(sizeof(typename Allocator::value_type) == 1u, "Allocator::value_type must be of size 1");
     protected:
         Allocator m_allocator;
 
-        virtual ~CCustomAllocatorCPUBuffer()
+        virtual ~CCustomAllocatorCPUBuffer() final
         {
-            this->convertToDummyObject();
+            freeData();
         }
-
-    public:
-        CCustomAllocatorCPUBuffer(size_t sizeInBytes, void* dat, core::adopt_memory_t, Allocator&& alctr = Allocator()) : ICPUBuffer(sizeInBytes, dat), m_allocator(std::move(alctr))
-        {
-        }
-
-        virtual void convertToDummyObject(uint32_t referenceLevelsBelowToConvert = 0u) override
+        inline void freeData() override
         {
-            if (isDummyObjectForCacheAliasing)
-                return;
-            convertToDummyObject_common(referenceLevelsBelowToConvert);
-            if (!canBeConvertedToDummy())
-                return;
-
             if (ICPUBuffer::data)
                 m_allocator.deallocate(reinterpret_cast<typename Allocator::pointer>(ICPUBuffer::data), ICPUBuffer::m_creationParams.size);
             ICPUBuffer::data = nullptr; // so that ICPUBuffer won't try deallocating
         }
+
+    public:
+        CCustomAllocatorCPUBuffer(size_t sizeInBytes, void* dat, core::adopt_memory_t, Allocator&& alctr = Allocator()) : ICPUBuffer(sizeInBytes,dat), m_allocator(std::move(alctr))
+        {
+        }
 };
 
 template<typename Allocator>
 class CCustomAllocatorCPUBuffer<Allocator, false> : public CCustomAllocatorCPUBuffer<Allocator, true>
 {
         using Base = CCustomAllocatorCPUBuffer<Allocator, true>;
+
     protected:
         virtual ~CCustomAllocatorCPUBuffer() = default;
+        inline void freeData() override {}
 
     public:
         using Base::Base;

include/nbl/asset/ICPUBuffer.h

@@ -210,7 +210,8 @@ class NBL_API2 IShaderCompiler : public core::IReferenceCounted
 		template<typename... Args>
 		static core::smart_refctd_ptr<ICPUShader> createOverridenCopy(const ICPUShader* original, uint32_t position, const char* fmt, Args... args)
 		{
-			assert(original == nullptr || (!original->isADummyObjectForCache() && original->isContentHighLevelLanguage()));
+			if (!original || original->isADummyObjectForCache() || !original->isContentHighLevelLanguage())
+				return nullptr;
 
 			constexpr auto getMaxSize = [](auto num) -> size_t
 			{
@@ -230,12 +231,13 @@ class NBL_API2 IShaderCompiler : public core::IReferenceCounted
 				}
 			};
 			constexpr size_t templateArgsCount = sizeof...(Args);
-			size_t origLen = original ? original->getContent()->getSize():0u;
-			size_t formatArgsCharSize = (getMaxSize(args) + ...);
-			size_t formatSize = strlen(fmt);
+			const size_t origLen = original ? original->getContent()->getSize():0u;
+			const size_t formatArgsCharSize = (getMaxSize(args) + ...);
+			const size_t formatSize = strlen(fmt);
 			// 2 is an average size of a format (% and a letter) in chars. 
 			// Assuming the format contains only one letter, but if it's 2, the outSize is gonna be a touch bigger.
-			size_t outSize = origLen + formatArgsCharSize + formatSize - 2 * templateArgsCount;
+			constexpr size_t nullTerminatorSize = 1u;
+			size_t outSize = origLen + formatArgsCharSize + formatSize + nullTerminatorSize - 2 * templateArgsCount;
 
 			nbl::core::smart_refctd_ptr<ICPUBuffer> outBuffer = nbl::core::make_smart_refctd_ptr<ICPUBuffer>(outSize);
 

include/nbl/asset/utils/IShaderCompiler.h

@@ -155,6 +155,7 @@ namespace nbl::core
             _NBL_STATIC_INLINE_CONSTEXPR bool         supportsArbitraryOrderFrees = resolve_supportsArbitraryOrderFrees<AddressAlloc>::value;
             _NBL_STATIC_INLINE_CONSTEXPR uint32_t     maxMultiOps                 = resolve_maxMultiOps<AddressAlloc>::value;
 
+            // TODO: make the printer customizable without making a `core`->`system` circular dep
             static inline void          printDebugInfo()
             {
                 printf("has_func_multi_alloc_addr : %s\n",                  has_func_multi_alloc_addr<AddressAlloc>::value ? "true":"false");

include/nbl/core/alloc/address_allocator_traits.h

@@ -126,15 +126,17 @@ class NBL_API2 IGPUCommandBuffer :
 
     IGPUCommandBuffer(core::smart_refctd_ptr<const ILogicalDevice>&& dev, E_LEVEL lvl, core::smart_refctd_ptr<IGPUCommandPool>&& _cmdpool, system::logger_opt_smart_ptr&& logger) : base_t(lvl), IBackendObject(std::move(dev)), m_cmdpool(_cmdpool), m_logger(std::move(logger))
     {
+        // prevent false positives on first `begin()`
+        m_resetCheckedStamp = m_cmdpool->getResetCounter();
     }
 
-    virtual ~IGPUCommandBuffer()
+    // meant to be invoked by most derived class
+    // TODO: idea make a macro for overriding all `delete` operators of a class to enforce a finalizer that runs in reverse order to destructors (to allow polymorphic cleanups)
+    inline void out_of_order_dtor()
     {
         // Only release the resources if the parent pool has not been reset because if it has been then the resources will already be released.
         if (!checkForParentPoolReset())
-        {
             releaseResourcesBackToPool();
-        }
     }
 
     system::logger_opt_smart_ptr m_logger;
@@ -252,9 +254,7 @@ class NBL_API2 IGPUCommandBuffer :
         m_commandList.head = nullptr;
         m_commandList.tail = nullptr;
 
-        // Example 05 crashes here, with assert in `ucrt` maybe vtable gets lost or something?
-        // For now commented out because we only have a Vulkan backend and function doesn't do anything.
-        //checkForParentPoolReset_impl();
+        checkForParentPoolReset_impl();
 
         return true;
     }
@@ -288,7 +288,7 @@ class NBL_API2 IGPUCommandBuffer :
         return true;
     }
 
-    uint64_t m_resetCheckedStamp = 0;
+    uint64_t m_resetCheckedStamp;
 
     // This bound descriptor set record doesn't include the descriptor sets whose layout has _any_ one of its bindings
     // created with IGPUDescriptorSetLayout::SBinding::E_CREATE_FLAGS::ECF_UPDATE_AFTER_BIND_BIT

include/nbl/video/IGPUCommandBuffer.h

@@ -64,6 +64,19 @@ class IGPUQueue : public core::Interface, public core::Unmovable
         uint32_t getFamilyIndex() const { return m_familyIndex; }
         E_CREATE_FLAGS getFlags() const { return m_flags; }
 
+        // When dealing with external/foreign queues treat `other` as nullptr
+        inline bool needsOwnershipTransfer(const IGPUQueue* other) const
+        {
+            if (!other)
+                return true;
+
+            if (m_familyIndex==other->m_familyIndex)
+                return false;
+            
+            // TODO: take into account concurrent sharing indices, but then we'll need to remember the concurrent sharing family indices
+            return true;
+        }
+
         inline constexpr static float DEFAULT_QUEUE_PRIORITY = 1.f;
 
         // OpenGL: const egl::CEGL::Context*

include/nbl/video/IGPUQueue.h

@@ -392,7 +392,7 @@ class NBL_API2 IPhysicalDevice : public core::Interface, public core::Unmovable
                 uint16_t transferDst : 1u;
                 uint16_t log2MaxSamples : 3u; // 0 means cant use as a multisample image format
 
-                SUsage()
+                constexpr SUsage()
                     : sampledImage(0)
                     , storageImage(0)
                     , storageImageAtomic(0)
@@ -405,7 +405,7 @@ class NBL_API2 IPhysicalDevice : public core::Interface, public core::Unmovable
                     , log2MaxSamples(0)
                 {}
 
-                SUsage(core::bitflag<asset::IImage::E_USAGE_FLAGS> usages):
+                constexpr SUsage(core::bitflag<asset::IImage::E_USAGE_FLAGS> usages) :
                     log2MaxSamples(0),
                     sampledImage(usages.hasFlags(asset::IImage::EUF_SAMPLED_BIT)),
                     storageImage(usages.hasFlags(asset::IImage::EUF_STORAGE_BIT)),
@@ -419,7 +419,7 @@ class NBL_API2 IPhysicalDevice : public core::Interface, public core::Unmovable
                     storageImageAtomic(0)
                 {}
 
-                inline SUsage operator & (const SUsage& other) const
+                constexpr SUsage operator&(const SUsage& other) const
                 {
                     SUsage result;
                     result.sampledImage = sampledImage & other.sampledImage;
@@ -435,7 +435,7 @@ class NBL_API2 IPhysicalDevice : public core::Interface, public core::Unmovable
                     return result;
                 }
 
-                inline SUsage operator | (const SUsage& other) const
+                constexpr SUsage operator|(const SUsage& other) const
                 {
                     SUsage result;
                     result.sampledImage = sampledImage | other.sampledImage;
@@ -451,7 +451,7 @@ class NBL_API2 IPhysicalDevice : public core::Interface, public core::Unmovable
                     return result;
                 }
 
-                inline SUsage operator ^ (const SUsage& other) const
+                constexpr SUsage operator^(const SUsage& other) const
                 {
                     SUsage result;
                     result.sampledImage = sampledImage ^ other.sampledImage;
@@ -467,7 +467,7 @@ class NBL_API2 IPhysicalDevice : public core::Interface, public core::Unmovable
                     return result;
                 }
 
-                inline bool operator<(const SUsage& other) const
+                constexpr bool operator<(const SUsage& other) const
                 {
                     if (sampledImage && !other.sampledImage) return false;
                     if (storageImage && !other.storageImage) return false;
@@ -482,7 +482,7 @@ class NBL_API2 IPhysicalDevice : public core::Interface, public core::Unmovable
                     return true;
                 }
 
-                inline bool operator == (const SUsage& other) const
+                constexpr bool operator==(const SUsage& other) const
                 {
                     return
                         (sampledImage == other.sampledImage) &&
@@ -528,7 +528,7 @@ class NBL_API2 IPhysicalDevice : public core::Interface, public core::Unmovable
             EQF_NONE = 0,
             EQF_GRAPHICS_BIT = 0x01,
             EQF_COMPUTE_BIT = 0x02,
-            EQF_TRANSFER_BIT = 0x04,
+            EQF_TRANSFER_BIT = 0x04, // TODO: investigate whether GRAPHICS or COMPUTE can NOT report TRANSFER at the same time
             EQF_SPARSE_BINDING_BIT = 0x08,
             EQF_PROTECTED_BIT = 0x10
         };