cginternals / globjects

C++ library strictly wrapping OpenGL objects.
https://globjects.org
MIT License
539 stars 59 forks source link

Buffer legacy implementation default target #340

Open X-Ryl669 opened 7 years ago

X-Ryl669 commented 7 years ago

The current code for legacy buffer implementation use a static target by default and all calls bind this static target before calling the GL function. So, it's not possible to have both a PBO and a VBO in a program with the current buffer class. The solution would be to set the target as a non-static member and a setTarget would change it.

Another solution would be to remove all glBindBuffer calls from the legacy implementation but that could break other people's code.

scheibel commented 7 years ago

I think you may refer to our guideline, that with globjects you won't need any glBind* in your code anymore. Using this assumption, your scenario may be a little problem. The OpenGL API provides some functions that requires an object to be bound to a binding point where the actual binding point has no semantical meaning. For those cases, we want to ease the use and provide a default binding point (if it doesn't matter which binding point is used, you can choose any, right?). This default binding point can be changed if it should somehow interfere with your source code. What you further describe is a usage scenario of buffers where the binding point semantics do matter. The VBO has to use GL_ARRAY_BUFFER and a PBO has to use GL_PIXEL_PACK_BUFFER as binding points. You are free to call Buffer::bind in your program providing those targets, but we suggest you use the abstracted interfaces of globjects where we hardcoded the targets for everyone's convenience.

If there is an additional issue, can you extend your example or provide some source code that is currently not working?

For the meantime, I'll provide a short introduction on how we would use PBOs in a program.

/*   */ auto pbo = new Buffer();
/*   */ auto vbo = new Buffer();
/*   */ auto vao = new VertexArray();
/*   */ auto fbo = new Framebuffer();
/*   */ auto colorTex = Texture::createDefault();
/*   */ auto depthTex = Texture::createDefault();
/*   */ fbo->attachTexture(GL_COLOR_ATTACHMENT0, colorTex);
/*   */ fbo->attachTexture(GL_DEPTH_ATTACHMENT, depthTex);
/*   */ //configure rendering
/* 1 */ vao->binding(0)->setBuffer(vbo, 0, sizeof(float)*3);
/*   */ //render
/*   */ fbo->bind(GL_FRAMEBUFFER);
/*   */ //vao->draw(...);
/* 2 */ fbo->readPixelsToBuffer(std::array<GLint, 4>{{ 0, 0, 1920, 1080 }}, GL_RGBA, GL_UNSIGNED_BYTE, pbo);

The crucial lines are numbered (1 and 2). In 1, we use a globjects buffer as VBO and configure a vertex array with geometry data. In 2, the framebuffer should read pixels to a buffer used as PBO. The corresponding OpenGL calls actually bind the buffer as GL_PIXEL_PACK_BUFFER.

X-Ryl669 commented 7 years ago

Typically, I'm trying to do this:

glBindBuffer(GL_PIXEL_UNPACK_BUFFER, pbo[i]);
glBufferData(GL_PIXEL_UNPACK_BUFFER, nbytes, NULL, GL_STREAM_DRAW); 

I can't do that with a Buffer because Buffer::setData does a glBindBuffer with a GL_COPY_WRITE_BUFFER as a target. Even if I do Buffer::bind(GL_PIXEL_UNPACK_BUFFER) before this call, it's changed in the Buffer::setData. The glBufferData call happens with the wrong target too. Thus, I can't tell the graphic driver how I want to use the PBO. NVidia is smart enough to figure out and change it as appropriate, but it's not standard

IMHO, the example you've given works for one case, because readPixelsToBuffer can only happen to a PBO. setData is too generic to make this assumption, and thus it does not allow to do what I want. The same happens for map and unmap rendering the current buffer code unusable for my case.

Check this gist for a concrete example: https://gist.github.com/roxlu/59a13936f1244de32140

X-Ryl669 commented 7 years ago

BTW, to stay within the projects guideline, either a PBOBuffer class could be provided, either changing the target as a non-static member would work. This will not break the assumption that you don't need to bind before use, does not break the existing code either, yet allow using the other types of buffers.

scheibel commented 7 years ago

I had a look at your example. The only functions where you specify the GL_PIXEL_UNPACK_BUFFER are:

All four accept GL_PIXEL_UNPACK_BUFFER as valid binding point, but I also see that they accept every other buffer binding point as well. I don't see any indications that the performance may differ. I would assume that performance can be impeded/improved by specifying the correct usage which you do in your example.

From the current OpenGL API design I learned that a buffer is just a storage area without any usage semantics attached. Within an application, you typically have one usage scenario and thus can choose a constant per-member target. We sometimes use buffers for multiple scenarios (one time array buffer, next time transform feedback buffer, then shader storage buffer) and just choose a binding point suitable for the current use case. It may be that OpenGL drivers make assumptions on how to handle buffers for possible improvements but I don't see any problem switching the GL_PIXEL_UNPACK_BUFFER with GL_TEXTURE_BUFFER or GL_DRAW_INDIRECT_BUFFER (as extreme example). I would additionally argue that your example doesn't use pixel pack/unpack buffer semantics but plain OpenGL buffer semantics and you use a specific buffer binding point for a generic use case.

X-Ryl669 commented 7 years ago

You probably missed the most important one, glTexImage2D (in my case). I still have to call Buffer::bind(GL_PIXEL_UNPACK_BUFFER) before calling it and Buffer::unbind() after it. Maybe a Texture::imageFromBuffer method is missing that's doing the sequence above (à la readPixelsToBuffer).

In all case, the issue is Buffer::setData doing glBufferData on a random target, if I understand what your experience says, should not matter. I'll test it to check if you are right.

scheibel commented 7 years ago

Yes, a imageFromBuffer member function is actually missing (#342).