-
Notifications
You must be signed in to change notification settings - Fork 21
Root/Push constants #22
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
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
0f03764
to
a8ac101
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).