cginternals / globjects

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

State has wrong behavior #329

Open X-Ryl669 opened 7 years ago

X-Ryl669 commented 7 years ago

This code:

auto currentState = globjects::State::currentState();
[...] // No state related code here

// Then change some state
globjects::ref_ptr<globjects::State> state = new globjects::State(globjects::State::ImmediateMode); // Apply immediately all changes
state->pixelStore(gl::GL_UNPACK_ALIGNMENT, 1); // In components size
state->pixelStore(gl::GL_UNPACK_ROW_LENGTH, width/3); // In components size

[...]
currentState->apply();

Trigger this GL error:

error: 0x500, high severity (API)
        GL_INVALID_ENUM error generated. Polygon modes for <face> are disabled in the current profile.

The backtrace for this error is:

#0  Stitcher::Application::__lambda0::operator() (__closure=0x10ed440, message=...) at ../src/Application.cpp:478
#1  0x00000000005b3f6d in std::_Function_handler<void(const globjects::DebugMessage&), Stitcher::Application::initSystem()::__lambda0>::_M_invoke(const std::_Any_data &, const globjects::DebugMessage &) (__functor=..., __args#0=...)
    at /usr/include/c++/4.8.2/functional:2071
#2  0x0000000000623649 in globjects::DebugImplementation_DebugKHR::debugMessageCallback(gl::GLenum, gl::GLenum, unsigned int, gl::GLenum, int, char const*, void const*) ()
#3  0x00007fffece62b18 in ?? () from /lib64/libnvidia-glcore.so.367.57
#4  0x00007fffece62c60 in ?? () from /lib64/libnvidia-glcore.so.367.57
#5  0x00007fffece62f22 in ?? () from /lib64/libnvidia-glcore.so.367.57
#6  0x000000000068e1c2 in glbinding::Function<void, gl::GLenum, gl::GLenum>::call(gl::GLenum&, gl::GLenum&) const ()
#7  0x00000000007383e2 in gl::glPolygonMode(gl::GLenum, gl::GLenum) ()
#8  0x000000000062c851 in globjects::State::apply() ()

Clearly, it's dealing with polygon mode which I've not touched in my code. I wonder if the "currentState" is a good idea at all. It's slow (because it loops other all states to restore them). I wonder if a per-thread storage list of modified state wouldn't be better. That is, any state changed with a State instance would append the previous state to a TLS's list (if it's not found in the list beforehand, obviously), and a static restoreState() would pop all item from the TLS's list to restore all the current modified state and not all the states like it's done currently, thus avoiding the error written above.

scheibel commented 7 years ago

You're right, a fine-grained handling of states would be best. We actually had plans to create state-stacks from which you can push and pop that would compute the actual difference for restoring the state (https://github.com/cginternals/globjects/pull/149). However, to compute differences we have to know the ground-truth and that is what globjects::State::currentState is about.

I assume you use a core profile, so polygon modes are disabled?

X-Ryl669 commented 7 years ago

However, to compute differences we have to know the ground-truth and that is what globjects::State::currentState is about.

Not necessarly. It depends where you want to pay the cost for state tracking. Either you pay the cost on first use (that is, you need to search for a state change in your state's stack and if not found, you'll have to query the driver for the "current" state's value), or on initialization (like the currentState does). For the fastest state changing, the latter is best, since you bootstrap from a known state (and as such, state change will be 1:1 to what's done with standard gl-func based coding). The former is more intuitive through, since you don't care about having to deal with a currentState anymore.

In general case however, if you expect some state, you should set it (and not rely on whatever driver's default value that could be modified by any library used), so maybe an hybrid approach would be better where you can create a "default" state stack value, you can restore anytime. This would avoid the need to searching the previous state every time you change a state.

Yes, I'm using a core profile.