vblanco20-1 / vkguide-comments

Storage for the comments of vulkan-guide
0 stars 0 forks source link

Vkguide 2 Beta Comments #14

Open utterances-bot opened 7 months ago

utterances-bot commented 7 months ago

- Vulkan Guide

Practical guide to vulkan graphics programming

https://www.vkguide.dev/docs/new_vkguide/chapter_1/vulkan_mainloop/

vblanco20-1 commented 7 months ago

This is the feedback comment channel for Vk guide 2 Early Beta. The comments here will be removed once full release is done. If you find issues with the current version or typos, post here to report them.

khushmmehta commented 7 months ago

Do you have any idea how i could set up this project for CLion?

MasterDrake commented 7 months ago

The hyperlink "Initializing Vulkan" on Chapter0: Code Walkthrough leads to the old version.

MasterDrake commented 7 months ago

1) I don't know if I did something wrong using the starting point or I missed some configuration that had to be done, but my engine project wasn't linked to "vkboostrap" so I coulnd't include the header or link with the .lib. I had to do it manually and the "project dependencies" thing didn't help either.

2) Something I never understood is this: "By doing that = {} thing, we are letting the compiler initialize the entire struct to zero. This is critical, as in general Vulkan structs will have their defaults set in a way that 0 is relatively safe. By doing that, we make sure we don’t leave uninitialized data in the struct." If so, then that doesn't also mean that .pNext is already set to either 0 or nullptr, so it's pointless to set it?

3)Typos:

4) Initialization steps lack the hyperlink to the next step

MasterDrake commented 7 months ago
MasterDrake commented 7 months ago

I think it's a me problem :/

MasterDrake commented 7 months ago
MasterDrake commented 7 months ago
MasterDrake commented 7 months ago

maybe?

MasterDrake commented 7 months ago

2) I have a doubt, VS reports me that I'm using 275 MB whether I'm in release or debug mode. Is it normal? I'm not even loading meshes or textures :/

vblanco20-1 commented 7 months ago

@MasterDrake, thanks for the reports, ill begin fixing all of those.

MasterDrake commented 7 months ago

@MasterDrake, thanks for the reports, ill begin fixing all of those.

Have fun 😊 I'll continue tomorrow and see how it goes Thanks you so much for this, btw!!

vblanco20-1 commented 7 months ago

@MasterDrake Ive tested it, my debug ram cost is also at 200+ MBs. seems like the cost just comes by initializing vulkan and its validation layers.

MasterDrake commented 7 months ago
MasterDrake commented 7 months ago
MasterDrake commented 7 months ago

2) "fmt::print("Loading GLTF: {}.", filePath);"

3) "std::optional<std::vector<std::shared_ptr>> loadGltfMeshes(VulkanEngine* engine, std::string_view filePath);" -> std::filesystem::path filePath also #include .

4) "#include <fastgltf/parser.hpp>

include <fastgltf/tools.hpp>

include <fastgltf/glm_element_traits.hpp>"

Also "//needed for the fastgltf variants template struct overloaded : Ts... { using Ts::operator()...; };" Never used this library so a lot of this stuff was new to me. I also was wondering if there was a way to query indices and vertices size from fastgltf so you can resize the vectors accordingly. It would be a good idea to const the accessors.

5) Since we're using c++20, I'm not asking to use ranges for stuff like: " constexpr bool OverrideColors = false; if (OverrideColors) std::ranges::for_each(vertices, [](Vertex& vtx) { vtx.color = glm::vec4(vtx.normal, 1.f); });" But it would be cool to use if stataments with initializer like this: "if (auto normals = p.findAttribute("NORMAL"); normals != p.attributes.end()) if (auto uv = p.findAttribute("TEXCOORD_0"); uv != p.attributes.end()) if (auto colors = p.findAttribute("COLOR_0"); colors != p.attributes.end())" Reducing the scope isn't vital here, but it's a c++17-ism that's pretty neat IMO.

6) #include <glm/gtx/transform.hpp> when fixing the camera Y axis.

7) "Now we need to change the render pass begin info to use this depth attachment and clear it correctly.": specify that is in the draw_geometry() function. 8) "We made the depth option when the pipelinebuilder was made, buit left it dsiabled": "dsiabled" -> "disabled" 9) " std::vector testMeshes;" -> "std::vector<std::shared_ptr>" 10) Again, I had to manually link fastgltf and fastgltf_simdjson. :( 11) I got validation errors screaming at me because a pipeline builder depth image format was create with VK_FORMAT_UNDEFINED instead of VK_FORMAT_D32_SFLOAT so apparently I had to change this line in init_triangle_pipeline(): "pipelineBuilder.set_depth_format(VK_FORMAT_UNDEFINED);" to " pipelineBuilder.set_depth_format(_depthImage.imageFormat);" even if the depth testing was disabled.

I wish you can name vulkan objects so the validation errors would be useful or should I say precise -.-

MasterDrake commented 7 months ago
MasterDrake commented 7 months ago

3) Mea culpa, I previously suggested to "poolSizes.resize(poolRatios.size());" in the DescriptorAllocatorGrowable::create_pool, but I obviously meant ".reserve" -.- I keep making this mistake even when I'm aware of this pitfall, grr

MasterDrake commented 7 months ago
MasterDrake commented 7 months ago
MasterDrake commented 7 months ago

1) Scene node classes go into "vk_types.h".

2) Not sure whether you're supposed to use "auto& c : children" or "auto c : children" when using shared_ptrs, but you're using both approaches and it's confusing.

3) "MaterialInstance material;": "MaterialData has become MaterialInstance*.

4) "void MeshNode::Draw(const glm::mat4& topMatrix, DrawContext& ctx)" goes into "vk_engine.cpp"

5) So far, GeoSurface doesn't have a material, so "def.material = &s.material->data;" is an error.

6) "We are binding the data every draw which is inneficient but we will fix that later.": "inneficient"->"inefficient"

7) "std::unordered_map<std::string, shared_ptr<Node*>> loadedNodes;": should be std::shared_ptr

8) "loadedNodes[m.name] = std::move(newNode);": should be m->name.

9) "newNode->mesh = std::make_shared(m);": this becomes "newNode->mesh = m;" in the source code, not sure the difference. Can't even compile without changing it.

10) For some messy reason, I didn't have "Cube" loaded so I crashed. I used a mesh I knew I had in my unordered_map(ehi, can we change it to martinus one and see how much is better) and it's huge so I'm not sure I'm doing ok :D Hopefully with the free camera... .Mantaining a project like this with so many refactors can become a pain in the ass. IMO, some of the deprecated stuff should be resolved before jumping to the next stage.

MasterDrake commented 7 months ago
MasterDrake commented 7 months ago

Before we begin loading everything, We are going to create some arrays to hold the structures. In GLTF files, everything works through indices, so we need a way to handle that . For example a mesh node will give the mesh index, not name or anything similar.


4) "    loadedScenes["structure"]->Draw(glm::mat4{ 1.f }, drawCommands);": "drawCommands" should be  "mainDrawContext".
5) I already said that but it needs a clean up since test meshes are still loaded and rendered.
6) It's needed "#include "vk_descriptors.h" in "vk_loaders.h" because of DescriptorAllocatorGrowable we just added.
7) "if (node->parent.get() == nullptr)": gives me an error that I can't understand and the source code is this: "if (node->parent.lock() == nullptr)"
8) Not gonna lie, this is a pretty big function we've got here and it's not over yet
MasterDrake commented 7 months ago
MasterDrake commented 7 months ago

Thank you so much!!

senorsenorjunior commented 7 months ago

"We set queueFamilyIndex to the _graphicsQueueFamily that we grabbed before. This means that the command pool will create commands that are compatible with any queue of that “graphics” family."

Both the code in the lesson and the function and the function in vk_initializers.cpp don't actually set the queueFamilyIndex field.


    VkCommandPoolCreateFlags flags /*= 0*/)
{
    VkCommandPoolCreateInfo info = {};
    info.sType = VK_STRUCTURE_TYPE_COMMAND_POOL_CREATE_INFO;
    info.pNext = nullptr;

    info.queueFamilyIndex = queueFamilyIndex;
    info.flags = flags;
    return info;
}```

By the way, thanks a ton for this updated guide! :)
MasterDrake commented 7 months ago

1) My "LoadedGLTF::clearAll" is different from the one in your source code. Not to mention the pointless "samplersToDestroy" variable after the samplers are destroyed. Unless I'm missing something. 2) Apparently I don't have a "GPUGLTFMaterial" for the uniform buffer :/ 3) I don't have an "AllocatedBuffer cameraBuffer" in FrameData 4) I don't have "EngineStats" as well, but that's not surprising. 5) I don't have " // the format for the draw image VkFormat _drawFormat;" 6) you added "init_renderables" and got rid of "init_triangle_pipeline" and "init_mesh_pipeline" 7) there's not render_nodes() for me. 8) I don't have "std::vector _framebuffers;" nor "VkDescriptorSet _defaultGLTFdescriptor;" 9)"globalDescriptorAllocator" is not Growable, it's fine? 10) I don't have " VkDescriptorPool _descriptorPool;" either. 11) I don't have " AllocatedBuffer _defaultGLTFMaterialData;"

12) In the end I had to got rid of: " VkPipelineLayout _trianglePipelineLayout; VkPipeline _trianglePipeline; VkDescriptorSetLayout _singleImageDescriptorLayout;

MaterialData defaultData;

VkPipelineLayout _meshPipelineLayout; VkPipeline _meshPipeline;

std::vector<std::shared_ptr> testMeshes;"

I'm not ready for vk_engine.cpp :(

vblanco20-1 commented 7 months ago

@MasterDrake most of those you have mentioned are never used, and its just dead code i havent fixed yet. you dont have to worry. Chapter 5 in particular is prone to that dead code because its the "original" code for the tutorial chapters, chapters 2 to 4 were made after it.

MasterDrake commented 7 months ago

Yeah, I know nothing too fancy, but at least I'm done :D

MasterDrake commented 7 months ago

I figure out why I coulnd't load the textures image

It was missing this and apparently my vk_loader code is broken but I still missed the bug while file-diffing :( "#define STB_IMAGE_IMPLEMENTATION

include "stb_image.h""

in "vk_images.cpp" I hate these headers implementation

Finally!

senorsenorjunior commented 7 months ago

Completely stuck at the Textures lesson, as soon as I set the _singleImageDescriptorSet to the _meshPipelineLayout.pSetLayouts field, the validation layer vomits this error and nothing renders

Validation Error: [ VUID-vkCmdDrawIndexed-None-02697 ] Object 0: handle = 0x3fbcd60000000028, type = VK_OBJECT_TYPE_PIPELINE; Object 1: VK_NULL_HANDLE, type = VK_OBJECT_TYPE_PIPELINE_LAYOUT; | MessageID = 0x9888fef3 | vkCmdDrawIndexed(): The VkPipeline 0x3fbcd60000000028[] (created with VkPipelineLayout 0xb097c90000000027[]) statically uses descriptor set (index #0) which is not compatible with the currently bound descriptor set's pipeline layout (VkPipelineLayout 0x0[]) The Vulkan spec states: For each set n that is statically used by the VkPipeline bound to the pipeline bind point used by this command, a descriptor set must have been bound to n at the same pipeline bind point, with a VkPipelineLayout that is compatible for set n, with the VkPipelineLayout used to create the current VkPipeline, as described in Pipeline Layout Compatibility (https://vulkan.lunarg.com/doc/view/1.3.239.0/windows/1.3-extensions/vkspec.html#VUID-vkCmdDrawIndexed-None-02697)

vblanco20-1 commented 7 months ago

@mattwhitson The texture chapter had an error on the code snippets where the init_mesh_pipeline() is updated with the new descriptor set layout and new shaders. Check that chapter again as i just fixed it

aer-i commented 7 months ago

If someone else also have problems with buffer device address (in my case vertices have wrong positions) try using alignas() for the vertex struct

vblanco20-1 commented 7 months ago

@aer-i Did you modify the vertex struct? With the exact code shown in the guide, it should align exactly to what the GPU wants in the shader with std430 rules. If you didnt, what gpu did you use?

aer-i commented 7 months ago

@vblanco20-1 Sorry, I should have mentioned that I have different vertex struct. Everything is fine, with alignment in the tutorial. I just tried moving to device address vertex buffer from normal vertex buffer, but then vertices broke in my game engine. I was a bit confused so I wanted to inform people with similar problem.

aer-i commented 7 months ago

"void VulkanEngine::init_descriptors() { // other code

^code init_desc_2 chapter-2/vk_engine.cpp }"

vblanco20-1 commented 7 months ago

Error on init_descriptors() fixed

YanTree commented 7 months ago

follow "2 Drawing with Compute / Setting up IMGUI" of Vkguide2, the visual studio catch one error below, any advice can fix it? VulkanEngine initialized Assertion failed: GImGui != 0 && "No current context. Did you call ImGui::CreateContext() and ImGui::SetCurrentContext() ?", file D:\vulkan-guide\third_party\imgui\imgui.cpp, line 4239

D:\vulkan-guide\bin\Debug\engine.exe (process 40164) exited with code 3.

vblanco20-1 commented 6 months ago

@YanTree make sure you call init_imgui() from the main init() function of the engine, you likely missed it.

YanTree commented 6 months ago

@vblanco20-1 oh! Right here, i missed call it, thx your advice.

C1oud555 commented 6 months ago

For a Mac user, Vulkan is supported by the MoltenVK, which only support api_version 1.2;

Although the required extension dynamic rendering and synchronization is supported, but as KHR extensions.

In order to do that, use latest Volk is not that hard

  1. use cmake to load volk
    add_subdirectory(volk) # complete repo of Volk, not existing one under third_party
  2. define corresponding features so Volk will load related functions
    
    #define VK_KHR_synchronization2
    #define VK_NO_PROTOTYPES

include

3. add KHR suffix in all synchronization2 functions, structures.... eg:
VkImageMemoryBarrier2 imageBarrier{};

-------> VkImageMemoryBarrier2KHR imageBarrier{};



Emmmmm, could be a advice or a note for Mac users.
wesleywong1995 commented 6 months ago

I followed the tutorial in the IMGUI chapter and found an error indicating that the setLayout was not closed after closing the program

mrriddleman commented 6 months ago

Found a few issues so far in Section 2:

  1. We don't seem to be destroying the descriptor pool. I added these lines at the end of init_descriptors():

_mainDeletionQueue.push_function([&]() { vkDestroyDescriptorSetLayout(_device, _drawImageDescriptorLayout, nullptr); globalDescriptorAllocator.destroy_pool(_device); });

  1. I'm not sure we're destroying the effect pipelines correctly in the PushConstants sections. I was receiving validation errors about destroying the pipelines.

I got it to not complain with this code at the end of init_background_pipelines():

_mainDeletionQueue.push_function([&]() { vkDestroyPipelineLayout(_device, _gradientPipelineLayout, nullptr);

    for (auto&& effect : backgroundEffects)
    {
        vkDestroyPipeline(_device, effect.pipeline, nullptr);
    }

});

  1. For the PushConstants lecture - There's no changes to the sky.comp shader if we change the push constant data in the UI. This is obviously because push constants weren't added to the sky.comp shader.

I modified the sky.comp compute shader by adding the constants:

version 450

layout (local_size_x = 16, local_size_y = 16) in; layout(rgba8,set = 0, binding = 0) uniform image2D image;

//push constants block layout( push_constant ) uniform constants { vec4 data1; vec4 data2; vec4 data3; vec4 data4; } PushConstants;

Then I changed the mainImage a little to make the sky change color + change the star density

void mainImage( out vec4 fragColor, in vec2 fragCoord ) { vec4 pushedData = PushConstants.data1; //new

vec2 iResolution = imageSize(image);
// Sky Background Color
vec3 vColor = pushedData.xyz * fragCoord.y / iResolution.y; //use the push constants

// Note: Choose fThreshhold in the range [0.99, 0.9999].
// Higher values (i.e., closer to one) yield a sparser starfield.
float StarFieldThreshhold = pushedData.w; //use the push constants

// Stars with a slow crawl.
float xRate = 0.2;
float yRate = -0.06;
vec2 vSamplePos = fragCoord.xy + vec2( xRate * float( 1 ), yRate * float( 1 ) );
float StarVal = StableStarField( vSamplePos, StarFieldThreshhold );
vColor += vec3( StarVal );

fragColor = vec4(vColor, 1.0);

}

Enjoying the tutorial so far!

OptimisticMonkey commented 6 months ago

To avoid crash when closing app (after Mesh Buffers section):


     destroy_buffer(rectangle.indexBuffer);
     destroy_buffer(rectangle.vertexBuffer);
     });```
