Another useful helper in OpenGLHelper


#1

Hi,

The current code doesn’t deal with OpenGL errors. Since they are quite sparse, probably we have been lucky ignoring them, but it can be crashy.
For example, if a glDisableClientState() fails, you’ll have a “pointer to buffer” still enabled, leading to a crash on the next glDrawArray call (and good luck to debug this).

Usually the good method to check for OpenGL error is to call glGetError() after each GL call. Since this can be very painful, I propose the following code:

// Example usage code
{
   GLGuard guard = MakeGLGuard(glDisable, GL_TEXTURE_2D);
   glEnable(GL_TEXTURE_2D);
   // Some gl call here we don't actually check for error
   [...]
}  // If any of the GL call fails, the guard will wake up and disable the texture state

// Another example
{
   GLGuard guard = MakeGLGuard(glDisableClientState, GL_TEXCOORD_ARRAY);
   glEnableClientState(GL_TEXCOORD_ARRAY);
   glTexPointer(...);
   glDrawArrays(...);
   guard.act();    // Force disabling the state anyway, regardless of the error state (so you don't write thing twice)
}

The GLGuard implementation:

struct GLGuardBase
{
    void act() const noexcept { dismissed = false; forced = true; }
    bool logOpenGLError() const noexcept
    {
        static String commonErrorCode[] = { "Invalid enum", "Invalid value", "Invalid operation", "Stack overflow", "Stack underflow", "Out of memory" };
        GLenum error = glGetError();
        if (error) 
        {
            Logger::outputDebugString(TRANS("OpenGL Error: ") + String((error - GL_INVALID_ENUM < numElementsInArray(commonErrorCode)) ? commonErrorCode[error - GL_INVALID_ENUM] : String("Unknown error")));
            return true;
        }
        return false;
    }
    // Construction and destruction is not allowed except for child classes
protected:
    GLGuardBase() : dismissed(true), forced(false) {}
    /** Copy constructor, undo the initial scope guard behavior */
    GLGuardBase(const GLGuardBase& other) : dismissed(other.dismissed) { other.dismissed = false; other.forced = false; }
    /** Destructor 
        It is not virtual because from point of declaration to point to destruction
        it is a child class which is created and the compiler directly call the right 
        child destructor (it also avoid setting up and using virtual function pointer) */
    ~GLGuardBase() {}

    // Member
protected:    
    /** When set to true, the destructor ignore the asked clean up */
    mutable bool dismissed, forced;

private:
    /** GL Guard are not moveable */
    GLGuardBase& operator= (const GLGuardBase&);
};
// Very very important to use this instead of the former
typedef const GLGuardBase & GLGuard;

/** Real implementation for a function with no parameter */
template <typename Fun>
class GLGuardImpl0 : public GLGuardBase
{
    Fun             function;
    // Construction and destruction
public:
    /** Unique constructor */
    GLGuardImpl0(const Fun& fun) : function(fun) {}
    /** Destructor - catch any pending exception to continue cleaning for any other guard */
    ~GLGuardImpl0() { if (forced || (!dismissed && logOpenGLError())) try{ function(); } catch (...) {} }
};

/** Real implementation for a function with 1 parameter */
template <typename Fun, typename Parm>
class GLGuardImpl1 : public GLGuardBase
{
    Fun             function;
    const Parm      parameter;
    // Construction and destruction
public:
    /** Unique constructor */
    GLGuardImpl1(const Fun& fun, const Parm& parm)   : function(fun), parameter(parm) {}
    /** Destructor - catch any pending exception to continue cleaning for any other guard */
    ~GLGuardImpl1()    { if (forced || (!dismissed && logOpenGLError())) try{ function(parameter); } catch (...) {} }
};

/** Real implementation for a function with 2 parameter */
template <typename Fun, typename Parm, typename Parm2>
class GLGuardImpl2 : public GLGuardBase
{
    Fun             function;
    const Parm      param1;
    const Parm2     param2;
    // Construction and destruction
public:
    /** Unique constructor */
    GLGuardImpl2(const Fun& fun, const Parm& parm1, const Parm2& parm2) : function(fun), param1(parm1), param2(parm2) {}
    /** Destructor - catch any pending exception to continue cleaning for any other guard */
    ~GLGuardImpl2()    { if (forced || (!dismissed && logOpenGLError())) try{ function(param1, param2); } catch (...) {} }
};

