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

Ability to bind vertex buffer using offset #3262

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

falia18
Copy link

@falia18 falia18 commented Mar 3, 2024

I'm @Justine1801 on Discord if you want to talk about the PR. I really care about this getting into the main repository if possible, because it makes it much easier for remastering games.

Summary of changes

In bgfx_p and renderers, replace startVertex and num by offset and size.
In bgfx.h, add functions to bind a vertex buffer using offset instead of startVertex. Also allows to set instance buffer stride when binding.

Why the changes

My company worked on the remaster of a AAA game (not announced yet) with OpenGL-like API for graphics and ran into limitations due to the way buffers were handled in the game.

I am confident the changes work, they were extensively tested with DX11 & DX12, GNMP, a bit with Vulkan (before we switched to NVN)
Examples run without issue for OpenGl but we couldn't test it in game due to other incompatibilities. Metal was not tested but the changes in renderer_mm are minimal so I don't think it will break anything.

Use cases

  • Non interleaved buffers (ex : pos1-pos2-pos3-uv1-uv2-uv3) where the offset of uv1 is not divisible by the stride of its layout
  • Buffers holding more than one object, with different layouts
  • Vertex layout not known at loading time (especially stride) and you need the dynamic vertex buffer to be allocated beforehand
  • Vertex layout changing at runtime for dynamic vertex buffer
  • With a few additionnal changes, it makes stride 0 work on almost all renderers, needed in our case to emulate glVertex4f, see below

The changes couldn't be made for TransientVertexBuffer due to the struct being in the API (I can't change startVertex to offset because it would break existing projects). In my opinion it doesn't matter as much, because a transient buffer is meant to be used immediately anyway, and there is less chance you don't have the layout on hand to bypass the limitations.

Fixes

Changing Dynamic vertex buffer stride between creation time and bind time now works. It worked for static vertex buffer already, except in DX12 see my previous PR

Left to do

  • Generate new bindings and doxygen
  • Documentation

I added the changes to IDL but I don't know how to generate doxygen and bindings with it. Which is also why I haven't made the changes in the documentation yet.

About stride 0

TLDR: OpenGL only supports it from version 4.3. Works with minimal changes in cross platform code for all other backends.

These are the few additionnal changes needed to make stride 0 work:

  • replace strideAlign by strideAlign<4>() to prevent a division by zero, and align to minimum 4 since metal and D3D don't always support below
  • prevent division by zero when computing m_numVertices and idb->num using stride
  • add assert for stride 0 in getAvailableTransientbuffer
  • add assert when using startVertex > 0 and stride == 0 in otjher
    With these it works out of the box for D3D, Vulkan, GNM, etc
    Based on the documentation, for Metal too but I haven't tested it.
    Also on GNMP and NVN (I'm still fighting with my company to get the renderers on the dev forums but they may never allow it)

The issue is OpenGL:
bgfx's code uses glVertexAttribPointer for vertex attributes, and when this function is given a stride of 0, the vertex attributes are understood to be tightly packed in the array (https://registry.khronos.org/OpenGL-Refpages/gl4/html/glVertexAttribPointer.xhtml)
Which means it won't work because it will autocompute the stride.

There is a way to make it work, and it's to replace that function by:
glBindVertexBuffer and glVertexAttribFormat
which work more like D3D functions.

But it's only available from OpenGL 4.3. It could be implemented with a define checking for minimal version, but from what I've seen while skimming through the gl renderer it's not something you do.

@falia18
Copy link
Author

falia18 commented Mar 16, 2024

Use cases example
Launch 01-cubes with any renderer other than d3d12 (d3d12 needs this fix to work).
It contains example of buffers where startVertex isn't sufficient to properly bind them. I also implemented the working solution with this PR for comparison. Define BIND_WITH_OFFSET and checkout this PR to make it run. This is the expected result:
image

Dynamic vertex buffer stride issue
Lauch 35-dynamic, it gives this:
image
instead of this:
image

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.

1 participant