-
Notifications
You must be signed in to change notification settings - Fork 594
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
Unify vkb::Subpass and vkb::rendering::HPPSubpass into vkb::rendering::Subpass<bindingType> #1035
base: main
Are you sure you want to change the base?
Conversation
…::Subpass<bindingType>
b4db8f9
to
c71b077
Compare
This does work fine for me (tested a few samples with the batch run), but I',m kinda unsure about the actual interface. Once we have unified the repo will we have to add the binding type to each and every class from the framework? If so, I fear that this will make the code hard to read. e.g. comparing this: std::vector<std::unique_ptr<vkb::Subpass>> subpasses{}; with: std::vector<std::unique_ptr<vkb::rendering::Subpass<vkb::BindingType::C>>> subpasses{}; Imo readability suffers a lot. Is it possible to somehow omit the binding type unless one explicitly wants to use the CPP variants? |
I agree, the resulting types are somewhat longer than before.
which would reduce the aforementioned complexity a bit: |
…ubpass<vkb::BindingType>
d224ce2
to
9cf86f6
Compare
void CommandBuffer::begin_render_pass(const RenderTarget &render_target, | ||
const std::vector<LoadStoreInfo> &load_store_infos, | ||
const std::vector<VkClearValue> &clear_values, | ||
const std::vector<std::unique_ptr<vkb::rendering::SubpassC>> &subpasses, |
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.
FWIW, some compilers will have issues with >> next to each other. I don't know if it's been fixed in all compilers or not as I just got into a habit of putting a space between the two. This compiles and works in Linux; and CI passes so this is probably a non-issue.
|
||
Subpass(const Subpass &) = delete; | ||
Subpass(Subpass &&) = default; | ||
virtual ~Subpass() = default; |
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.
Shouldn't this be override/final instead of virtual here?
template <vkb::BindingType bindingType> | ||
inline void Subpass<bindingType>::set_color_resolve_attachments(std::vector<uint32_t> color_resolve) | ||
{ | ||
color_resolve_attachments = color_resolve; |
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.
copy a vector on each call. maybe move is better?
template <vkb::BindingType bindingType> | ||
inline void Subpass<bindingType>::set_input_attachments(std::vector<uint32_t> input) | ||
{ | ||
input_attachments = input; |
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.
copy vector on each call. check if move is correct.
template <vkb::BindingType bindingType> | ||
inline void Subpass<bindingType>::set_output_attachments(std::vector<uint32_t> output) | ||
{ | ||
output_attachments = output; |
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.
copy vector on each call. check if move is correct.
@@ -277,7 +281,7 @@ void CommandBuffer::bind_index_buffer(const core::Buffer &buffer, VkDeviceSize o | |||
vkCmdBindIndexBuffer(get_handle(), buffer.get_handle(), offset, index_type); | |||
} | |||
|
|||
void CommandBuffer::bind_lighting(LightingState &lighting_state, uint32_t set, uint32_t binding) | |||
void CommandBuffer::bind_lighting(vkb::rendering::LightingStateC &lighting_state, uint32_t set, uint32_t binding) |
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.
we're already in the vkb namespace. It's safe to use rendering::LightingStateC
Description
Build tested on Win10 with VS2022. Run tested on Win10 with NVidia GPU.
General Checklist:
Please ensure the following points are checked:
Note: The Samples CI runs a number of checks including:
Sample Checklist
If your PR contains a new or modified sample, these further checks must be carried out in addition to the General Checklist: