blurrypiano / littleVulkanEngine

Code repo for video tutorial series teaching Vulkan and computer graphics
MIT License
835 stars 148 forks source link

Problem with defaultPipelineConfigInfo #1

Closed Archie3d closed 3 years ago

Archie3d commented 3 years ago

Hi, I am following your tutorial, which is great, thanks a lot!

So, I've spotted a problem with PipelineConfigInfo LvePipeline::defaultPipelineConfigInfo(uint32_t width, uint32_t height); function. This generates a self-referencing structure that contains pointers to its own members. When returning the structure a copy is performed (at least with MS Visual Studio compiler I use, perhaps clang implements return move optimisation here), so that the PipelineConfigInfo structure pointers become invalid.

This can be fixed by changing the signature to

static void defaultPipelineConfigInfo(PipelineConfigInfo& configInfo, uint32_t width, uint32_t height);

or similar.

Also the validation layer complains that viewportInfo.flags, multisampleInfo.flags, and depthStencilInfo.flags have not been set to zero.

blurrypiano commented 3 years ago

Yikes you are absolutely right, thank you for pointing this out! I'll add a correction in the upcoming video and some annotations on the current video pointing out the error.

I'll either use your solution or revert to how I have things in the sandbox project. https://github.com/blurrypiano/littleVulkanEngine/blob/master/littleVulkanEngine/core/lve_pipeline.cpp#L75 So essentially removing VkPipelineViewportStateCreateInfo from PipelineInfoConfig struct, and then initializing it in the createGraphicsPipeline function instead. This was a last second change I made carelessly thinking it'd make the tutorial flow easier to follow, and you're right that my compiler optimized it to a move.

Also the validation layer complains that viewportInfo.flags, multisampleInfo.flags, and depthStencilInfo.flags have not been set to zero.

I suspect this is due to another compiler difference I have overlooked, with my compiler automatically initializaing these values to 0. I'll make sure to test all tutorial code with multiple compilers before releasing to avoid missing errors like this in the future.

Archie3d commented 3 years ago

Thanks for addressing this.

Regarding the flags, it was my fault: because the defaultPipelineConfigInfo does not initialize all the fields of the PipelineConfigInfo structure, it is important to perform the explicit construction of the PipelineConfigInfo (and all the Vulkan structures really).

So, the following code will cause some spurious crashes inside vkCreateGraphicsPipelines:

PipelineConfigInfo pipelineConfig;
LvePipeline::defaultPipelineConfigInfo(pipelineConfig, lveSwapChain.width(), lveSwapChain.height());

but this is the correct way that initializes all the fields to default values:

PipelineConfigInfo pipelineConfig{};  // <---- very easy to overlook!
LvePipeline::defaultPipelineConfigInfo(pipelineConfig, lveSwapChain.width(), lveSwapChain.height());

Looking forward for the next updates, and thanks again for this!

blurrypiano commented 3 years ago

I added a comment in tutorial 04 signalling and issue, and the problems been fixed in tutorial 5. I think it's better not to fix it in the tutorial 04 code to maintain consistency with the videos.

yeokaiwei commented 2 years ago

@blurrypiano Could you kindly point out the error within the video? Putting it in the comments is most confusing to find.