search

floooh / sokol

minimal cross-platform standalone C headers
https://floooh.github.io/sokol-html5
zlib License
6.82k stars 475 forks source link

Added checks to avoid needless calls to glVertexAttribPointer and Div… #835

Closed LorenRoosendaal closed 1 year ago

LorenRoosendaal commented 1 year ago

I noticed that in some specific cases we were paying a good percentage of extra cost per sg_apply_bindings (dropping raw draw calls processed in a little benchmark by as much as 35%) because it doesn't check if the newly bound buffer actually has different Attributes (in optimized production code they can often be the same). Added a little fix.

Hope I didn't slip up anywhere, this is just a quick commit from the browser ;-) looks clean to me though!

floooh commented 1 year ago

Thanks, I think that should work. I'll give it a whirl later today!

PS: more like tomorrow, another PR took a bit longer than expected :)

floooh commented 1 year ago

I'm starting to look into the PR now.

floooh commented 1 year ago

Ah ok, I found a flaw with the PR :)

It's not very obvious from the GL spec, but the currently bound vertex buffer isn't independent from the vertex attribute state, but instead the current vertex buffer binding (at the time when glVertexAttribPointer() is called) is stored in GL's vertex attribute state, otherwise pulling vertex components of the same vertex from different buffers wouldn't work. The glVertexAttribPointer() call basically 'captures' the current buffer binding and stores it in the vertex attrib state.

From https://registry.khronos.org/OpenGL-Refpages/gl4/html/glVertexAttribPointer.xhtml:

Screenshot 2023-05-21 at 12 31 22

This means that just changing the buffer bindings doesn't change any existing vertex attribute state (e.g. existing vertex attribute definitions will still refer to the buffer that was bound at the time glVertexAttribPointer was called last time for this vertex attribute).

I'm also somewhat sure that in the olden days of sokol_gfx.h, this code actually treated the buffer binding as a separate state (so it looked similar to your PR) but this fell apart when I added support for pulling vertex components from multiple vertex buffers (unfortunately this change must have been before I combined sokol_gfx.h into a single header, so git blame stops at that point).

There's also this 'subtle fix' for the vertex attribute divisor from 5 years ago which moved the glVertexAttribDivisor call from being independent from the glVertexAttribPointer call into the same code block:

https://github.com/floooh/sokol/commit/fa960b02393082ec47a9e2715d7c820053b0a75e

In this case I'm not sure though if the glVertexAttribDivisor call can be put into an additional if-block, need to carefully read the GL spec and wiki again.

In any case, I'll close the PR without merging, sorry about that.