Regarding the VertexBuffer implementation in Renderables..

[eigenbom]

This commit has made all the renderables use a vertex buffer if available. Which is great; however, before this change drawables were quite lightweight, and we could create one of these drawables at the last moment. For instance...

loop {

   // construct sprite
   sf::Sprite sprite {...};
   target.draw(sprite);  
}
 

But now, afaict, this will call glGenBuffers and then glDeleteBuffers every frame. If this change is to be preserved I think the docs should not call sf::Sprite lightweight anymore.

[eigenbom]

And here's some real-world data: after manually disabling vertex buffers in sf::Sprite, my application went from 30-50fps to 60-70fps. Most of my use cases are of the form above, where I construct a sf::Sprite during my render pass.

Additionally, when I turn on debugging in OpenGL, I get many performance warnings regarding the vertex buffers, as I think they are eating up all the memory.

[binary1248]

I really don't see why, from a technical perspective, what you are trying to achieve is only doable by creating a temporary sprite every frame...

Assuming that the user should be able to actually recognize something on their screen, said sprite would have to be drawn over multiple frames anyway, meaning that it would have to be recreated multiple times with very similar if not identical parameters. Except for special cases (static linking combined with some heavy duty LTO), the call to the constructor of sf::Sprite and indirectly its members will not be optimized away and will also have some kind of CPU performance impact. The fact that it doesn't stick out (yet) in a measurement does not mean it doesn't exist.

While I am not the original author of the documentation, I can say that the term "lightweight" came in when switching from SFML 1.6 to SFML 2.0 in order to emphasize the fact that sf::Sprite no longer holds the huge chunk of texture data like it used to in SFML 1.6. Making any other forms of performance guarantees, especially when relying on the graphics hardware/drivers to do the right thing, is not such a good idea. Something that runs fast on one system does not have to run fast on another, especially when it comes to OpenGL.

When considering such optimizations, we always consider what kind of effects it might have on the behaviour and performance of the program. It was already clear back then that frequent creation of sf::Sprites and all the other drawables would cause VBOs to be created and destroyed all the time. Based on inspection of publicly available code, it was clear that this usage pattern wasn't really something to take into consideration since performance concious users would, as explained above, reuse objects over a longer period of time, or just forget the pre-made drawables and manage vertex data themselves.

We optimize for best and average case performance. Worst case performance is something no library/API/programming language will optimize for since it is virtually impossible to predict all the ways something can be used incorrectly let alone infeasible to write workarounds for every single case. Unlike algorithms that operate on input data which cannot be known beforehand and have to reckon with the worst, we assume developers do not program randomly and have some kind of motivation to get the best performance out of their application whatever means necessary. As long as there are enough ways to get good performance, we won't let worst case performance regressions hinder any optimizations made to improve the former.

Last but not least, the main reason this change was necessary, regardless of performance impact, is that client side arrays are no longer supported in core OpenGL 3.2+ and OpenGL ES 2+. In order to future proof SFML and be able to gradually move in this direction, we had to add an implementation that uses VBOs for vertex data storage instead of system RAM. Disabling this change in any way would lock the implementation and your application to using legacy OpenGL and/or OpenGL ES 1.0 forever.

Future optimizations can always be made to optimize even such usage patterns. Making use of a single large VBO from which we allocate fragments to every drawable is something I have had on my list for quite some time, I just haven't got around to doing that just yet. The necessary backend changes to make this possible are being worked on already, it's just a matter of time until it sees the light of day.

[Laurent]

While I am not the original author of the documentation, I can say that the term "lightweight" came in when switching from SFML 1.6 to SFML 2.0 in order to emphasize the fact that sf::Sprite no longer holds the huge chunk of texture data like it used to in SFML 1.6.

sf::Sprite never held the texture data, even in SFML 1. It has always worked the same way, except that sf::Texture was sf::Image back then.
As far as I remember, the term "lightweight" was actually used for this very specific reason that sf::Sprite instances could be copied around, instanciated on the fly, etc. with almost no performance penalty. So eigenbom is right about it, and the documentation should be updated to avoid further confusion.

I can understand users wondering or complaining about this change, as it affects how SFML must be used. Before that, you could use SFML drawables however you wanted, with no difference in performances. Now the API still allows all kind of usage, but not all of them give good performances, so you have to read more carefully the documentation, and think about how to manage that stuff. That's not a bad thing, but this is important enough to be emphasized in the documentation -- the best would be to enforce good usage with the API, but in this case it seems rather difficult

[eigenbom]

I really don't see why, from a technical perspective, what you are trying to achieve is only doable by creating a temporary sprite every frame...

I have a commercial game that has used SFML for 8 years now -- I'm only trying to update to the next minor version of SFML. I'm not sure the SFML team wants it's users to have to re-architect parts of their code for this minor version update. In any case, at least please consider this as a preliminary negative report for this change, instead of dismissing it as an outlier or a worst case usage.

[maxdeviant]

For what it's worth, I'm doing something very similar in my own engine:

pub fn draw_image(ctx: &mut Context, image: &Image, params: DrawImageParams) {
    let mut sprite = SfSprite::with_texture(&image.texture);
    sprite.set_position(SfVector2f::from(params.position));

    if let Some(clip_rect) = params.clip_rect {
        sprite.set_texture_rect(&clip_rect.into());
    }

    if let Some(color) = params.color {
        sprite.set_color(&color.into());
    }

    if let Some(scale) = params.scale {
        sprite.set_scale(scale);
    }

    ctx.window.draw_sprite(&sprite, SfRenderStates::default())
}
 

Regardless of whether or not this is a "correct" usage of sf::Sprite, it seems like a pretty major change to deprecate this pattern in a minor version.