-
Notifications
You must be signed in to change notification settings - Fork 618
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
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
@@ -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
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 tend to use the complete namespace. I think, it's easier to mentally locate the used entity.
If you want, I can remove that vbk::
here, though.
…t by const reference.
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.
Compiles and runs fine for me. Code changes look fine to me 👍🏻
Merging with 3 approvals. Thanks! |
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: