BoomingTech / Piccolo

Piccolo (formerly Pilot) – mini game engine for games104
MIT License
5.69k stars 1.79k forks source link

Vulkan 断言条件修正 #438

Closed ShenMian closed 6 months ago

ShenMian commented 1 year ago

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

v3c70r commented 1 year ago

我认为 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 commented 1 year ago

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

ShenMian commented 9 months ago

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

kwbm commented 9 months ago

本地测试了一下,会有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 commented 8 months ago

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

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

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

ShenMian commented 8 months ago

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

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

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

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

kwbm commented 8 months ago

@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 commented 8 months ago

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

ShenMian commented 8 months ago

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