Revert change to `sf::Drawable::draw` signature which made `sf::RenderStates` a const reference, and make it a copy again

[Bambo-Borris]

Revert change to `sf::Drawable::draw` signature which made `sf::RenderStates` a const reference, and make it a copy again

[danieljpetersen]

For whatever it's worth, I agree with Bambo. The cost of a draw call is enormous, I don't think that the gains are going to meaningfully offset the still enormous cost of a draw call. You'd have to have A LOT of draw calls I imagine, at which point you've already lost.

[vittorioromeo]

sf::RenderStates is quite big as it contains a sf::Transform, plus other 5 members of integer size. We'd be copying sizeof(float) * 16 + sizeof(int) * 5 every time we invoke draw. This could be significant if users are drawing many drawables in a loop.

Is the explicit copy really that annoying? If anything, it shows that quite a bit of data is being copied explicitly.

This API is kinda weird though, maybe this is a symptom of a larger problem? Can RenderStates be taken by non-const reference?

[Bambo-Borris]

For whatever it's worth, I agree with Bambo. The cost of a draw call is enormous, I don't think that the gains are going to meaningfully offset the still enormous cost of a draw call. You'd have to have A LOT of draw calls I imagine, at which point you've already lost.

That is fair, and you are right.

Still, I'd take this as an opportunity to figure out why the API is so awkward in the first place before jumping on a revert. Maybe we can do something even better, but not opposed to a revert.

While I feel there is merit in deeper interrogation of the drawing API, I think it's largely out of the scope of the issue raised here. I think realigning the API to work as it did in 2.0 via a revert so that the ergonomics are kept the same presently is worth it. And revisiting the API discussion when the move away from legacy GL is an option is likely the best time. We're better off bringing this back to how it was before prior to a release of 3.0.

[eXpl0it3r]

What hasn't been mentioned is that the fix was original done because it popped up in static code analysis tools, see the original PR #2008
Unfortunately, due to Coverity having the worst UI ever, I don't know if we can still see what it complained about exactly. Maybe @binary1248 still remembers or can dig up the original report.

Performance is in my view a non-issue. We've been living with this in SFML 2 for the past ten years or so and I don't remember having people run into a bottleneck with this part ever.

For me it's generally fine to revert the change.

Yes, maybe there's a more ergonomic API and since it probably would be breaking, it would be the "right time". On the other hand, it might also not be the right time, as we may come up with quite a different rendering API for SFML 4 and having SFML 3 be a quality of life release for C++17 support, I believe we don't need a major API revamp for this.

[binary1248]

It was SonarCloud and yes... they truly have one of the most unusable user interfaces in recent memory.

[ChrisThrasher]

While I'm all for some theoretically performance gains, we're not sure if those performance gains are meaningful compared to the very real ergonomic issues this change has introduced. If we're going to make the library more annoying to use, we need a bit more evidence that the performance gains are worth it. I'm open to reverting this.

[Bambo-Borris]

sf::RenderStates is quite big as it contains a sf::Transform, plus other 5 members of integer size. We'd be copying sizeof(float) * 16 + sizeof(int) * 5 every time we invoke draw. This could be significant if users are drawing many drawables in a loop.

Is the explicit copy really that annoying? If anything, it shows that quite a bit of data is being copied explicitly.

This API is kinda weird though, maybe this is a symptom of a larger problem? Can RenderStates be taken by non-const reference?

I would suppose there are implications in it being non-const in effecting draw calls subsequent to your custom drawable. I think the API is rather strange but has previously made for nice ergonomics. Anyone making custom drawables is likely to be manipulating the RenderStates object and thus they will have to eat the cost of the copy anyway. And honestly the draw itself is more costly (also more problematic in immediate mode rather than the model favoured by later GL versions and other rendering APIs. I also think it's likely that powerusers who make use of sf::Drawable likely have some scheme of batching which means the cost of the copy is not so bad but the removal of the explicit copy is favourable for readability. And in general the issue with the copy on lots of drawables would be better sorted with batching (considering that the GL draw call itself is the largest overhead).

I may be speaking largely for myself in this issue, which is why I raised it, maybe I should merely avoid the sf::Drawable base but I've utilised it in many projects for the smooth ergonomics it offers when performing a draw on my own custom types (as it was intended for). Would be nice to see some other heavy SFML users chime in with their viewpoint.

[vittorioromeo]

For whatever it's worth, I agree with Bambo. The cost of a draw call is enormous, I don't think that the gains are going to meaningfully offset the still enormous cost of a draw call. You'd have to have A LOT of draw calls I imagine, at which point you've already lost.

That is fair, and you are right.

Still, I'd take this as an opportunity to figure out why the API is so awkward in the first place before jumping on a revert. Maybe we can do something even better, but not opposed to a revert.

[vittorioromeo]

For whatever it's worth, I agree with Bambo. The cost of a draw call is enormous, I don't think that the gains are going to meaningfully offset the still enormous cost of a draw call. You'd have to have A LOT of draw calls I imagine, at which point you've already lost.

That is fair, and you are right.
Still, I'd take this as an opportunity to figure out why the API is so awkward in the first place before jumping on a revert. Maybe we can do something even better, but not opposed to a revert.

While I feel there is merit in deeper interrogation of the drawing API, I think it's largely out of the scope of the issue raised here. I think realigning the API to work as it did in 2.0 via a revert so that the ergonomics are kept the same presently is worth it. And revisiting the API discussion when the move away from legacy GL is an option is likely the best time. We're better off bringing this back to how it was before prior to a release of 3.0.

I'm not talking about a major change to the API. I'm saying that if -- for instance -- it makes more sense to pass sf::RenderStates as a non-const reference, this is the right time to make the change (prior to 3.0 release).

[ChrisThrasher]

See #2949