template <typename Fun>
inline GLGuardImpl0<Fun*>                                MakeGLGuard(const Fun& fun)   { return GLGuardImpl0<Fun*>(fun); }
template <typename Fun, typename Parm>
inline GLGuardImpl1<Fun*, Parm>                          MakeGLGuard(const Fun& fun, const Parm& parm) { return GLGuardImpl1<Fun*, Parm>(fun, parm); }
template <typename Fun, typename Parm, typename Parm2>
inline GLGuardImpl2<Fun*, Parm, Parm2>                   MakeGLGuard(const Fun& fun, const Parm& parm, const Parm2& parm2) { return GLGuardImpl2<Fun*, Parm, Parm2>(fun, parm, parm2); }

If it’s hard to understand, I’ll explain in few words how it works.

  • The basic idea is to create a templated object whose destructor calls a function pointer set by the overloaded MakeGLGuard function.
  • All these objects derives from a GLGuardBase so they advertize a common interface, but no virtual table is used (for performance). This is possible because when taking a const reference on a temporary object, the compiler will use the scope of the reference to extend the life of the temporary. So when you do: “const Obj & a = Obj(“c”);”, the temporary “c” destructor will be called when “a” goes out of scope. This is the hardest part to understand, but you are already using it every day when you use const reference in any function’s parameter.
  • You must use the “GLGuard” typedef in order to see the above behaviour (or the more explicit and painful “const GLGuardBase &”).

If you think it’s worth it, or have any comment on this code, please do so, I put this code in public domain.


#2

Up.


#3

Cute idea…

The most worrying thing you said was “if glDisableClientState() fails”… Can that actually happen? If so, that’s a real nightmare because I really don’t want to waste time calling glGetError for trivial functions like that!

Not 100% sold on your solution though - it seems messy, and I don’t like the way the parameters get stored and re-used, it just feels like a lot of unnecessary code will be generated. But it could be done really neatly with lambda functions, the auto keyword and variadic templates… maybe it’s an idea that should wait until c++11 is more widely used?


#4

Another thread could update/delete the context for one (I know about Context lock, but “shit happens”), or, more insidiously, the communication between the OpenGL client and the driver can be interrupted. OpenGL is not always running locally on your computer.

I think you’ve missed one important point. OpenGL error are accumulated in a stack, until you pop them up with glGetError().
This means you don’t have to deal with calling glGetError for trivial functions, but after a “crashy” state change.

For example, if you forget to disable a state, the next call to glDrawArrays/Elements will crash (due to poor management in AMD’s and NVidia OGL driver).
It’s almost impossible at the time of the crash to figure out the reason with the stack trace, one has to log all OGL call to guess the reason.

I’d like to remind you that both crashes in the Juce OGL renderer and demo were due to leaking states.
IIRC, the first one was a glEnableClientState(TEX_COORD) on the 2th texture unit that wasn’t disabled.

In fact, it’s in the C++98 standard, but you usually don’t think as this. When you do:

void func(const Obj & obj)
{
   [...]
}

func(Obj(32));

You’re applying the same idea that’s shown here. The const reference in the function parameter will force the compiler to extend the lifetime of the Obj(32)'s temporary.
I don’t think any unnecessary code will be generated compared to adding “if (glGetError()) glWhatever(Param)” everywhere (and forgetting most of them).
There’s a lot of place for optimization here, and I think a good compiler will inline the whole stuff to the strict minimum.


#5

Thanks, I’ll ponder this.

Sorry, I didn’t notice what you were doing using the const-refs to hold the parameters until they were used later. That’s much better, and hopefully the compiler could do a pretty good job of optimising it.

Actually, that’s not quite the same thing - in any statement, all temporary objects are guaranteed to last until the end of the statement, so in the case of a function call, its parameter objects could never be deleted until the function call has completed anyway. I think that the use of a const ref to extend the scope of a returned object is actually a special case.