LukasBanana / LLGL

Low Level Graphics Library (LLGL) is a thin abstraction layer for the modern graphics APIs OpenGL, Direct3D, Vulkan, and Metal
BSD 3-Clause "New" or "Revised" License
2.03k stars 135 forks source link

Inaccurate(?) documentation for `CreateBufferArray`. #115

Closed gdianaty closed 2 months ago

gdianaty commented 3 months ago

Hello.

In the documentation for CreateBufferArray, the following remark can be found:

\remarks All buffers within this array must have the same binding flags.

However, looking at LLGL's Example_Instancing example, the vertex buffer is created with LLGL::BindFlags::VertexBuffer and the per-instance data buffer is created with no bind flags defined. When run with the -d parameter to enable the LLGL Debug layer, no errors occur on Vulkan, D3D11 or D3D12. Obviously, the example itself works just fine.

Why are heterogenous binding flags allowed here? Am I misunderstanding the documentation, or is there some kind of exception for per-instance data?

LukasBanana commented 3 months ago

This example code could probably use a more explicit approach of declaring its buffers as it's not immediately obvious that the per-instance buffer uses the same buffer descriptor and only overrides all other members but inherits the same binding flags (see Example.cpp:179):

LLGL::BufferDescriptor desc;

desc.debugName      = "Vertices";
desc.size           = sizeof(vertexData);
desc.bindFlags      = LLGL::BindFlags::VertexBuffer;
desc.vertexAttribs  = vertexFormatPerVertex.attributes;
vertexBuffers[0] = renderer->CreateBuffer(desc, vertexData);

desc.debugName      = "Instances";
desc.size           = static_cast<std::uint32_t>(sizeof(Instance) * instanceData.size());
/* Does not override desc.bindFlags here, so it inherits the same value */
desc.vertexAttribs  = vertexFormatPerInstance.attributes;
vertexBuffers[1] = renderer->CreateBuffer(desc, instanceData.data());

It's also worth noting that the BufferArray interface was originally an alternative to ResourceHeap but only for buffers. Now it's solely used for arrays of vertex buffers, to which per-instance data also counts and hence such buffers have to be created with the VertexBuffer binding flag, too.

gdianaty commented 2 months ago

Understood!

Thanks for clarifying.

Would you like to leave this issue ticket open and edit the Example_Instancing example to clarify, or shall we go ahead and close this one out?

I'm happy to make those clarifying edits and open a PR.

LukasBanana commented 2 months ago

Instancing example is a bit better to read now; See Instancing/Example.cpp:180