Skip to content
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

Vulkan 断言条件修正 #438

Closed
wants to merge 4 commits into from
Closed

Vulkan 断言条件修正 #438

wants to merge 4 commits into from

Conversation

ShenMian
Copy link
Contributor

@ShenMian ShenMian commented Mar 9, 2023

Vulkan 的顶点缓冲区至少支持 std::numeric_limits<uint32_t>::max() 个顶点1, 为确保顶点数较多的模型能够成功加载, 对断言进行了修正.

Footnotes

  1. https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/vkCmdDraw.html

@v3c70r
Copy link

v3c70r commented May 4, 2023

我认为 Vertex Count 的最大值应该和 index buffer type 一致。vertex count 的最大值不应该超过 index buffer 数据类型能指向的最大值。所以这个 assert 是正确的。

根据上面一行

mesh_data.m_index_buffer = std::make_shared<BufferData>(mesh_vertices.size() * sizeof(uint16_t));

这里 index buffer 是 uint_16, 所以用 std::numeric_limits<uint16_t>::max() 应该是正确的。

FYI: Vulkan 支持 8-bit, 16-bit 和 32-bit 的 VkIndexType

@ShenMian
Copy link
Contributor Author

ShenMian commented May 5, 2023

@v3c70r 谢谢指出这个问题, 不过该“修正”是为了支持更大的顶点数和索引数, 并不是指原本的断言是错误的. 所以选择将原有的 uint16_t 改为更大的 uint32_t.
目前该 PR 的修改不正确, 还需要将其他相关地方一并修改成 uint32_t 类型.

@Ol6rin Ol6rin requested a review from kwbm May 12, 2023 16:04
@ShenMian
Copy link
Contributor Author

@kwbm 请审核一下该 PR, 谢谢 :D

@kwbm
Copy link
Contributor

kwbm commented Oct 23, 2023

本地测试了一下,会有validation layer的报错,可以再看一下能不能修改解决
validation layer: Validation Error: [ VUID-vkCmdDrawIndexed-firstIndex-04932 ] Object 0: handle = 0x593f7400000001a4, type = VK_OBJECT_TYPE_BUFFER; | MessageID = 0x20509750 | vkCmdDrawIndexed(): index size (4) * (firstIndex (0) + indexCount (408)) + binding offset (0) = an ending offset of 1632 bytes, which is greater than the index buffer size (816). The Vulkan spec states: (indexSize {times} (firstIndex + indexCount) + offset) must be less than or equal to the size of the bound index buffer, with indexSize being based on the type specified by indexType, where the index buffer, indexType, and offset are specified via vkCmdBindIndexBuffer (https://vulkan.lunarg.com/doc/view/1.2.198.1/windows/1.2-extensions/vkspec.html#VUID-vkCmdDrawIndexed-firstIndex-04932)

@v3c70r
Copy link

v3c70r commented Nov 20, 2023

个人认为不应该将 unit16 的 index buffer 替换为 uint32 的 index buffer. 至少应该提供能使用 unit16 index buffer 的选项。

理想状况应该是根据加载的模型来选择使用相应的 index buffer data type. 或者在模型处理阶段将模型拆分成较小的 submesh 以适应 unit16 index buffer.

index buffer 增大会导致内存使用量增大。同时会导致缓存使用率低和寄存器占用率高的问题。根据硬件的不同和 shader 的复杂度,可能会对性能有不同程度的影响。

@ShenMian
Copy link
Contributor Author

ShenMian commented Nov 21, 2023

@v3c70r 请问 "导致寄存器占用率高" 是什么意思呢?

如果先阶段还不能为单独的模型的索引缓冲区指定索引类型, 我觉得还是应该先改为 uint32_t, 作为一个临时解决方案. 否则会部分顶点数较多的模型无法加载.
对于原本顶点数少的模型增加的内存使用可能不多, 而拥有更多顶点数的模型也必须使用 uint32_t.

  1. 为模型选择合适的索引类型. (应该还没有实现该功能, 因为全部索引类型都是硬编码的)
  2. Submesh. (这个我不了解)
  3. 默认使用更大的索引类型. (简单粗暴)

@v3c70r
Copy link

v3c70r commented Nov 21, 2023

抱歉我查了一下,可能并不会导致寄存器使用率增加。因为 index buffer 在 vertex fetch 阶段之后就没有用了。在 vertex shader 里面应该用不上 index buffer 的数据 🤔 。
但是因为会对性能有潜在影响,比如有人做过 profiling。所以我认为可能不写死会好一点。不过目前解决加载大模型的问题的优先级较高,可能这个也不是大问题。

@kwbm
Copy link
Contributor

kwbm commented Dec 1, 2023

@ShenMian 测试你的最新改动,本地环境debug版本仍然会报validation layer的问题,我的vulkan SDK版本是1.2.198.1
validation layer: Validation Error: [ VUID-vkCmdDrawIndexed-firstIndex-04932 ] Object 0: handle = 0xc48f160000000142, type = VK_OBJECT_TYPE_BUFFER; | MessageID = 0x20509750 | vkCmdDrawIndexed(): index size (4) * (firstIndex (0) + indexCount (111000)) + binding offset (0) = an ending offset of 444000 bytes, which is greater than the index buffer size (222000). The Vulkan spec states: (indexSize {times} (firstIndex + indexCount) + offset) must be less than or equal to the size of the bound index buffer, with indexSize being based on the type specified by indexType, where the index buffer, indexType, and offset are specified via vkCmdBindIndexBuffer (https://vulkan.lunarg.com/doc/view/1.2.198.1/windows/1.2-extensions/vkspec.html#VUID-vkCmdDrawIndexed-firstIndex-04932)
我看你的最新一条改动是
image
你本地环境没遇到这个validation layer报错吗?

@kwbm
Copy link
Contributor

kwbm commented Dec 1, 2023

初步看了一下,下面地方需要修改
image

@ShenMian
Copy link
Contributor Author

ShenMian commented Dec 1, 2023

@kwbm 我在本地环境下运行确实没有出现 validation layer 的报错. 我会根据你的报错信息再分析一下.
好的, 我修改一下.

@ShenMian ShenMian closed this by deleting the head repository Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants