OpenSceneGraph icon indicating copy to clipboard operation
OpenSceneGraph copied to clipboard

Fix VBO dispatching

Open jpabst0 opened this issue 5 years ago • 5 comments

Here are a few points to summarize my assumptions and understanding of the code.

  • When VAOs are used, VBOs are used (if supported) regardless off the usingVertexBufferObjects setting.

This is status quo and seems reasonable. However, the usingVertexBufferObjects stetting still determines if EBOs are used.

So, if one sets up an Drawable

drawable->setUseVertexBufferObjects(false); drawable->setUseVertexArrayObject(true);

one would get VAOs with VBOs, but not EBOs.

I don't know what is really intended and if such setup is practically used. (The compileGLObjects functions don't seem to handle it either.) I think it would make sense to also use EBOs, as it's a more efficient use of VAOs. To achieve this, one can simply replace in drawImplementations the line

bool usingVertexBufferObjects = state.useVertexBufferObject(_supportsVertexBufferObjects && _useVertexBufferObjects);

with this one:

bool usingVertexBufferObjects = vas->getVertexBufferObjectSupported();

Anyway, one should be aware that the variable usingVertexBufferObjects is somewhat misleading because it doesn't tell whether VBO are really used.

  • When not using VAOs, the usingVertexBufferObjects setting determines if VBOs and EBOs are used.

I added these lines to Drawable::draw:

vas->setVertexBufferObjectSupported(usingVertexBufferObjects); vas->setRequiresSetArrays(true);

The first fixes the missing setup so that the VBOs get properly dispatched, the latter helps setting up the vas object transparently for subclass drawImplementations.

It simplifies the condition

!usingVertexArrayObjects || vas->getRequiresSetArrays()

to

vas->getRequiresSetArrays()

  • I couldn't figure out why the VBO's are unbound at the end of each drawImplementation. Shouldn't the VertexArrayState and it's lazy update mechanism handle this? I guess it is for compatibility with custom Drawables.

Actually I discovered a reason to unbind the VBO when VAOs are used. In OpenGL, the bound VBO is not part of the VAO state while in OSG it is modeled so. Not unbinding the VBO can cause it to not get bound in the next frame, if it is falsely assumed to not have changed. I will provide a detailed description and an example program in the issue report.

As simplification and workaround I replaced the lines

if (usingVertexBufferObjects && !usingVertexArrayObject) { // unbind the VBO's if any are used. vas->unbindVertexBufferObject(); vas->unbindElementBufferObject(); }

with these ones

// unbind the VBO's if any are used. vas->unbindVertexBufferObject(); if (!state.useVertexArrayObject(_useVertexArrayObject)) vas->unbindElementBufferObject();

Note that in case of using VAOs, the unbindVertexBufferObject function is a no-op, except for the case that one of the VBOs really got updated. This is even the case when the data variance is set to DYNAMIC. So it shouldn't hurt performance or undermine the purpose of VAOs.

When not using VAOs, both buffer objects are unbound like it has been the case before.

jpabst0 avatar Nov 17 '20 14:11 jpabst0

I have done a first pass review and build/tested the example mentioned in #992. The changes are significant and non obvious enough that I will need to really deeply examine the issue - it'd be very easy to "fix" one usage case but then break others in subtle and difficult to track down ways.

For now I'll move on to review other PR's that more straightforward to review as this one could take a while to properly review and test and consolidate on what should be done.

openscenegraph avatar Jan 18 '21 12:01 openscenegraph

I've applied this PR in november 2020 and there was no difference in my apps. NOTE: I use VAO everywhere where possible.

valid-ptr avatar Jan 18 '21 15:01 valid-ptr

@valid-ptr What you say there was no difference, what do you mean? No fixes to problems you see, or no new problems appeared? What about performance?

robertosfield avatar Jan 18 '21 16:01 robertosfield

@jpabst0 This PR really touches on the design/implementation of the VertexArrayState which has some complex interactions - it's all so complicated because OpenGL/OSG has evolve from fixed function, no vertex arrays, to supporting vertex arrays, to supporting vertex buffer objects, to supporting vertex array objects. Trying to keep all the different usage cases working has been a bit of nightmare.

To properly dive into this topic will likely take me a day or two, something I can't afford to do right away - I have to juggle my time between the VSG and OSG open source work, with bits of paid client work that pays for it, so far this month I'm two weeks of unpaid open source work vs 1 day paid work, so can't go indefinitely doing support.

robertosfield avatar Jan 18 '21 16:01 robertosfield

What you say there was no difference, what do you mean? No fixes to problems you see, or no new problems appeared? What about performance?

  1. no new problems appeared
  2. same performance

valid-ptr avatar Jan 18 '21 19:01 valid-ptr