OpenGL framebuffer management

You know, I've done this once or twice myself, and it's made me a fan of using a second container to accumulate the list of elements to remove, and then a loop over that container...

Not the most efficient answer, perhaps, but far less error-prone than modifying a container during iteration.

Without knowing what type of container this is and how the element type is implemented (doing proper RAII?), it's hard to tell whether it's a bug at all. For std::vector, it would surely be problematic due to iterator invalidation, but e.g. for std::list this is perfectly OK.

The capitalization of the members suggests that the container is no standard container at all, but rather a homebrew thingie (which may or may not invalidate iterators after the erased one).

Just add the missing ++it to the else part?

Just add the missing ++it to the else part?

Oh wow, I didn't even see that one! This only ever works if the first element times out at all.

wintertime is right, ++it was missing. Actually found the bug with gDEBugger when seeing suspicious FBO deletions & creations. Naturally I was totally prepared to blame Nvidia for the performance loss compared to the D3D11 build :)

Yes, it's a custom container, a hash map to be precise. The code should work with any number of element however, it increments the current element's age until reaching the limit, erases it, then goes to the next until no more framebuffers..

obviously you should have written

for (auto it = framebuffers.Begin(); it != framebuffers.End();)
{
    if (it->second->framesSinceUse > MAX_FRAMEBUFFER_AGE)
        it = framebuffers.Erase(it);
    else
        ++(it++)->second->framesSinceUse;
}

Honestly I'd have gone with this instead, seems clearer than tacking in into parenthesis:

for (auto it = framebuffers.Begin(); it != framebuffers.End();)
{
    if (it->second->framesSinceUse > MAX_FRAMEBUFFER_AGE)
        it = framebuffers.Erase(it);
    else {
        ++it->second->framesSinceUse;
        ++it;
    }
}

It honestly still makes me do a double-take on reading, and that's not a good thing. The whole point of a decent coding standard is that right code should look right and wrong code should look wrong, and that prefix increment isn't helping things at all.

Change it to a postfix increment and it immediately jumps out at you that something's missing.

Honestly I'd have gone with this instead, seems clearer than tacking in into parenthesis

Agreed. We should avoid the parentheses.

for (auto it = framebuffers.Begin(); it != framebuffers.End();)
{
    if (it->second->framesSinceUse > MAX_FRAMEBUFFER_AGE)
        it = framebuffers.Erase(it);
    else
        ++it++->second->framesSinceUse;
}

++it++? .... ugh... the goggles. they do nothing.

Code executed after each frame, ensures not keeping around unused FBO's

Do you really need to do that?

++it++? .... ugh... the goggles. they do nothing.

When your code starts looking like it was written in Brainfuck you know you've crossed a line. The line beyond which you're more machine now than man. Twisted and evil.

++it++? .... ugh... the goggles. they do nothing. :wacko:

When your code starts looking like it was written in Brainfuck you know you've crossed a line. The line beyond which you're more machine now than man. Twisted and evil.

If you strike it down, it will become more powerful than you can ever imagine...

Do you really need to do that?

I create FBO's automatically by color format and resolution (to avoid some Nvidia performance bugs which would appear if I'd just reuse a single FBO) and the cleanup is just for stupid cases, like user resizing the window by dragging, and 100+ framebuffers for different resolutions (eg. 640x480, 641x481, 642x482..) being created and left in the memory.

Now thinking of it, screen resize is probably the only major case where (a lot of) extra unused buffers would be left around, so could as well do a cleanup for that only and not for the general case.

Do you really need to do that?

I create FBO's automatically by color format and resolution (to avoid some Nvidia performance bugs which would appear if I'd just reuse a single FBO) and the cleanup is just for stupid cases, like user resizing the window by dragging, and 100+ framebuffers for different resolutions (eg. 640x480, 641x481, 642x482..) being created and left in the memory.

Now thinking of it, screen resize is probably the only major case where (a lot of) extra unused buffers would be left around, so could as well do a cleanup for that only and not for the general case.

Sounds like your window resize handling needs to be a bit better rather than having a sexy FBO management setup. Cos right now that sexy FBO management is only serving to mask bugs elsewhere.

I create FBO's automatically by color format and resolution (to avoid some Nvidia performance bugs which would appear if I'd just reuse a single FBO) and the cleanup is just for stupid cases, like user resizing the window by dragging, and 100+ framebuffers for different resolutions (eg. 640x480, 641x481, 642x482..) being created and left in the memory.

I see what you mean but "resizing the window" is a pretty specific action. ie, reallocate FBO, delete old one, you'll know which is which in whatever block of code you have for resizing the window. You don't need a LRU queue for freeing FBOs for that, just handle the cases where you have to reallocate FBOs individually (say, resize window, shadow resolution change, SSAO quality change, things like that).

I see what you mean but "resizing the window" is a pretty specific action. ie, reallocate FBO, delete old one, you'll know which is which in whatever block of code you have for resizing the window. You don't need a LRU queue for freeing FBOs for that, just handle the cases where you have to reallocate FBOs individually (say, resize window, shadow resolution change, SSAO quality change, things like that).

This.

Also, your OS's window manager - assuming it's halfway civilized - is going to enable you to detect when a resize event completes, so you shouldn't need to be destroying and recreating FBOs while the resize is still happening. Look at e.g WM_EXITSIZEMOVE on Windows.

Yeah, in response to a resize event I might be (re)creating various textures, such as gbuffers. The difference is that a texture actually interests me and I manage them explicitly (I naturally wouldn't be keeping the old resolution gbuffer around), but FBO's don't interest me at all, they're just an internally-used tool to allow the graphics backend to always present a Direct3D-like API to the user (ie. SetRenderTarget(), SetDepthStencil()) and therefore ideally managed automatically.

Change it to a postfix increment and it immediately jumps out at you that something's missing.

Normally I use postfix exclusively but I know some C++ programmers tend to scream at you if you ever use postfix ¯\(º_o)/¯ (of course it's just a bunch of them but it's still annoying)