Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ray Tracing Pipeline #820

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

kevyuu
Copy link

@kevyuu kevyuu commented Jan 21, 2025

Description

Implement Ray Tracing Pipeline

Testing

Using example_tests

TODO list:

  • Implement Ray Tracing Pipeline Creation
  • Implement Ray Tracing Pipeline Binding Command
  • Implement Trace Ray Command

@devshgraphicsprogrammingjenkins
Copy link
Contributor

[CI]: Can one of the admins verify this patch?

@kevyuu kevyuu force-pushed the raytracing_pipeline branch 2 times, most recently from c41848f to 7757888 Compare January 21, 2025 07:24
Comment on lines 14 to 15
class CVulkanRayTracingPipeline final : public IGPURayTracingPipeline
{

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't indent just because of a namespace

Comment on lines +828 to +857
class IGPUCommandPool::CTraceRaysCmd final : public IFixedSizeCommand<CTraceRaysCmd>
{
public:
CTraceRaysCmd(
core::smart_refctd_ptr<const IGPUBuffer>&& raygenGroupBuffer,
core::smart_refctd_ptr<const IGPUBuffer>&& hitGroupsBuffer,
core::smart_refctd_ptr<const IGPUBuffer>&& missGroupsBuffer,
core::smart_refctd_ptr<const IGPUBuffer>&& callableGroupsBuffer) :
m_raygenGroupBuffer(raygenGroupBuffer),
m_hitGroupsBuffer(hitGroupsBuffer),
m_missGroupsBuffer(missGroupsBuffer),
m_callableGroupsBuffer(callableGroupsBuffer) {}


private:
core::smart_refctd_ptr<const IGPUBuffer> m_raygenGroupBuffer;
core::smart_refctd_ptr<const IGPUBuffer> m_hitGroupsBuffer;
core::smart_refctd_ptr<const IGPUBuffer> m_missGroupsBuffer;
core::smart_refctd_ptr<const IGPUBuffer> m_callableGroupsBuffer;
};


class IGPUCommandPool::CBindRayTracingPipelineCmd final : public IFixedSizeCommand<CBindRayTracingPipelineCmd>
{
public:
CBindRayTracingPipelineCmd(core::smart_refctd_ptr<const IGPURayTracingPipeline>&& pipeline) : m_pipeline(std::move(pipeline)) {}

private:
core::smart_refctd_ptr<const IGPURayTracingPipeline> m_pipeline;
};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very nice you got the hang of it!

Comment on lines +47 to +53
case asset::IShader::E_SHADER_STAGE::ESS_RAYGEN: [[fallthrough]];
case asset::IShader::E_SHADER_STAGE::ESS_ANY_HIT: [[fallthrough]];
case asset::IShader::E_SHADER_STAGE::ESS_CLOSEST_HIT: [[fallthrough]];
case asset::IShader::E_SHADER_STAGE::ESS_MISS: [[fallthrough]];
case asset::IShader::E_SHADER_STAGE::ESS_INTERSECTION: [[fallthrough]];
case asset::IShader::E_SHADER_STAGE::ESS_CALLABLE:
return L"lib";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alichraghi what were the old problems when making a lib stage shader with our HLSL stuff?

I remember we only had issues with the Compute and Graphics pipelines using it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

microsoft/DirectXShaderCompiler#6959 got closed but then DXC was crashing when i updated it but seems like it's actually usable now

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok lets reheat your PR with the lib target and test it on an example that really needs unscrewing of multiple preprocessor and template compilations (maybe arithmetic unit test?)

Comment on lines 38 to 48
{".comp.hlsl",IShader::E_SHADER_STAGE::ESS_COMPUTE},
{".mesh.hlsl",IShader::E_SHADER_STAGE::ESS_MESH},
{".task.hlsl",IShader::E_SHADER_STAGE::ESS_TASK},
{".rgen.hlsl",IShader::E_SHADER_STAGE::ESS_RAYGEN},
{".rahit.hlsl",IShader::E_SHADER_STAGE::ESS_ANY_HIT},
{".rchit.hlsl",IShader::E_SHADER_STAGE::ESS_CLOSEST_HIT},
{".rmiss.hlsl",IShader::E_SHADER_STAGE::ESS_MISS},
{".rint.hlsl",IShader::E_SHADER_STAGE::ESS_INTERSECTION},
{".rcall.hlsl",IShader::E_SHADER_STAGE::ESS_CALLABLE},
};
auto shaderStage = IShader::E_SHADER_STAGE::ESS_UNKNOWN;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah one thing that #754 was missing was something like this, to let .hlsl only (no stage.hlsl) extension map to library all-or-library stage

Comment on lines 120 to 150
template<typename BufferType>
struct SStridedBufferRegion
{
static constexpr inline size_t WholeBuffer = ~0ull;

size_t offset = 0ull;
size_t stride = 0;
size_t size = WholeBuffer;
core::smart_refctd_ptr<BufferType> buffer = nullptr;


inline operator SStridedBufferRegion<const BufferType>&() {return *reinterpret_cast<SStridedBufferRegion<const BufferType>*>(this);}
inline operator const SStridedBufferRegion<const BufferType>&() const {return *reinterpret_cast<const SStridedBufferRegion<const BufferType>*>(this);}

explicit inline operator bool() const {return isValid();}

inline bool isValid() const
{
if (!buffer || offset>=buffer->getSize() || size==0ull || stride>buffer->getSize())
return false;
return actualSize()<=buffer->getSize()-offset;
}

inline size_t actualSize() const
{
return size!=WholeBuffer ? size:buffer->getSize();
}
inline bool operator==(const SStridedBufferRegion<const BufferType>& rhs) const { return buffer==rhs.buffer && offset==rhs.offset && actualSize()==rhs.actualSize() && stride==rhs.stride; }
inline bool operator!=(const SStridedBufferRegion<const BufferType>& rhs) const { return !operator==(rhs); }
};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to have this new thing in the API, take an SBufferOffset + stride in the functions that take this



