Allocated refactor #906
Allocated refactor #906
Conversation
jherico
force-pushed
the
allocated-refactor
branch
from
d48412c to
23c40a2
Compare
jherico
force-pushed
the
allocated-refactor
branch
from
1afec04 to
29f68b5
Compare
There was a problem hiding this comment.
I'm seeing these validation errors multiple times when I run virtually any sample, followed by a driver crash.
[error] [framework/core/instance.cpp:50] 357932245 - VUID-VkImageSubresourceRange-levelCount-01720: Validation Error: [ VUID-VkImageSubresourceRange-levelCount-01720 ] Object 0: handle = 0xa000000000a, type = VK_OBJECT_TYPE_IMAGE; | MessageID = 0x15559cd5 | vkCreateImageView(): pCreateInfo->subresourceRange.levelCount is zero. The Vulkan spec states: If levelCount is not VK_REMAINING_MIP_LEVELS, it must be greater than 0 (https://www.khronos.org/registry/vulkan/specs/1.3-extensions/html/vkspec.html#VUID-VkImageSubresourceRange-levelCount-01720)
[error] [framework/core/instance.cpp:50] 1022674482 - VUID-VkImageSubresourceRange-layerCount-01721: Validation Error: [ VUID-VkImageSubresourceRange-layerCount-01721 ] Object 0: handle = 0xa000000000a, type = VK_OBJECT_TYPE_IMAGE; | MessageID = 0x3cf4c632 | vkCreateImageView(): pCreateInfo->subresourceRange.layerCount is zero. The Vulkan spec states: If layerCount is not VK_REMAINING_ARRAY_LAYERS, it must be greater than 0 (https://www.khronos.org/registry/vulkan/specs/1.3-extensions/html/vkspec.html#VUID-VkImageSubresourceRange-layerCount-01721)
[error] [framework/core/instance.cpp:50] -1968073940 - VUID-VkImageViewCreateInfo-imageViewType-04973: Validation Error: [ VUID-VkImageViewCreateInfo-imageViewType-04973 ] Object 0: handle = 0xa000000000a, type = VK_OBJECT_TYPE_IMAGE; | MessageID = 0x8ab1932c | vkCreateImageView(): pCreateInfo->subresourceRange.layerCount (0) must be 1 when using viewType VK_IMAGE_VIEW_TYPE_2D (try looking into VK_IMAGE_VIEW_TYPE_*_ARRAY). The Vulkan spec states: If viewType is VK_IMAGE_VIEW_TYPE_1D, VK_IMAGE_VIEW_TYPE_2D, or VK_IMAGE_VIEW_TYPE_3D; and subresourceRange.layerCount is not VK_REMAINING_ARRAY_LAYERS, then subresourceRange.layerCount must be 1 (https://www.khronos.org/registry/vulkan/specs/1.3-extensions/html/vkspec.html#VUID-VkImageViewCreateInfo-imageViewType-04973)
|
This sadly doesn't build for me, so I can't test it:
I'm on Windows with VS2022 17.8.7 |
jherico
force-pushed
the
allocated-refactor
branch
from
1b4461b to
2a6745b
Compare
|
@SaschaWillems @gary-sweet should be ready for testing and review. |
There was a problem hiding this comment.
Builds fine for me now, but it breaks some samples. E.g. the ray tracing extended and reflection samples now crashes for me following this error:
[error] [framework\core\instance.cpp:50] -330527817 - VUID-vkMapMemory-memory-00682: Validation Error: [ VUID-vkMapMemory-memory-00682 ] Object 0: handle = 0x84785e00000001b9, type = VK_OBJECT_TYPE_DEVICE_MEMORY; | MessageID = 0xec4c8bb7 | vkMapMemory(): Mapping memory without VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT set. Memory has type 0 which has properties VkMemoryPropertyFlags(0). The Vulkan spec states: memory must have been created with a memory type that reports VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT (https://www.khronos.org/registry/vulkan/specs/1.3-extensions/html/vkspec.html#VUID-vkMapMemory-memory-00682)
[error] [framework\core/allocated.h:207] Detected Vulkan error: ERROR_MEMORY_MAP_FAILED
Probably need to to test all samples and also on different platforms to make sure we don't break anything with this.
|
@SaschaWillems I've fixed the ray tracing examples to use the builder pattern but retain the usage flags used in the old code. Regarding the buffers in question I guess my question is, since these buffers are only written once, why not just use straight up
I've now tested all samples and verified that they all produce output and no new validation errors as far as I can tell (the HLSL examples produce validation errors on main, and the |
jherico
force-pushed
the
allocated-refactor
branch
3 times, most recently
from
e757747 to
3370698
Compare
|
This all seems to work ok on my platform now, but I notice there is a clang format check failure so I can't approve it yet. |
jherico
force-pushed
the
allocated-refactor
branch
from
e239951 to
425e1c7
Compare
There was a problem hiding this comment.
I added some comments to one of the samples I did. In general I'm fine with refactoring to make things better/easier, but such refactors shouldn't do fundamental changes to existing samples mainly for the sake of simplifying/unifying things. With many of the samples I write I'm deliberately explicit so people don't need to poke around the framework to see how things are done. And this PR undoes at least some of that, which I'm not too happy with.
| color_image_view.subresourceRange.layerCount = 1; | ||
| color_image_view.image = storage_image.image; | ||
| VK_CHECK(vkCreateImageView(get_device().get_handle(), &color_image_view, nullptr, &storage_image.view)); | ||
| vkb::core::ImageBuilder image_builder{{width, height, 1}}; |
There was a problem hiding this comment.
I'd prefer to roll this back. The whole reason I made this (sample) so explicit, was so that people could easily adopt that code. These changes add a new layer of abstraction (on top of the framework) that would make it harder for people to follow/adopt this sample.
| /* | ||
| Gets the device address from a buffer that's needed in many places during the ray tracing setup | ||
| */ | ||
| uint64_t RaytracingBasic::get_buffer_device_address(VkBuffer buffer) |
There was a problem hiding this comment.
Same as above. There is a reason I made all of this very explicit.
|
And while I'm in general a fan of the builder pattern, I'm kinda skeptical. Afaik we don't use it elsewhere and it kinda feels odd to introduce (yet) another pattern for an already convoluted and hard to maintain framework. Not sure on this, what do the other reviewers think of that approach? |
| const VkExtent3D image_extent = {width, height, 1}; | ||
| linked_list_head_image = std::make_unique<vkb::core::Image>(get_device(), image_extent, VK_FORMAT_R32_UINT, VK_IMAGE_USAGE_STORAGE_BIT | VK_IMAGE_USAGE_TRANSFER_DST_BIT, VMA_MEMORY_USAGE_GPU_ONLY, VK_SAMPLE_COUNT_1_BIT); | ||
| linked_list_head_image_view = std::make_unique<vkb::core::ImageView>(*linked_list_head_image, VK_IMAGE_VIEW_TYPE_2D, VK_FORMAT_R32_UINT); | ||
| vkb::core::ImageBuilder builder{width, height}; |
There was a problem hiding this comment.
I don't see why or how this is better than before? I personally prefer the way it was.
There was a problem hiding this comment.
The current image constructor looks like this:
Image(
Device const &device,
const VkExtent3D &extent,
VkFormat format,
VkImageUsageFlags image_usage,
VmaMemoryUsage memory_usage = VMA_MEMORY_USAGE_AUTO,
VkSampleCountFlagBits sample_count = VK_SAMPLE_COUNT_1_BIT,
uint32_t mip_levels = 1,
uint32_t array_layers = 1,
VkImageTiling tiling = VK_IMAGE_TILING_OPTIMAL,
VkImageCreateFlags flags = 0,
uint32_t num_queue_families = 0,
const uint32_t *queue_families = nullptr);Even if you ignore the Device argument as being standard to pretty much all the VK resource ctors, that's still a dozen parameters with more than half of them being defaulted. Most style checkers I know start flagging at anything more than 7 ctor paramers.
if you consider Image(device, extent, format, VK_IMAGE_USAGE_SAMPLED_BIT, VMA_MEMORY_USAGE_AUTO, VK_SAMPLE_COUNT_1_BIT, 1, 6), it's not immediately obvious to a downstream consumer of these examples looking at the code what 1 and 6 mean in this context, or why the sample count is set when it hasn't been in prior image creation code. It's only by already knowing the layout of the constructor that you can understand these values.
The builder pattern allows...
- reasonable defaults for values that are infrequently changed, such as the tiling or sample count
- access to ALL of the possible values you might want to modify, which the current ctor does not
- In particular, the
VmaMemoryUsageflag is the only part ofVmaAllocationCreateInfothat can be customized through this parameter, whileVmaAllocationCreateFlags, which is likely something consumers would want to change far more often, is not
- In particular, the
- The builder pattern makes it perfectly clear to readers of the examples what the meaning of the values being set are, while constructor param doesn't unless you're looking at the constructor definition.
I have no doubt that you can find places in the code where the builder pattern ends up appearing "just as complex" or even consumes more lines of code as the existing constructor based approach, but IMO, in places like this we're sacrificing brevity for readability and clarity to consumers of the example code as a learning tool.
Granted I made this change before I added the build methods to the builder and the with_vma_usage call is almost certainly superfluous, so this could be re-written as
linked_list_head_image = vkb::core::ImageBuilder{width, height}
.with_format(VK_FORMAT_R32_UINT)
.with_usage(VK_IMAGE_USAGE_STORAGE_BIT | VK_IMAGE_USAGE_TRANSFER_DST_BIT)
.build_unique(device);Which would be slightly more compact and avoid the temporary named builder variable. Would that make you happier?
| image_usage, | ||
| VMA_MEMORY_USAGE_GPU_ONLY, | ||
| VK_SAMPLE_COUNT_1_BIT); | ||
| vkb::core::ImageBuilder builder{image_extent}; |
There was a problem hiding this comment.
Same here. At least for me, the builder pattern seems out of place in here.
There was a problem hiding this comment.
I could replace the entire block with
shading_rate_image = vkb::core::ImageBuilder{image_extent}
.with_format(VK_FORMAT_R8_UINT)
.with_usage(VK_IMAGE_USAGE_FRAGMENT_SHADING_RATE_ATTACHMENT_BIT_KHR | VK_IMAGE_USAGE_TRANSFER_DST_BIT)
.build_unique(*device);
shading_rate_image_compute = vkb::core::ImageBuilder{image_extent}
.with_format(VK_FORMAT_R8_UINT)
.with_usage(VK_IMAGE_USAGE_STORAGE_BIT | VK_IMAGE_USAGE_TRANSFER_DST_BIT)
.build_unique(*device);and eliminate the need for a lambda completely, making the code shorter overall. Is that easier to follow?
jherico
force-pushed
the
allocated-refactor
branch
3 times, most recently
from
3d409c5 to
c27ac1b
Compare
|
@SaschaWillems I have removed all use of the builder pattern from the code (except the internal usage in the image and buffer classes to simplify constructors. I have left, and expanded the use of convenience functions for creating staging buffers, as well as noticed and corrected some minor bugs in populating staging buffers where they fail to flush before unmapping. |
|
Any idea why the profiles sample cpp is displayed as completely changed (all lines) in this PR? It only looks like the copyright has changed, but I'm not able to see any other change due to the whole file being displayed as different. |
|
It's the same for a lot of other files too, which makes it nigh impossible to review these changes:
|
jherico
force-pushed
the
allocated-refactor
branch
from
c27ac1b to
52f81f8
Compare
|
@SaschaWillems It looks like some of the files got converted from LF to CRLF. Most of the files in the repository are LF, but a few are CRLF (prior to this change). I've updated the commit to revert the line ending changes. I must have misconfigured something in git, because I thought I had my system configured to checkout and commit LF, always (autocrlf=false). I'll review my settings and try to figure out how this happened. @tomadamatkinson might be worthwhile to add to the pre-commit hook something that would verify that none of the line-endings have changed for existing files. These are the only 5 files on |
jherico
force-pushed
the
allocated-refactor
branch
from
52f81f8 to
f1049d1
Compare
jherico
force-pushed
the
allocated-refactor
branch
from
f1049d1 to
4a06788
Compare
|
@jherico I'm a bit baffled, but this seems to have regressed 32 bit builds (both arm and x86), where 64 bit build without issues: |
|
@kanavin I'll try to locally reproduce and find a fix today. |
|
ok, so the problem here is that on 32 bit architectures the handles still need to be 64 bit. So while on 64 bit architectures This means that all the template specializations like this: template <>
constexpr VkObjectType get_object_type(const VkImage &handle)
{ ... }
template <>
constexpr VkObjectType get_object_type(const VkImageView &handle)
{ ... }have the exact same signature and can't be differentiated by the compiler the way they can in 64-bit land. @tomadamatkinson should a 32-bit build be added to the CI configs? Two distinct PR's of mine appear to have caused regressions for |
|
So, when I run Looking at the readme, I'm unclear on if 32-bit builds are supported since all of the instructions seem to explicitly specify 64-bit arch. I'm working on a branch that resolves some of the 32-bit issues I've seen so far, but I'm not touching the third-party build issues other than for astc. |
I was worried that in the absence of 32 bit CI that gates all changes, fixing one issue will uncover another (with no clear end in sight). Please do fix what's in your 'jurisdiction' and I'll see what can be done about the rest (my interest is primarily 32 bit linux, especially arm and risc-v which are still used to ship products). |
Description
This PR is currently built on top of #907 and #901 and should not be merged until they're approved and merged.Migrate the code common to the objects using the VMA library to a single location, mostly the new
allocated.hheader and template types. The four classes,vkb::core::Buffer,vkb::core::Image,vkb::core::HPPBufferand ``vkb::core::HPPImage` are now all implemented in terms of common base classes.The new classes prefer the use of a builder pattern for construction, with reasonable defaults for some values set in the image builder types, reducing the cognitive load of using them as well as the complexity of the image constructors. In a future PR the existing ctors will be marked deprecated and all the samples referencing them will be updated to use the builder pattern.
The buffer types have new static methods intended to be used to create short-lived staging buffers containing data. These static methods include convenience signatures to make it easy to upload containers full of data... so for instance, this code
reduces to
Fixes #900
I'm struggling to 100% test the samples as
fragment_shading_rate_dynamiccurrently crashes on my desktop, even on the main branch and the hlsl shader examples both produce validation warnings.Fixed #941
Partial fix the the example noted in this bug and at least one other place, but there should probably be an assert if unmap is called without flush ever having been called (regardless of whether the memory is coherent).
General Checklist:
Please ensure the following points are checked:
Note: The Samples CI runs a number of checks including: