mosra / magnum

Lightweight and modular C++11 graphics middleware for games and data visualization
https://magnum.graphics/
Other
4.74k stars 439 forks source link

Mixing with raw openGL calls and Context::current().resetState() #626

Open TrevorCash opened 11 months ago

TrevorCash commented 11 months ago

I am trying to get up and running with using raw openGL inside a simple Application. (I am porting an application that used to use raw SDL to now use Magnum)

I am using the imgui example as a base.

if I use some simple openGL calls:

glBindVertexArray(gameGraphics->linesVAO);
glDrawArrays(GL_LINES, 0, maxLines);
glBindVertexArray(0);

In the Application::drawEvent()

I get a crash after a frame (or 2?) when imgui is draw with the Mesh object.

So to midigate I started to use Magnum::GL::Context::current().resetState(Magnum::GL::Context::State::EnterExternal); and Magnum::GL::Context::current().resetState(Magnum::GL::Context::State::ExitExternal);

Magnum::GL::Context::current().resetState(Magnum::GL::Context::State::EnterExternal);

glBindVertexArray(gameGraphics->linesVAO);
glDrawArrays(GL_LINES, 0, maxLines);
glBindVertexArray(0);

Magnum::GL::Context::current().resetState(Magnum::GL::Context::State::ExitExternal);

So now it no longer crashes (Great!)

I have found that using Magnum::GL::Context::current().resetState(); causes some corruption of the framebuffer with imgui or something. because if you add this code:

Magnum::GL::Context::current().resetState(Magnum::GL::Context::State::EnterExternal);
Magnum::GL::Context::current().resetState(Magnum::GL::Context::State::ExitExternal);

right after the _imgui.newFrame() call. the application will distort the imgui windows when the application windows is resized by dragging.

I am testing on windows.

In general I should be able to bind different gl buffers / textures as long as I use resetState() right to push/pop my changes?

mosra commented 11 months ago

I get a crash after a frame (or 2?) when imgui is draw with the Mesh object.

To explain this behavior, the state tracker at that point thinks the Mesh object VAO was still bound, while in reality you called glBindVertexArray(0) in the meantime. And GL has this great misfeature where it interprets the buffer offsets as pointers into client memory if no vertex buffer is bound, leading to a null pointer dereference and crash. That's the extreme scenario and the resetState() tells the state tracker that it shouldn't assume a particular VAO is bound and instead bind it again.

In general I should be able to bind different gl buffers / textures as long as I use resetState() right to push/pop my changes?

Yes. Buffers, textures, framebuffers, VAOs, currently used shaders, transform feedback. That all is tracked (and the tracked state is reset by this call). However note that the reset isn't resetting the state itself -- that'd be too slow -- it's just resetting the tracker. In other words it causes Magnum to explicitly rebind/reset a particular state next time it gets used, but until then GL isn't touched in any way.

corruption of the framebuffer with imgui or something

The state reset in this cause leads to the state tracker thinking no known framebuffer is bound (so, not even the default one). Which then unfortunately leads to the GL::defaultFramebuffer.setViewport() call in viewportEvent() in that example not being propagated to a glViewport() call, because it's not sure that GL::defaultFramebuffer is currently bound. Calling

GL::Context::current().resetState(GL::Context::State::EnterExternal);
GL::Context::current().resetState(GL::Context::State::ExitExternal);
GL::defaultFramebuffer.bind();

"fixes" this, calling the resetState() before the clear() also fixes it. You could also exclude framebuffer-related state from these if you aren't touching them in any way in the raw GL calls, which fixes that too:

GL::Context::current().resetState(GL::Context::State::EnterExternal & ~GL::Context::State::Framebuffer);
GL::Context::current().resetState(GL::Context::State::ExitExternal & ~GL::Context::State::Framebuffer);

Honestly, I'm not sure what would be the best way to handle this problem inside Magnum, apart from completely ditching the design where Magnum pretends a viewport rect is framebuffer-local state (which in reality it isn't).

TrevorCash commented 11 months ago

Thanks, I think I understand the behalviour better.

Initially I expected that State::EnterExternal and State::ExitExternal would do a full push/pop. and State::ExitExternal would make the tracker think that the default framebuffer is bound again (or whatever state it was in at State::EnterExternal time). (which in my use case it still would be bound because I did not alter that).

mosra commented 11 months ago

would make the tracker think that the default framebuffer is bound again (or whatever state it was in at State::EnterExternal time)

Yes, that's where the problem is. There's about sixty thousand state variables, recording all of them and setting them back to the previous would be extremely costly :/ Instead, the state tracker just resets its internal assumptions -- and expects the "external" code to also not have any assumptions about any state either -- which isn't great from an encapsulation perspective as it leads to corner cases like this, but doesn't cause perf problems.

I'll think of this a bit more, maybe the bound framebuffer is a special enough state (due to how setViewport() works, and due to how the default framebuffer works) that it may be worth to save and restore it.