-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Root/Push constants #22
base: master
Are you sure you want to change the base?
Conversation
# Conflicts: # src/FlyCube/CommandList/DXCommandList.h Fix root constants # Conflicts: # src/FlyCube/CommandList/DXCommandList.h
No need to worry about metal, just place If you're interested, there are 2 problems with the metal
|
@@ -104,5 +104,7 @@ class CommandList : public QueryInterface { | |||
uint32_t query_count, | |||
const std::shared_ptr<Resource>& dst_buffer, | |||
uint64_t dst_offset) = 0; | |||
virtual void SetGraphicsConstant(uint32_t root_parameter_index, uint32_t value, uint32_t byte_offset) = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is better to use PushConstants, to match it with [[vk::push_constant]]
in hlsl.
void PushConstants(ShaderType shader_type, uint32_t dst_offset, const void* data, uint32_t size)
In Vulkan we can have at most 1 push_constant block in each shader, I suggest to add the same limitation in FlyCube. In this case ShaderType
+ active Pipeline
will probably be enough to find root_parameter_index in DirectX12 implementation.
In my opinion PushConstants
is enough to for all cases, but I don't mind having PushConstant
as well.
void PushConstant(ShaderType shader_type, uint32_t dst_offset, uint32_t value)
@@ -732,6 +732,16 @@ void DXCommandList::ResolveQueryData(const std::shared_ptr<QueryHeap>& query_hea | |||
D3D12_RESOURCE_STATE_UNORDERED_ACCESS, 0)); | |||
} | |||
|
|||
void DXCommandList::SetGraphicsConstant(uint32_t root_parameter_index, uint32_t value, uint32_t byte_offset) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the current implementation, how do you know which root_parameter_index
value you need to pass?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The root parameter index is the index of the binding set/layout set in the list given to CreateBindingSetLayout or CreateBindingSet.
void VKCommandList::SetGraphicsConstant(uint32_t root_parameter_index, uint32_t value, uint32_t byte_offset) | ||
{ | ||
decltype(auto) pipeline_layout = m_state->GetPipelineLayout(); | ||
m_command_list->pushConstants(pipeline_layout, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You also need to specify pipeline_layout_info.pPushConstantRanges
in VKBindingSetLayout. But it probably won't work without information about push constant size.
@@ -408,6 +408,7 @@ struct BindKey { | |||
uint32_t space = 0; | |||
uint32_t count = 1; | |||
uint32_t remapped_slot = ~0; | |||
bool is_root_constant = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to use ViewType::kPushConstant
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually no, it's better to use ViewType::kConstantBuffer
. Because ShaderReflection
must be able to distinguish ViewType::kPushConstant
from ViewType::kConstantBuffer
, but there is no way to do so in dxil, since dxc ignore [[vk::push_constant]]
when compile to dxil.
But since we still need to pass the size for push_constant, it is better to pass it separately from the BindKey.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I use ViewType::kConstantBuffer
instead of this boolean, how can I know if the root BindKey should go to a root constant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, we can separate them.
struct PushConstant {
ShaderType shader_type = ShaderType::kUnknown;
uint32_t size = 0;
};
std::shared_ptr<BindingSetLayout> CreateBindingSetLayout(const std::vector<BindKey>& descs, const std::vector<PushConstant>& push_constants = {});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what is best to use, just ShaderType or BindKey.
- ShaderType
At most 1 push constant per shader stage to match Vulkan limitation.
Slot and space are 0 because spirv do not save bindings when [[vk::push_constant]] is used. - BindKey
Allow multiple push constant in DirectX12 and Metal, but Vulkan still supports at most 1 push constant per shader stage at slot/space 0 with using [[vk::push_constant]].
More flexible, but harder to use in Vulkan.
According to the documentation, |
3df3b1a
to
9acb37f
Compare
Hello,
I added D3D12 Root constant and Vulkan's push constant to the command buffer API. I don't really have experience writing metal code so I'm not sure what I did works (and I can't test either).