-
Notifications
You must be signed in to change notification settings - Fork 652
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::allocated::Allocated and vkb::allocated::HPPAllocated into vkb::allocated::Allocated<bindingType> #1193
base: main
Are you sure you want to change the base?
Conversation
9d862e1
to
941c9ee
Compare
… vkb::allocated::Allocated<bindingType>
941c9ee
to
8519880
Compare
Could you add some comments to the allocated class? I'm having a hard time understand what it does and why we need it (instead of e.g. simply using a buffer class). Having some comment for the class would make things easier to understand. |
Yeah, that class never was properly commented, so it's up to the original author. As someone who is not doing C++ on a professional basis it's getting harder and harder for me to understand and use the framework, and having proper code comments would really help. |
I can create a PR improving the comments for the the allocated class and it's children. Should I create it against the main branch or against your branch @asuessenbach ? |
@jherico Would be great, if you would comment against my branch. |
@asuessenbach so, the purpose behind breaking up If I can try to merge the two classes in the header in my PR, or I can just leave it as is (likely with a note in the |
I also see the builder base class has been moved to its own file, but even in that file it should probably still be in the |
Would be great, if you would. But that could also be done after this PR has been merged.
If you think, it's worth to move the builder base class into the |
…dition (#1) * Adding documentation to the allocated class hierarchy, base classes edition * fixup! Adding documentation to the allocated class hierarchy, base classes edition * fixup! Adding documentation to the allocated class hierarchy, base classes edition * fixup! fixup! Adding documentation to the allocated class hierarchy, base classes edition
7f4a758
to
f56f7bd
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.
LGTM. Tested on Win11 with an RTX 4070, full batch and no issues.
Would be really helpful if this was rebased, as batch mode crashes for me without. |
7736155
to
4327fe6
Compare
… vkb::allocated::Allocated<bindingType>
…dition (#1) * Adding documentation to the allocated class hierarchy, base classes edition * fixup! Adding documentation to the allocated class hierarchy, base classes edition * fixup! Adding documentation to the allocated class hierarchy, base classes edition * fixup! fixup! Adding documentation to the allocated class hierarchy, base classes edition
…kan-Samples into unify_Allocated
Description
Next class unified between C- and C++-bindings.
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:
If this PR contains framework changes:
batch
command line argument to make sure all samples still work properlySample Checklist
If your PR contains a new or modified sample, these further checks must be carried out in addition to the General Checklist: