Fix OpenGL texture coordinate pointer not being updated

[binary1248]

Fix OpenGL texture coordinate pointer not being updated when the array enabled state changes but the RenderTarget's cache state doesn't.

https://en.sfml-dev.org/forums/index.php?topic=22574.0

Test with https://gist.github.com/FRex/b04a5de124d2ce82a2a859bc345ae1a1

[FRex]

The check is now overzealous and intermixed texture and untextured cached draws will cause constant rebinding of the 2 or 3 pointers to same values. Maybe just do:

 if (enableTexCoordsArray || useVertexCache)
                glCheck(glTexCoordPointer(2, GL_FLOAT, sizeof(Vertex), data + 12));

Always bind vertex cache texture pointer too and rest of the code stays the same, that way intermixed cached draws require 0 pointer bindings. It's not perfect but I think it fixes the bug and is no worse (or even better) than your more complex code for almost all cases (the only worse one I can think of is single cached draw surrounded by uncached ones, that'd set this texture pointer needlessly).

[binary1248]

The new version should reduce rebinding to a minimum in all cases.

[FRex]

I didn't take a good look yet (it's starting to look a bit too complex for my liking) but speaking of 'all cases' (I noticed this when I read your first fix): what if someone does something like draw an uncached thing few times in a row? This is totally unaccounted for right now.

I do that very often right now in one place: drawing tons of circles, 1 per each bullet I have, I'll probably do quads via a VertexArray there later to have textures and for performance, etc. but it's still as valid of a use case as this here. The pointers are the same so there should be no binding calls for them but there of course are.

We could add a private ensurePointers function that takes a vertices pointer and whether or not texturing is enabled. We'd also need to keep track of two things: last vertices pointer and if texturing was ever enabled for it. Basically if pointer changes: bind color and vertices and remember that there is no texturing enabled for that pointer, then if texturing is enabled and texturing wasn't enabled for it yet, bind texture coords too and set that remember texturing enabled to true. See this example: https://gist.github.com/FRex/6f80ef13eb12511d4d558162729b8709

This would break ABI (good target for 2.6 then?) because we'd need an extra field or two in RenderTarget but it'd make it handle multiple non-cached draws and it'd make the code much simpler than it is now by trying to cover the edge cases: just call this function with vertices, user's or our cache's, no edge cases.

Should I ask this on the forum?

[binary1248]

In the case you draw something uncached multiple times in a row, the first block would be entered every single time because !useVertexCache is true.

[FRex]

Yes?
I meant we should maybe optimize that use case too (since this all is optimization for people who don't use just VertexArray), I often do things like:

sf::CircleShape sha;
//etc.
for(const Bullet& b : bullets)
{
    sha.setPosition(b.position);
    target.draw(sha);
}

I can't imagine I'm the only one.

Let's not mix this improvement discussion with the bug fix discussion of this PR, here's a thread: https://en.sfml-dev.org/forums/index.php?topic=22586.0

[binary1248]

Laurent implemented the vertex cache solely to optimize drawing polygons consisting of four vertices or less. Everything else will take the normal path.

If someone cared about squeezing every bit of performance out of their code, they would batch themselves anyway and not rely on SFML to do some kind of guess work for them. There have been many discussions about batching for a long time and they mostly lead to the conclusion that the user should do it themselves.

[LaurentGomila]

I have to admit, the caching logic can be hard to follow if you haven't touched it for a while. But your fix looks good.