VideoBackends:OGL: Creating vertex formats shouldn't unbind anything #11279
Conversation
| @@ -77,6 +76,9 @@ GLVertexFormat::GLVertexFormat(const PortableVertexDeclaration& vtx_decl) | |||
| SetPointer(SHADER_TEXTURE0_ATTRIB + i, vertex_stride, vtx_decl.texcoords[i]); | |||
|
|
|||
| SetPointer(SHADER_POSMTX_ATTRIB, vertex_stride, vtx_decl.posmtx); | |||
|
|
|||
| // Other code shouldn't have to worry about its vertex formats being randomly unbound | |||
| ProgramShaderCache::ReBindVertexFormat(); | |||
There was a problem hiding this comment.
When GLVertexFormat is used normally, is s_last_VAO not set? All the code in this constructor looks like setup code, we haven't used the bound VAO yet. So rebinding at this point seems a bit strange. Especially if you don't look at the definition and are just wondering why you would need to rebind, you already bound on line 64.
I haven't looked at your code for culling in any detail. Maybe it should be calling ReBindVertexFormat().
There was a problem hiding this comment.
Line 61 binds the new VAO to OpenGL, but doesn't tell ProgramShaderCache about it, putting them out of sync. To fix this, you need to do something to make ProgramShaderCache::s_last_VAO match what's bound in OpenGL. The old code did that by changing s_last_VAO, while the new code does it by changing what's bound in OpenGL, which is what ReBindVertexFormat is for. (It binds what ProgramShaderCache thinks is the currently bound vertex format, which is momentarily not what's actually bound due to the creation routine needing to temporarily bind the new VAO to set properties on it.)
The problem with the old method is that OGL::Renderer would assume that if it bound a vertex format, it would stay bound until it bound something else. Since OGL::Renderer is kind of in charge of managing all OpenGL things (except this), I'd consider that reasonable to assume, but if you'd rather go the other way, we could make OGL::Renderer always re-bind the VAO regardless of whether its new pipeline is different from the previous one or not.
There was a problem hiding this comment.
Thanks for the explanation @TellowKrinkle . This bit was what I was curious about:
due to the creation routine needing to temporarily bind the new VAO to set properties on it
If the sole purpose of the binding on 61 is to set properties on that VAO, then rebinding makes sense. I was concerned the software actually expected to use the bound VAO after it was bound here. But looking further, like the software uses the VAO in the ProgramShaderCache when BindVertexFormat is set. I should have looked closer before..
TellowKrinkle
force-pushed
the
GlobalStateIsGreat
branch
from
df5e8c5
to
e3cc420
Compare
Should make #11208 not break half the fifoci traces
(Note: The full issue is that
OGL::Rendereronly sets the pipeline if it changed, not expecting it to be partially unbound. This doesn't affect anything by itself because without CPUCull, we never create a vertex format without immediately using it in a pipeline, but with CPUCull, we can create a vertex loader and then cull all the triangles that used it before switching back to the previous pipeline, completely skipping it.)