Wrap use of RenderTarget.setActive() for correctness.

[NQNStudios]

A few months ago when I asked for help with a graphics bug in the SFML Discord, folks there immediately thought the problem must be our use of RenderTarget.setActive() and setActive(false) all over the place. Apparently most SFML devs use those functions almost never.

The documentation of setActive() says this:

This function makes the render target's context current for future OpenGL rendering operations (so you shouldn't care about it if you're not doing direct OpenGL stuff). A render target's context is active only on the current thread, if you want to make it active on another thread you have to deactivate it on the previous thread first if it was active. Only one context can be current in a thread, so if you want to draw OpenGL geometry to another render target don't forget to activate it again. Activating a render target will automatically deactivate the previously active context (if any).

So, setActive() is only needed prior to direct OpenGL operations, and when it is called, the state of being "active" is snatched from whatever had it before. That means we're calling it in tons of unnecessary places, which can potentially cause a place where it is necessary, to not be active as expected.

I decided to wrap the setActive() function in a way that tracks which RenderTarget is the active one, and makes it illegal to activate a different target before the other one is finished. This led me to seeing that we could remove almost all of our setActive() calls harmlessly, and only have it called within functions that do OpenGL stuff.

Honestly I thought fixing up the usage of setActive() would magically fix #602 because by all appearances it has to do with texture/buffer corruption somehow. It did not.

Maybe display() is actually the key to that problem...

[NQNStudios]

Oh -- I think we were also completely mistakenly calling setActive() to try to manage window focus, when there's a different function for that: requestFocus()

Request the current window to be made the active foreground window.

At any given time, only one window may have the input focus to receive input events such as keystrokes or mouse events. If a window requests focus, it only hints to the operating system, that it would like to be focused. The operating system is free to deny the request. This is not to be confused with setActive().

And this would explain several issues I'm gonna dig up and link here.

#183 #168 #152 #60

[NQNStudios]

I think the logic behind this makes sense, but suddenly there are more graphical errors than there were before, including the window losing focus during combat and Visual Studio coming to the front. 🤦‍♀️ Very frustrating.

[NQNStudios]

I can't give up on this one because at the very least, our current setActiveRenderTarget() function is useless -- it's just calling subclass functions on 2 subclasses when casting to subclasses is not necessary at all.

[CelticMinstrel]

This looks a little questionable. Adding an extra "must enable GL" parameter doesn't make much sense – what's wrong with just calling setActive unconditionally there? It's clearly doing GL stuff. And I notice there's at least one place where you removed a call to setActive directly above a GL line.

The overall goal of removing all but the most needed setActive calls seems fine though. I bet none of the setActive(false) calls are required, and setActive(true) is only needed in a few places.

[NQNStudios]

I was thinking that for clip_rect() and clip_region(), the render target would have to stay active as long as the scissor effect is in use, and give up being active when undo_clip() is called.

[CelticMinstrel]

Hmm, maybe. I actually have no idea. Things like glScissor do set a global variable somewhere, so that might be part of the context and still persist if you switched to a different context and back, but I'm not really sure.

[NQNStudios]

Well, seeing as this didn't fix anything, and I can't substantiate a connection between the perceived problem and a reproducible bug, I'm gonna close it. Might circle back later with a better-informed approach.