-
Notifications
You must be signed in to change notification settings - Fork 342
Rework the readme #2358
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: main
Are you sure you want to change the base?
Rework the readme #2358
Conversation
|
Is there a reason you want to rework the readme? |
|
Hi @asuessenbach! I've been meaning to do this for a while, actually. I have a list of things that I found a little less than ideal with the current readme:
If you're OK with this, I can proceed with making these changes, naturally subject to your review at the end. I hope to improve the documentation at the end of this exercise. |
|
You're more than welcome to rework the readme! |
9896e08 to
c2a3479
Compare
- Remove HTML hyperlinks; markdown generates automatically - Use backticks \` to demarcate config macros - Remove `VULKAN_HPP_NO_STD_MODULE` option
- `-` instead of `*` - Spaces around lists
86cebc2 to
581d2fe
Compare
- Minor changes to main readme so that links work
- Minor changes in main readme
- Add builder pattern
4a2fb83 to
576fd7a
Compare
|
@asuessenbach, this is still in progress (and hence in draft mode), but do feel free to have a look at the branch to see the direction I'm going in. Hopefully intend to complete this by the end of the year. |
|
How about adding CMake's include(FetchContent)
FetchContent_Declare(vulkan-hpp
GIT_REPOSITORY "https://github.com/KhronosGroup/Vulkan-Hpp.git"
GIT_TAG "v1.4.336" # release tag, commit hash or branch name
GIT_SHALLOW ON)
FetchContent_MakeAvailable(vulkan-hpp) |
|
For the "Build Instructions", it is likely best to separate header generation and sample/test building. At the very least, the build section should state that building is purely for those tasks and that headers can be used by just including them. For merely using the headers (because e.g. they haven't landed in Vulkan-Headers yet), quickly showing how to link against |
|
Both good points @M2-TE. I am focusing mostly on |
- Rework flow; start with vulkan fundamentals - Move things around - Adhere to 1 sentence, 1 line
70f1aba to
293eca2
Compare
- Added TODOs for review - Added section on `std::expected`
- Reformat type traits and vkformat traits lists as tables
|
I'm much happier about the Usage document; probably a few final updates left for the last few sections. At this point, I'd like to ask you guys, @M2-TE and @asuessenbach to review what has been done; I have a lot of After that, I intend to work on migrating the RAII guide to |
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 is a really great PR, fantastic work!
I've added some comments/suggestions where relevant.
and finally revise the building and installation sections
woops, I also added some comments regarding those sections. If it's too early, just mark them as resolved and I'll revisit them once you have finished the section!
|
|
||
| ## Minimum requirements | ||
|
|
||
| The generator, samples, and tests requires a C++11 compiler. The following compilers are known to work, and tested during releases: |
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 generator actually requires a C++20 compiler:
Line 116 in 67ec7c9
| set_target_properties( VulkanHppGenerator PROPERTIES CXX_STANDARD 20 CXX_STANDARD_REQUIRED ON ) |
|
|
||
| 1. Install dependencies: | ||
| - Install CMake and git; ensure both are available from a shell. | ||
| - Install the LunarG Vulkan SDK. |
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 SDK should not be needed for either the generators or the tests/samples. The headers and xml registry are fetched from the Vulkan-Headers submodule.
It might be worth noting here that, in order to build samples, you need additional deps on Ubuntu:
Vulkan-Hpp/.github/workflows/build.yml
Line 79 in 67ec7c9
| sudo apt-get update && sudo apt-get install lsb-release gpg software-properties-common wget |
|
|
||
| If a `clang-format` executable is found by CMake, the define `CLANG_FORMAT_EXECUTABLE` is set accordingly. In that case, the generated headers are formatted using the `.clang-format` file located in the root directory of this project; otherwise, the formatting is left as hard-coded in the generator. | ||
|
|
||
| Use `clang-format` version 21.1.0 to format the generated files. |
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 line comes a bit out of nowhere after the previous paragraph; did you mean to say that you should use this version for PRs? It should probably say either that, or that it is the recommended version in general.
|
|
||
| Vulkan-Hpp provides a `vk::SharedHandle<Type>` interface. For each Vulkan handle type `vk::Type` there is a shared handle `vk::SharedType` which will delete the underlying Vulkan resource upon destruction, e.g. `vk::SharedBuffer` is the shared handle for `vk::Buffer`. | ||
|
|
||
| Unlike `vk::UniqueHandle`, `vk::SharedHandle` takes shared ownership of the resource as well as its parent. This means that the parent handle will not be destroyed until all child resources are deleted. This is useful for resources that are shared between multiple threads or objects. |
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.
Since you were on the topic of multithreading, is it worth mentioning that shared handles are not thread safe? Handling mutexes (mutices?) is up to the user.
| vk::SharingMode::eExclusive, 0, nullptr, vk::ImageLayout::eUndefined}); | ||
| ``` | ||
|
|
||
| #### Designated initialisers |
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.
| #### Designated initialisers | |
| #### Designated initializers |
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.
Sadge. I use British English...
| Vulkan-Hpp provides a per-function dispatch mechanism where each function representing a Vulkan operation accepts as its last parameter a dispatch class. | ||
| This class must provide a callable type for each Vulkan function that is required. | ||
|
|
||
| <!-- TODO: What is this code sample exactly? --> |
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.
Isn't it just a duplicate of the following section?
Instead of VULKAN_HPP_DEFAULT_DISPATCHER it uses vk::detail::DispatchLoaderDynamic directly, using dld and such, rather than defaultDispatchLoaderDynamic.
The information about vulkan-1 is still useful though, so perhaps best merged with the dynamic dispatch section below it?
|
|
||
| - Use your own dynamic loader, which needs to provide a templated function `getProcAddress`. Refer to the implementation of `vk::detail::DynamicLoader` in `vulkan.hpp` for an example. | ||
|
|
||
| <!-- TODO: Add a skeleton implementation of `struct YourDynamicLoader`? --> |
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.
That would be a nice idea. Probably just a stripped down version from what we have in vulkan.hpp is sufficient.
| #if VULKAN_HPP_DISPATCH_LOADER_DYNAMIC == 1 | ||
| VULKAN_HPP_DEFAULT_DISPATCH_LOADER_DYNAMIC_STORAGE | ||
| #endif | ||
|
|
||
| auto main(int argc, char* const argv[]) -> int | ||
| { | ||
| #if ( VULKAN_HPP_DISPATCH_LOADER_DYNAMIC == 1 ) | ||
| // initialize minimal set of function pointers | ||
| VULKAN_HPP_DEFAULT_DISPATCHER.init(); | ||
| #endif |
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 VULKAN_HPP_DISPATCH_LOADER_DYNAMIC == 1 | |
| VULKAN_HPP_DEFAULT_DISPATCH_LOADER_DYNAMIC_STORAGE | |
| #endif | |
| auto main(int argc, char* const argv[]) -> int | |
| { | |
| #if ( VULKAN_HPP_DISPATCH_LOADER_DYNAMIC == 1 ) | |
| // initialize minimal set of function pointers | |
| VULKAN_HPP_DEFAULT_DISPATCHER.init(); | |
| #endif | |
| VULKAN_HPP_DEFAULT_DISPATCH_LOADER_DYNAMIC_STORAGE | |
| auto main(int argc, char* const argv[]) -> int | |
| { | |
| // initialize minimal set of function pointers | |
| VULKAN_HPP_DEFAULT_DISPATCHER.init(); |
Probably best to just use the dynamic dispatcher stuff directly. It's what the example is for, so no need to confuse readers with additional #ifdefs.
| #endif | ||
|
|
||
| auto appInfo = vk::ApplicationInfo("My App", 1, "My Engine", 1, vk::makeApiVersion(1, 0, 0, 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.
| // ... | |
| // ... | |
| // load function pointers with created instance | |
| VULKAN_HPP_DEFAULT_DISPATCHER.init(instance); | |
| // OPTIONAL: load function pointers specifically for a vk::device | |
| VULKAN_HPP_DEFAULT_DISPATCHER.init(device); |
Can't hurt to add the other two inits here as well, right?
| Some functions might provide information that depends on the vulkan version. | ||
| As all functions here operate only on strings, the Vulkan versions are encoded by a string prefixed with "VK_VERSION_", followed by the major and the minor version, separated by an underscore, like this: `VK_VERSION_1_0`. | ||
|
|
||
| <!-- TODO: the descriptions in this table need to be reworded a bit, it's a little hard to follow. --> |
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.
Maybe just use function names and parameters, then add a separate column for return values?
I'm not sure how much more readable that is exactly, but things like the constexpr macro can be stripped out (add another column with checkmarks for which funcs can be constexpr?).
No worries, I can collapse them and work on your suggestions for the usage doc first. Thanks for being so comprehensive :) |
No description provided.