OptimisticMonkey commented 6 months ago

To avoid crash when closing app (after Mesh Buffers section):


_mainDeletionQueue.push_function([&]() {
     destroy_buffer(rectangle.indexBuffer);
     destroy_buffer(rectangle.vertexBuffer);
     });```
OptimisticMonkey commented 6 months ago

Likewise, need to add delete stuff after Mesh Loading section:

_mainDeletionQueue.push_function([&]() {
    for( auto&& meshAsset : testMeshes)
        {
        destroy_buffer(meshAsset->meshBuffers.indexBuffer);
        destroy_buffer(meshAsset->meshBuffers.vertexBuffer);
        }
    });
OptimisticMonkey commented 6 months ago

One more minor cleanup tip - when exiting there is a high chance the current in flight frame will not have the _deletionQueue called. Added to cleanup():

//Make sure to flush all possible in flight frames
for (FrameData framedata : _frames)
{
    framedata._deletionQueue.flush();
}
hartmutbehrens commented 6 months ago

If you're trying to follow along on macOS then you've likely run into linker issues. Inserting this after the filrst call to target_link_libraries(vkbootstrap,...) helped me:

target_link_libraries(vkbootstrap PUBLIC "iconv -framework AudioToolbox -framework Carbon -framework Cocoa -framework CoreFoundation -framework CoreGraphics -framework CoreHaptics -framework CoreAudio -framework CoreVideo -framework ForceFeedback -framework GameController -framework IOKit -framework Metal")
SNAKFRIEN commented 6 months ago

If I understand correctly, some vma memory usage flags, such as VMA_MEMORY_USAGE_GPU_ONLY, are deprecated. This link contains more info: https://gpuopen-librariesandsdks.github.io/VulkanMemoryAllocator/html/deprecated.html

Fletterio commented 6 months ago

I just finished chapter 2

I don't know what causes it or if it is reproducible for anyone else. I noticed this in my own version of the code (starting from the starting point project an adding stuff as I go) and then I built the chapter-2 code in the all-chapters project and the same thing happens

If anyone has the time can you check if you get the same issue from RenderDoc? I've spent the whole day trying to debug it and didn't get anywhere