New debug draw extension for AABBs #900

keptsecret · 2025-07-08T08:45:47Z

No description provided.

include/nbl/ext/DebugDraw/CDrawAABB.h

include/nbl/ext/DebugDraw/builtin/hlsl/common.hlsl

include/nbl/ext/DebugDraw/builtin/hlsl/aabb_instances.fragment.hlsl

include/nbl/ext/DebugDraw/builtin/hlsl/aabb_instances.vertex.hlsl

include/nbl/ext/DebugDraw/CDrawAABB.h

src/nbl/ext/DebugDraw/CDrawAABB.cpp

devshgraphicsprogramming · 2025-09-17T09:49:27Z

src/nbl/ext/DebugDraw/CDrawAABB.cpp

+			CHLSLCompiler::SOptions options = {};
+			options.stage = stage;
+			options.preprocessorOptions.sourceIdentifier = filePath;
+			options.preprocessorOptions.logger = params.utilities->getLogger();
+			options.preprocessorOptions.includeFinder = includeFinder.get();
+			shaderSrc = compiler->compileToSPIRV((const char*)shaderSrc->getContent()->getPointer(), options);
+
+			return params.utilities->getLogicalDevice()->compileShader({ shaderSrc.get() });


@AnastaZIuk can we make @YasInvolved use CMake to make precompiled SPIR-V with NSC right way in a future PR to CI the shaders?

Okay now that master has the infrastructure for it, @AnastaZIuk decide if you want to teach @keptsecret how to do it, write a Wiki/Readme or have @Przemog1 do it

I will get you a readme tutorial

devshgraphicsprogramming · 2025-09-17T10:16:28Z

src/nbl/ext/DebugDraw/CDrawAABB.cpp

+	auto getRequiredAccessFlags = [&](const bitflag<IDeviceMemoryAllocation::E_MEMORY_PROPERTY_FLAGS>& properties)
+		{
+			bitflag<IDeviceMemoryAllocation::E_MAPPING_CPU_ACCESS_FLAGS> flags(IDeviceMemoryAllocation::EMCAF_NO_MAPPING_ACCESS);
+
+			if (properties.hasFlags(IDeviceMemoryAllocation::EMPF_HOST_READABLE_BIT))
+				flags |= IDeviceMemoryAllocation::EMCAF_READ;
+			if (properties.hasFlags(IDeviceMemoryAllocation::EMPF_HOST_WRITABLE_BIT))
+				flags |= IDeviceMemoryAllocation::EMCAF_WRITE;
+
+			return flags;
+		};


only check for write flag on the mapping

devshgraphicsprogramming · 2025-09-17T10:59:00Z

src/nbl/ext/DebugDraw/CDrawAABB.cpp

+	smart_refctd_ptr<IGPUBuffer> indicesBuffer;
+	params.utilities->createFilledDeviceLocalBufferOnDedMem(
+		SIntendedSubmitInfo{ .queue = params.transfer },
+		std::move(bufparams),
+		unitAABBIndices.data()
+	).move_into(indicesBuffer);


it would be slightly less complex (wouldn't have to take the utilities in the creation params) if you just shot off a single use commandbuffer with updateBuffer command since the update is less than 64kb (you just need the usage flag on the buffer to allow it)

utilities is still used to get logical device and logger elsewhere. Though we could pass those into creation params separately instead

devshgraphicsprogramming · 2025-09-17T10:59:54Z

src/nbl/ext/DebugDraw/CDrawAABB.cpp

+		.offset = 0,
+		.size = sizeof(SPushConstants)
+	};
+	return device->createPipelineLayout({ &pcRange , 1 }, nullptr, nullptr, nullptr, nullptr);


why no tcall the createPipelineLayoutFromPCRange

devshgraphicsprogramming · 2025-09-17T11:01:19Z

src/nbl/ext/DebugDraw/CDrawAABB.cpp

