-
Notifications
You must be signed in to change notification settings - Fork 40
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
fix support for read of 3D images with unnormalised sampler #614
Conversation
5ecd83c
to
f07fe59
Compare
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.
Fine with the approach. A couple of (suggested) name tweaks would make the code much easier to read and maintain.
@@ -237,7 +245,8 @@ struct cvk_kernel_argument_values { | |||
} | |||
|
|||
if (m_entry_point->has_pod_arguments() || | |||
m_entry_point->has_image_metadata()) { | |||
m_entry_point->has_image_metadata() || | |||
m_entry_point->has_sampler_metadata()) { |
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's really time we refactor push constant management to make it more readable. In a lot of places, they're still named "POD". Best done as a standalone changes.
sizeof(uint32_t) - | ||
push_constant_range.offset; | ||
push_constant_range.size = | ||
offset + sizeof(uint32_t) - push_constant_range.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.
This code is now screaming "refactor me" :). To be done as a standalone change.
f07fe59
to
4d5f8d1
Compare
Set the sampler mask in the push constant, but also create a new sampler with normalised coordinate if not already the case or already created.
4d5f8d1
to
d707806
Compare
This PR will need to have clspv and spirv-headers updated to go it |
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.
LGTM
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.
LGTM. To be able to update clspv, we are going to need to get #609 in first.
This PR depends on google/clspv#1234
We parse the new NormalizedSamplerMaskPushConstant non-semantic instruction from KhronosGroup/SPIRV-Headers#377.
Set the sampler mask in the push constant, but also create a new sampler with normalised coordinate if not already the case or already created.