New debug draw extension for AABBs #900
Conversation
include/nbl/ext/DebugDraw/builtin/hlsl/aabb_instances.fragment.hlsl
Outdated
Show resolved
Hide resolved
include/nbl/ext/DebugDraw/builtin/hlsl/aabb_instances.vertex.hlsl
Outdated
Show resolved
Hide resolved
include/nbl/ext/DebugDraw/builtin/hlsl/aabb_instances.vertex.hlsl
Outdated
Show resolved
Hide resolved
include/nbl/ext/DebugDraw/builtin/hlsl/aabb_instances.vertex.hlsl
Outdated
Show resolved
Hide resolved
| 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() | ||
|
|
There was a problem hiding this comment.
approved,
although this snippet which is copied for each ext requires rewrite at some point (dirty), the rewrite shall not be part of this PR
AnastaZIuk
left a comment
AnastaZIuk
left a comment
There was a problem hiding this comment.
left some comments, please address
| static core::smart_refctd_ptr<video::IGPUPipelineLayout> createPipelineLayoutFromPCRange(video::ILogicalDevice* device, const asset::SPushConstantRange& pcRange); | ||
|
|
||
| // creates default pipeline layout for pipeline specified by draw mode (note: if mode==BOTH, returns layout for BATCH mode) | ||
| static core::smart_refctd_ptr<video::IGPUPipelineLayout> createDefaultPipelineLayout(video::ILogicalDevice* device, DrawMode mode = ADM_DRAW_BATCH); |
There was a problem hiding this comment.
the last argument should not be defaulted!
There was a problem hiding this comment.
| ${NBL_DEBUG_DRAW_HLSL_MOUNT_POINT}/common.hlsl | ||
| ${NBL_DEBUG_DRAW_HLSL_MOUNT_POINT}/draw_aabb.unified.hlsl |
There was a problem hiding this comment.
@AnastaZIuk IMHO these should still be embedded and finable in the archive
I'm not saying the SPV precompute needs to add its DEPENDS to the archive, but these files need to be explicitly listed out here
There was a problem hiding this comment.
sure we will keep the source too in post PR
main reason we didn't is build system tooling needs an upgrade to do so (dc thread)
| constexpr std::string_view NBL_EXT_MOUNT_ENTRY = "nbl/ext/DebugDraw"; | ||
|
|
||
| const smart_refctd_ptr<IFileArchive> DrawAABB::mount(smart_refctd_ptr<ILogger> logger, ISystem* system, const std::string_view archiveAlias) | ||
| const smart_refctd_ptr<IFileArchive> DrawAABB::mount(smart_refctd_ptr<ILogger> logger, ISystem* system, video::ILogicalDevice* device, const std::string_view archiveAlias) |
There was a problem hiding this comment.
this function deals with archives and file system, why is ILogicalDevice taken?
There was a problem hiding this comment.
Sorakrit put core::string with key name to validate required file presence, I replaced it with device to use the getter instead
We could also hardcode key name inside since in this case there are no permutations, then we remove ILogicalDevice. Our auto-gen API could actually be smarter and with no permutations have constexpr getters with no args
| constexpr auto MdiSizes = std::to_array<offset_t>({ sizeof(hlsl::float32_t3), sizeof(InstanceData) }); | ||
| // shared nPoT alignment needs to be divisible by all smaller ones to satisfy an allocation from all | ||
| constexpr offset_t MaxAlignment = std::reduce(MdiSizes.begin(), MdiSizes.end(), 1, [](const offset_t a, const offset_t b)->offset_t {return std::lcm(a, b); }); | ||
| constexpr offset_t MaxAlignment = sizeof(InstanceData); |
There was a problem hiding this comment.
alignof not sizeof
There was a problem hiding this comment.
you should also assert that the alignment is <= sizeof(InstanceData) due to the roundups and divisions you do later in the loop
| if (srcIt == aabbInstances.end()) | ||
| break; |
There was a problem hiding this comment.
this should never happen, assert srcIt<=end() before the ++ or srcIt<end()
Signed-off-by: Corey <corey.w108@gmail.com>
No description provided.