+		singlePipeline = createPipeline(params, params.singlePipelineLayout.get(), "single.vertex.hlsl", "aabb_instances.fragment.hlsl");
+		if (!singlePipeline)
+		{
+			logger->log("Failed to create pipeline!", ILogger::ELL_ERROR);
+			return nullptr;
+		}
+	}
+
+	smart_refctd_ptr<IGPUGraphicsPipeline> batchPipeline = nullptr;
+	if (params.drawMode & ADM_DRAW_BATCH)
+	{
+		batchPipeline = createPipeline(params, params.batchPipelineLayout.get(), "aabb_instances.vertex.hlsl", "aabb_instances.fragment.hlsl");


shouldn't you default the layouts if they're missing?

devshgraphicsprogramming · 2025-09-17T11:01:49Z

src/nbl/ext/DebugDraw/CDrawAABB.cpp

+	SPushConstantRange pcRange = {
+		.stageFlags = IShader::E_SHADER_STAGE::ESS_VERTEX,
+		.offset = 0,
+		.size = sizeof(SPushConstants)


your createDefaultPipelineLayout needs to take an enum about what pipeline this is for, cause you use two different push constant structs

devshgraphicsprogramming · 2025-09-17T11:15:48Z

src/nbl/ext/DebugDraw/CDrawAABB.cpp

+	return true;
+}
+
+hlsl::float32_t4x4 DrawAABB::getTransformFromAABB(const hlsl::shapes::AABB<3, float>& aabb)


this should return 3x4 not 4x4

devshgraphicsprogramming · 2025-09-17T11:16:44Z

src/nbl/ext/DebugDraw/CDrawAABB.cpp

+bool DrawAABB::renderSingle(IGPUCommandBuffer* commandBuffer, const hlsl::shapes::AABB<3, float>& aabb, const hlsl::float32_t4& color, const hlsl::float32_t4x4& cameraMat)
+{
+	if (!(m_cachedCreationParams.drawMode & ADM_DRAW_SINGLE))
+	{
+		m_cachedCreationParams.utilities->getLogger()->log("DrawAABB has not been enabled for draw single!", ILogger::ELL_ERROR);
+		return false;
+	}
+
+	commandBuffer->bindGraphicsPipeline(m_singlePipeline.get());
+	commandBuffer->setLineWidth(1.f);


make a struct with command buffer pointer, camera viewProj matrix and line width to use for both functions

devshgraphicsprogramming · 2025-09-17T11:17:15Z

src/nbl/ext/DebugDraw/CDrawAABB.cpp

+	SSinglePushConstants pc;
+
+	hlsl::float32_t4x4 instanceTransform = getTransformFromAABB(aabb);
+	pc.instance.transform = hlsl::mul(cameraMat, instanceTransform);


promoted_mul to mul 4x4 with 3x4

devshgraphicsprogramming · 2025-09-17T11:19:14Z

src/nbl/ext/DebugDraw/CDrawAABB.cpp

+	std::vector<InstanceData> instances(aabbInstances.size());
+	for (uint32_t i = 0; i < aabbInstances.size(); i++)
+	{
+		auto& inst = instances[i];
+		inst = aabbInstances[i];
+		inst.transform = hlsl::mul(cameraMat, inst.transform);
+	}


can we skip having this temporary vector and do this loop as a lambda we call instead of the memcpy below?

devshgraphicsprogramming · 2025-09-17T11:20:46Z

src/nbl/ext/DebugDraw/CDrawAABB.cpp

+	auto instancesIt = instances.begin();
+	const uint32_t instancesPerIter = streaming->getBuffer()->getSize() / sizeof(InstanceData);
+	using suballocator_t = core::LinearAddressAllocatorST<offset_t>;
+	while (instancesIt != instances.end())


you'll deadlock if the streaming buffer is not large enough, check aabbInstances.size()>instancesPerIter and return false

Isn't that why we loop below this? We update and draw AABBs in batches that fit the streaming buffer size. Or am I misunderstanding how the streaming buffer works?

…ing in params struct

…and batch

devshgraphicsprogramming · 2025-12-03T18:08:36Z

src/nbl/ext/DebugDraw/CMakeLists.txt

+include(${NBL_ROOT_PATH}/cmake/common.cmake)
+
+set(NBL_EXT_INTERNAL_INCLUDE_DIR "${NBL_ROOT_PATH}/include")
+
+set(NBL_EXT_DEBUG_DRAW_H
+    ${NBL_EXT_INTERNAL_INCLUDE_DIR}/nbl/ext/DebugDraw/CDrawAABB.h
+)
+
+set(NBL_EXT_DEBUG_DRAW_SRC
+    "${CMAKE_CURRENT_SOURCE_DIR}/CDrawAABB.cpp"
+)
+
+nbl_create_ext_library_project(
+    DEBUG_DRAW
+    "${NBL_EXT_DEBUG_DRAW_H}"
+    "${NBL_EXT_DEBUG_DRAW_SRC}"
+    "${NBL_EXT_DEBUG_DRAW_EXTERNAL_INCLUDE}"
+    ""
+    ""
+)
+
+# this should be standard for all extensions
+set(_ARCHIVE_ENTRY_KEY_ "nbl/ext/DebugDraw/builtin/hlsl") # then each one has unique archive key
+get_filename_component(_ARCHIVE_ABSOLUTE_ENTRY_PATH_ "${NBL_EXT_INTERNAL_INCLUDE_DIR}" ABSOLUTE)
+get_filename_component(_OUTPUT_DIRECTORY_SOURCE_ "${CMAKE_CURRENT_BINARY_DIR}/src" ABSOLUTE)
+get_filename_component(_OUTPUT_DIRECTORY_HEADER_ "${CMAKE_CURRENT_BINARY_DIR}/include" ABSOLUTE)
+
+target_compile_definitions(${LIB_NAME} PRIVATE _ARCHIVE_ABSOLUTE_ENTRY_PATH_="${_ARCHIVE_ABSOLUTE_ENTRY_PATH_}")
+target_compile_definitions(${LIB_NAME} PRIVATE _ARCHIVE_ENTRY_KEY_="${_ARCHIVE_ENTRY_KEY_}")
+
+if(NBL_EMBED_BUILTIN_RESOURCES)
+    set(_BR_TARGET_ extDebugDrawbuiltinResourceData)
+
+    LIST_BUILTIN_RESOURCE(RESOURCES_TO_EMBED "common.hlsl")
+    LIST_BUILTIN_RESOURCE(RESOURCES_TO_EMBED "single.vertex.hlsl") # (*)
+    LIST_BUILTIN_RESOURCE(RESOURCES_TO_EMBED "aabb_instances.vertex.hlsl") # (*)
+    LIST_BUILTIN_RESOURCE(RESOURCES_TO_EMBED "aabb_instances.fragment.hlsl") # (*)
+
+    ADD_CUSTOM_BUILTIN_RESOURCES(${_BR_TARGET_} RESOURCES_TO_EMBED "${_ARCHIVE_ABSOLUTE_ENTRY_PATH_}" "${_ARCHIVE_ENTRY_KEY_}" "nbl::ext::debug_draw::builtin" "${_OUTPUT_DIRECTORY_HEADER_}" "${_OUTPUT_DIRECTORY_SOURCE_}")
+    LINK_BUILTIN_RESOURCES_TO_TARGET(${LIB_NAME} ${_BR_TARGET_})
+endif()
+
+add_library(Nabla::ext::DebugDraw ALIAS ${LIB_NAME})


@AnastaZIuk review the Cmake please

devshgraphicsprogramming · 2025-12-03T18:09:44Z

src/nbl/ext/CMakeLists.txt

+if(NBL_BUILD_DEBUG_DRAW)
+    add_subdirectory(DebugDraw)
+    set(NBL_EXT_DEBUG_DRAW_INCLUDE_DIRS
+        ${NBL_EXT_DEBUG_DRAW_INCLUDE_DIRS}
+        PARENT_SCOPE
+    )
+    set(NBL_EXT_DEBUG_DRAW_LIB
+        ${NBL_EXT_DEBUG_DRAW_LIB}
+        PARENT_SCOPE
+    )
+endif()
+


@AnastaZIuk review please

devshgraphicsprogramming · 2025-12-15T06:28:33Z

conflict on examples_tests needs resolving (probably just merge master to both branches)

devshgraphicsprogramming · 2025-12-17T17:50:21Z

include/nbl/ext/DebugDraw/CDrawAABB.h

+
+            DrawMode drawMode = ADM_DRAW_BOTH;
+
+            core::smart_refctd_ptr<video::IUtilities> utilities;


it absolutely silly to keep the utilities around, when all you need from it is the logical device and logger

logger should be configurable and you should ask for it here (you can default it to utilities' if its nullptr), it shouldn't be stolen from utilities by default.

if you need utilities then place along side the transfer queue in SCreationParameters

the logical device needs to be neither in SCachedCreationParameters or SCreationParameters, should be extracted and kept as a member.

devshgraphicsprogramming · 2025-12-17T17:50:47Z

include/nbl/ext/DebugDraw/CDrawAABB.h

+                assert(bool(assetManager));
+                assert(bool(assetManager->getSystem()));
+                assert(bool(utilities));
+                assert(bool(transfer));
+                assert(bool(renderpass));


this is NOT how one validates!

the only assert thats okay out of this bunch is

assert(bool(assetManager->getSystem()));

devshgraphicsprogramming · 2025-12-17T17:57:17Z

include/nbl/ext/DebugDraw/CDrawAABB.h

+            core::smart_refctd_ptr<video::IGPUPipelineLayout> singlePipelineLayout;
+            core::smart_refctd_ptr<video::IGPUPipelineLayout> batchPipelineLayout;


should be default initialized to nullptr, since you provide fallbacks/defaults if null

devshgraphicsprogramming · 2025-12-17T17:58:23Z

include/nbl/ext/DebugDraw/CDrawAABB.h

+        struct DrawParameters
+        {
+            video::IGPUCommandBuffer* commandBuffer = nullptr;
+            hlsl::float32_t4x4 cameraMat = hlsl::float32_t4x4(1);


don't do defaults for camera matrix, makes no sense

devshgraphicsprogramming · 2025-12-17T17:58:54Z

include/nbl/ext/DebugDraw/CDrawAABB.h

+            return true;
+        }
+
+        static hlsl::float32_t3x4 getTransformFromAABB(const hlsl::shapes::AABB<3, float>& aabb);


this should be inline too

keptsecret added 8 commits July 1, 2025 11:04

Merge branch 'mesh_loaders' into new_debug_draw

865e606

latest example

ca86128

merge master, fix conflicts

0ae3da2

latest example

cd2ef95

merge master

fe55bd7

added debug draw aabb extension, moved from ex

98ccfb2

removed todos

a755514

support hlsl AABBs, also OBBs with transform

473592b