Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
TrevorCash opened this issue Aug 8, 2023 · 3 comments
Open
Projects

Comments

@TrevorCash
Copy link

TrevorCash commented Aug 8, 2023

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
Copy link
Owner

mosra commented Aug 9, 2023

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).

@mosra mosra added this to TODO in GL via automation Aug 9, 2023
@TrevorCash
Copy link
Author

TrevorCash commented Aug 9, 2023

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
Copy link
Owner

mosra commented Aug 9, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
GL
  
TODO
Development

No branches or pull requests

2 participants