#include "nbl/asset/IShader.h"
#include "nbl/asset/RasterizationStates.h"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you need this header for?

Comment on lines 13 to 15
namespace nbl::asset
{
struct SShaderGroupsParams

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't indent just because of a namespace

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also such struct should be nested inside IRayTracingPipelineBase

Comment on lines 32 to 34
core::vector<SGeneralGroup> missGroups;
core::vector<SHitGroup> hitGroups;
core::vector<SGeneralGroup> callableGroups;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vectors (and other containers that need an allocator) are banned from structs (esp ones used for input), use spans instead (better yet, spans of const)


struct SCachedCreationParams final
{
SShaderGroupsParams shaderGroups;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the groups can't be cached verbatim, need to be copied into local storage (because they must use spans

Comment on lines 60 to 63
public:
struct SCreationParams : IPipeline<PipelineLayoutType>::SCreationParams
{
protected:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I require extra indentation for public/protected/private and stuff in them

Comment on lines 527 to 531
bool IGPUCommandBuffer::traceRays(const asset::SStridedBufferRegion<IGPUBuffer>& raygenGroupRegion,
const asset::SStridedBufferRegion<IGPUBuffer>& missGroupsRegion,
const asset::SStridedBufferRegion<IGPUBuffer>& hitGroupsRegion,
const asset::SStridedBufferRegion<IGPUBuffer>& callableGroupsRegion,
uint32_t width, uint32_t height, uint32_t depth);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The strided buffer it better expressed as bufferbinding (buffeR+offset struct) and a stride as a separate thing

P.S. what about traceRaysIndirect and the command that sets the stack size?

Comment on lines 11 to 13
namespace
{
VkPipelineBindPoint vkCast(asset::E_PIPELINE_BIND_POINT bindPoint)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. why the private namespace
  2. I don't remember calling the all my other functions vkCast I'm sure I included some of the Vk enum in the name

Comment on lines 889 to 891
// Because binding of the Gfx pipeline can happen outside of a Renderpass Scope,
// we cannot check renderpass-pipeline compatibility here.
// And checking before every drawcall would be performance suicide.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this comment makes no sense here, it only makes sense in bindGraphicsPipeline

@@ -1760,7 +1760,6 @@
"type": "uint32_t",
"name": "shaderGroupHandleSize",
"value": 32,
"expose": "DISABLE",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do not expose, its a constexpr essentially, you can add it as a constexpr static inline here:

@@ -601,6 +601,7 @@ std::unique_ptr<CVulkanPhysicalDevice> CVulkanPhysicalDevice::create(core::smart
logger.log("Not enumerating VkPhysicalDevice %p because it reports limits of exact-type contrary to Vulkan specification!", system::ILogger::ELL_INFO, vk_physicalDevice);
return nullptr;
}
properties.limits.shaderGroupHandleSize = rayTracingPipelineProperties.shaderGroupHandleSize;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines 901 to 907
if (!m_cmdpool->m_commandListPool.emplace<IGPUCommandPool::CBindRayTracingPipelineCmd>(m_commandList, core::smart_refctd_ptr<const IGPURayTracingPipeline>(pipeline)))
{
NBL_LOG_ERROR("out of host memory!");
return false;
}

m_noCommands = false;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be useful to keep the most recently bound pipeline around so we can validate/check stuff like
https://registry.khronos.org/vulkan/specs/latest/html/vkspec.html#VUID-vkCmdTraceRaysKHR-flags-03696

during a call to traceRays and traceRaysIndirect

Comment on lines 1814 to 1816
}

if (!m_cmdpool->m_commandListPool.emplace<IGPUCommandPool::CTraceRaysCmd>(m_commandList,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add checks for:
https://registry.khronos.org/vulkan/specs/latest/html/vkspec.html#VUID-vkCmdTraceRaysKHR-pRayGenShaderBindingTable-03681
https://registry.khronos.org/vulkan/specs/latest/html/vkspec.html#VUID-vkCmdTraceRaysKHR-pRayGenShaderBindingTable-03682
https://registry.khronos.org/vulkan/specs/latest/html/vkspec.html#VUID-vkCmdTraceRaysKHR-stride-03690

and friends

TL;DR check that:

  • buffers have SBT usages
  • their base address (BDA + offset) are aligned to shaderGroupBaseAlignment
  • the stride (if any) is aligned to shaderGroupHandleAlignment
  • the stride is less than maxShaderGroupStride

and leave comments with hyperlinks to VUIDs from the spec (see the beautiful validation of renderpasses).

@kevyuu kevyuu force-pushed the raytracing_pipeline branch from 84fae66 to 98025d3 Compare February 6, 2025 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants