Make sf::VertexArray a plain struct with public members

[therocode]

At the moment sf::VertexArray has two data members std::vector<Vertex> m_vertices and PrimitiveType m_primitiveType which are encapsulated by being private along with some public methods like a plain get/set pair for primitive type, and a limited set of vector-like accessors/modifiers for vertices.

My argument: This encapsulation buys nothing for the user and actually severly limits the power of sf::VertexArray unnecessarily. Even conceptually, this is just a list of vertices + a primitive type. This is perfectly represented by a public std::vector<Vertex> and a public PrimitiveType.

• There is no validation behind the API that ensures that the primitive type and vertices are set in a way that makes sense to one another.
• There is no real restriction on what data can be put in, since primitive type is a plain get/set which is fully permissive. And for the vertices, you have clear() and push_back() and operator[] which all also end up being fully permissive. So nothing significant is encapsulated.

My proposal:

• Change class VertexArray into struct VertexArray
• Make the members public, and rename them to vertices and primitiveType (stripping m_ since they are now part of the pulbic API)
• Remove:
• • getVertexCount (now .vertices.size())
• • operator[] (now .vertices[])
• • clear() (now .vertices.clear())
• • resize() (now .vertices.resize())
• • append() (now .vertices.push_back())
• • [set/get]PrimitiveType() (now .primitiveType and .primitiveType = )
• Keep:
• • virtual void draw(), will still inherit drawable and should hence be kept.
• • getBounds(), useful piece of functionality that is not provided by the now public members.

The main benefit of this is that the user gets the full power of the vector API when working with vertices:

• Iterators (use with <algorithm>, range-based for, fits with 3rd party APIs, etc)
• .reserve() for allocation-free push_back
• .at() safe indexing
• Moving of vertex data in/out of the struct, useful f.ex. if vertices arrive from other parts of the codebase
• ...and so on

Another benefit is familiarity. Every C++ programmer knows how to use vector. Why have a stripped down restrictive version of it that is different? .getVertexCount() and .append() instead of the familiar .size() and .push_back().

There's also less work for the SFML team, as it shrinks the API surface (while giving more power to the API). No more debates around if sf::VertrexArray should add iterators, or whatever other functions people are missing from vector. Just delete, make public, and done.

Next to all these benefits, I have a hard time justifying the existence of this encapsulation layer. I don't even know what concrete benefits it actually provides for such a simple data structure like this one, which is not really a "new abstraction" but more a composition between a list of vertices and a primitive type. A struct with these two represents this perfectly, cleanly and transparently with zero surprises. And note how the struct is still a dedicated new type, so it's still possible to add utility functions for it (such as the getBounds()) and it can still absolutely inherit Drawable.

Thoughts?

[vittorioromeo]

Alternatively, we could publicly derive from std::vector. Haven't thought of all the implications but it would expose the entire std::vector API, avoid the .vertices verbosity, and provide an easier migration path from SFML2.

[therocode]

It's as far as I know unidiomatic to inherit from types that are not designed for this purpose, and that'd be a more complex solution kept around forever just for the sake of a brief migration period from SFML 2? That's hard to justify IMO.

I also don't see how .vertices is a "verbosity", really. It's really natural to use .blah to access an inner member when composition is used.

Lastly, I doubt std::vector's destructor is virtual so I think that would mean that it's not destroyed properly if VertexArray is stored as a Drawable. Not entirely sure on this one though.

[vittorioromeo]

"Unidiomatic" doesn't necessarily mean "wrong", we should at least consider the option. Typing .vertices every time you want the access the vector is objectively more verbose than the status quo, and works against the idea of sf::VertexArray having a container-like API. Also, sf::Drawable's destructor is virtual, so that should care of the destruction concerns, but I need to double-check that.

Yet another option is removing sf::VertexArray altogether, and creating a ref type that can cheaply be instantiated as a temporary:

class VertexArrayRef : Drawable
{
private:
    const std::vector<sf::Vertex>& m_vector;
    const sf::PrimitiveType m_primitiveType;
    
public:
    [[nodiscard]] explicit VertexArrayRef(
        const std::vector<sf::Vertex>& vector, sf::PrimitiveType primitiveType);

    void draw() const override;
    sf::FloatRect getBounds() const;
};

The user would then use it in conjunction with an existing vector (or any other contiguous container, if we store two sf::Vertex*) as follows:

std::vector<sf::Vertex> myVertices{/* ... */};
// ...
myRenderWindow.draw(sf::VertexArrayRef{myVertices, sf::PrimitiveType::Tris});

[therocode]

Typing .vertices every time you want the access the vector is objectively more verbose than the status quo

Not with the definition of verbose as "unnecessarily long". It's just longer, but that's not necessarily a bad thing. How often do you expect users to access these vertices? The little extra length is a cheap price for the no-surprises approach that also gives inevitably more power.

Also, sf::Drawable's destructor is virtual, so that should care of the destruction concerns, but I need to double-check that.

I don't think that's enough. If the std::vector destructor is to be called, it needs to be in a v-table and I don't think you'll get this without std::vector being a virtual type.

This VertexArrayRef approach seems even more complicated to me. Introducing even more surprises than the current approach because now you suddently need two variables to store the vertex array and you introduce the issue of use-after-free and a whole load of other questions are dumped on the table like how copy construction/assignment should work, and how this would fit into potential future batching APIs etc etc. I really don't see the justification for this complex approach.

[vittorioromeo]

I don't think that's enough. If the std::vector destructor is to be called, it needs to be in a v-table and I don't think you'll get this without std::vector being a virtual type.

Empirical test: https://gcc.godbolt.org/z/fKGMaaqEs

I really don't see the justification for this complex approach.

The current approach with sf::VertexArray is suboptimal as it couples the storage of the vertices with the SFML drawing capability. It prevents users from managing their own storage, using a custom container which is not std::vector, or even drawing a subrange of an existing std::vector as vertices.

The goal of sf::VertexArray is to let people use a range of contiguous sf::Vertex objects as a SFML drawable. To achieve that, sf::VertexArray doesn't need to own that range at all -- it can just reference an existing one.

That approach would make sf::VertexArray very lightweight in terms of API and coupling, and would allow its use with any contiguous range of vertices. Copy operations would have the same semantics as std::string_view.

[therocode]

Empirical test: https://gcc.godbolt.org/z/fKGMaaqEs

Ah, cool 👍 good to know.

As for the rest of your reply, seems like we have very different ideas on what the problem is with sf::VertexArray and what the ideal state is.

... is suboptimal as it couples the storage of the vertices with the SFML drawing capability I agree this is a problem, but it's a different one to the issue raised in this ticket which would be "the problem is that sf::VertexArray has a weak and restricted API which could be solved by making the internals public which would be a quick win that doesn't touch any other design considerations".

Your question re decoupling of Drawable is a good one, and one worthy of investigation but also a much bigger one, and one that can IMO be solved orthogonally to the issue I meant to raise here. F.ex. it would be feasible to make the internals public as proposed, and in a subsequent change, solidify sf::VertexArray as a more lower-level construct by removing the Drawable inheritance and then make a higher level generic sf::Geometry drawable or something. A totally different and broader discussion IMO.

Also, regarding making sf::VertexArray operate on two pointers so that it can reference other storage.... Is this really a worthy goal? Have you ever needed this? In my experience, vector is the storage that people use for performant tight storage anyway, and what other storages are you expecting that also have contiguous memory? I am not sure about the use cases that makes you want this. But if it's along the lines of "several objects that are individual drawables that use the same vertex data to share the same models" or similar, then IMO keep sf::VertexArray the same as it is now: A lower level owning storage of a list of vertices. Then implement a new drawable sf::Geometry or something, that refers to an sf::VertexArray. That way sf::VertexArray becomes the data owning instance similar to sf::Texture or sf::SoundBuffer, and sf::Geometry would be the equivalent to sf::Sprite and sf::Sound. With that, sf::VertexArray could no longer inherit Drawable.
Such an arrangement would IMO be cleaner and more SFML-like than sf::VertexArray referencing pointers of other memory.

But again, I'd see all that as an orthogonal discussion.

[Bromeon]

Good suggestion! I'm for this change, but against inheriting from std::vector.

Inheriting from STL types has been discouraged as bad practice in the C++ community for a very long time, and I don't think it's something that users would expect. In addition, we use a different naming convention, so we would have array.push_back() and array.primitiveType in the same class.

Also, it seemed like the majority's view on verbosity and idiomatic C++ was the opposite when it came to std::uint8_t, std::size_t & Co? More verbose code was endorsed with the argument to be more faithful to C++. This also affected the user (directly, and by setting an example in the API). So I'm not following the reasoning now 🤔

---

VertexArrayRef is an interesting idea on the other hand. It might create a small indirection for the average case, probably acceptable. But if we do it, the range should be constructible from a raw pointer as well:

VertexArrayRef(const std::vector<Vertex>& vertices, PrimitiveType primitiveType);
VertexArrayRef(const Vertex* vertices, size_t size, PrimitiveType primitiveType);

Or directly use something integrated with std::begin()/std::end(), although that may make less clear which concrete types it accepts.

[therocode]

My biggest gripe with the VertexArrayRef idea is that it is harder to use. You won't be able to use it without also separately holding an std::vector or otherwise, and you get all the lifetime issues that comes from it too. It also seems a bit weird to me to have the data definition of a vertex array (vertices + primitive type) split up so that the vertices part is owned separately from the primitive type part.

All in all, while it's a useful thing on paper to be able to refer to deferred memory of Vertex*elsewhere, what are the motivating use cases? The notion of "to be able to refer vertex memory elsewhere" is not a use-case. What would a broader use case for this look like? My feeling is that it would be rare.

[Bromeon]

Good points. It also came to my mind that we already have a RenderTarget::draw() overload which takes raw vertex pointer + size, so there's little benefit in creating an extra intermediate type.

void sf::RenderTarget::draw(
 	const Vertex* 	 	vertices,
	std::size_t  	 	vertexCount,
	PrimitiveType  	 	type,
	const RenderStates &  	states = RenderStates::Default 
) 	

Maybe we should really not overthink things and expose the two fields vertices + primitiveType in a struct?

[eXpl0it3r]

I hadn't really had time to read the whole discussion, my counter point to the proposal however is, that it breaks a lot of the API design decisions we've taken in SFML. Beyond "primitive" types like Vector, Rect, Color and Vertex, we're never exposing inner workings of a class. sf::VertexArray is by my definition not a primitive type, as it also derives from sf::Drawable, thus implementing behavior beyond its own class boundary (unlike e.g. Rect::contains() or Color::toInteger() etc.).

If the argumentation should then be to remove the sf::Drawable inheritance, then I'd probably argue much more, that the type itself shouldn't exist, just to combine a list vertices and a primitive type.

[Bromeon]

If I remember correctly, we had a similar discussion for sf::String quite a while ago. There are quite a few operations a class must support in order to be useful as a container, and sf::VertexArray is clearly very limited in that sense.

So the alternative might be adding lots of operations (iterator support, insertion/deletion, etc). But it would still only be our poor-man's version of std::vector. That doesn't mean it's wrong per-se.

The question we should ask ourselves: do we gain something from the encapsulation, maybe reserve the right to change things in the future? Realistically we will never use anything different than std::vector for the internal storage, so maybe other parts of the implementation that can change?

[therocode]

I hadn't really had time to read the whole discussion, my counter point to the proposal however is, that it breaks a lot of the API design decisions we've taken in SFML. Beyond "primitive" types like Vector, Rect, Color and Vertex, we're never exposing inner workings of a class. sf::VertexArray is by my definition not a primitive type, as it also derives from sf::Drawable, thus implementing behavior beyond its own class boundary (unlike e.g. Rect::contains() or Color::toInteger() etc.).

If the argumentation should then be to remove the sf::Drawable inheritance, then I'd probably argue much more, that the type itself shouldn't exist, just to combine a list vertices and a primitive type.

This to me sounds like a pure argument out of principle, which I think isn't the right way. If we stop something good, out of pure principle, then the principle is not useful and should not be adhered to. If there are real tangible reasons to not do it however, then that's of course a different thing but those concrete things can be talked about. Principle for the sake of principle alone just seems like tying our hands behind our backs.

I would personally agree though as a separate issue that sf::VertexArray should not inherit sf::Drawable as it seems more like a lower-level building block than a drawable in its own right. And as stated previously in this discussion I think it could make sense to consider introducing a higher level drawable that references an sf::VertexArray. sf::Geometry g{someVertexArray}; for example. That way you get a higher level drawable that can be positioned/transformed but reference the same static geometry set. This comes closer to what was discussed above as well with referencing, without turning sf::VertexArray into a ref type.

[eXpl0it3r]

This to me sounds like a pure argument out of principle, which I think isn't the right way. If we stop something good, out of pure principle, then the principle is not useful and should not be adhered to. If there are real tangible reasons to not do it however, then that's of course a different thing but those concrete things can be talked about. Principle for the sake of principle alone just seems like tying our hands behind our backs.

The point is much more that to get a consistent API and build up expectations for consumers that hold true for all APIs, such a decision can't be made in isolation. So yes, it's an argument out of principle, but the principle isn't something to just be ignored. If we want to steer in a different direction from the current API design, it needs to be an explicit decision and one that's done across the whole API and not just for a single instance.

Said differently, from an outside look it would be rather confusing having some classes as pure structs with public members and some classes with abstractions and overall no guiding principle as to when is used what.

[danieljpetersen]

As a heavy user of SFML, I'd love to see something along these lines make it into SFML 3. As a general comment regarding the API of SFML, I find that it's too aggressive in hiding the internals to the detriment of its use. For example, for one of my projects I wanted to batch all text drawn to the screen and with the current design I couldn't figure out a good way to do so. I ended up creating a bitmap font system where I draw the text to a RenderTexture which I then use as a spritesheet to batch the text calls, but I'd much have preferred being able to access the vertices from sf::Text or giving the sf::Text a reference to a VertexArray for it to use or something. Along a similar line, in the past I was using https://github.com/Skyrpex/RichText but I stopped using it because it was causing noticeable FPS drops when used heavily. We should be able to improve on this by allowing a more flexible and open API.

So I suppose regarding eXpl0it3r's comments, if consistency in the API is a roadblock I think it's worth it to take similar steps throughout the API in order to allow this change to go through.

Regarding inheriting from vector, I agree that it'd be a bit weird and unexpected as a user of the API, but I think it'd be an improvement over the status quo regardless. I liked vittorioromeo's other suggestion to decouple the vector with VertexArrayRef, but I don't think it'd be worth the additional complexity in its use. It'd make the VertexArray closer in behavior to VertexBuffer, but like therocode says, I've never wanted to use another container type for this. I often opt to use a VertexArray over VertexBuffer just because it's slightly easier to use.

I very much agree with vittorioromeo's concern about this being more verbose (.vertices instead of just []). Honestly, I'd even be happy with just keeping everything as is now but just moving the vertices vector to a public member. It'd be redundant but you get rid of the verbosity and can access the raw vector if you have a need for it.

[ChrisThrasher]

I think inheriting from std::vector is a compelling option to keep code terse and perhaps worth implementing to see how it turns out but I'm leaning slightly in favor of composition in this case.

I'm not sold on the added complexity of a non-owning sf::VertexArrayRef type. Perhaps we explore that later but I think we can simplify the API in one PR and explore different ownership models later.

As for API consistency, SFML has its fair share of aggregate types like sf::VideoMode, sf::Vector, sf::ContextSettings, sf::Rect, and sf::BlendMode. The precedent is that where encapsulation is not needed, it is not used. We do not defensively hide all those data members behind setters and getters nor should we.

[ChrisThrasher]

https://github.com/ChrisThrasher/SFML/commit/1b103ef2826b63644dae03533e259c27e49a917f

Implementing this via inheritance actually makes for great calling code so here's what that looks like. I made sure to make use of the more expressive std::vector API to simplify things.

[therocode]

I'm leaning more towards the composition way (std::vector as a member instead of inherited) since inheriting from standard containers is commonly seen as bad practice, and I think this will put off a lot of people. Even in the core guidelines it explicity states:

Do not use inheritance when simply having a data member will do. Usually this means that the derived type needs to override a base virtual function or needs access to a protected member." https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rh-domain. Doesn't mean it's automatically bad, but a quick google will show that it's controversial.

It also seems to me like the tad more complex approach since it's multiple inheritance with not-as-clear semantics (like in the beginning of this thread when at least I was confused on if it was even going to call the destructors and vittorio added "...but I need to double-check that"). Nothing IMO is clearer than simple aggregate composition, and I don't buy the argument that the verbosity is a problem. Again, how often are users going to type that extra va.vertices? Way less often than say static_cast<> or other things that are "long" but provides clarity.

Also, if you have a piece of code that provides vertices from an algorithm, like std::vector<sf::Vertex> generate_v();, how would you make an sf::VertexArray out of this if it's inherited? Is there a way? With composition you'd simply do sf::VertexArray{vertices, PrimitiveType::Triangles} but with inheritance you can't exactly do that, and you can't also do va.vertices = vertices; or even va = vertices;. Seems like for this method to be usable, we would need a design that covers a set of constructors/assignment operators that is able to handle what would be expected, including move ones. This adds further complexity compared to simply an aggregate, which is ironic when the purpose of this proposal was to get away from having to reinvent the std::vector API.

[vittorioromeo]

My main problem with this suggestion is that if we make everything public in sf::VertexArray, there's little point for the type to exist. It's basically a pair of std::vector<sf::Vertex> and sf::PrimitiveType.

That doesn't need to be provided by SFML. If we end up going in this direction, I'd rather remove sf::VertexArray completely, and tell people to use

void sf::RenderTarget::draw(
 	const Vertex* 	 	vertices,
	std::size_t  	 	vertexCount,
	PrimitiveType  	 	type,
	const RenderStates &  	states = RenderStates::Default 
) 	

instead. getBounds and the more convenient draw interface could be provided as free functions or with a ref-wrapper as I mentioned above, which would give people more flexibility as well.

I don't really see much point in keeping sf::VertexArray if it doesn't provide any encapsulation or invariants. I'd rather remove it than make it a struct.

[therocode]

I think it's a bit of a false dichotomy to claim that a type either needs encapsulation/invariants or that it's useless. Aggregates are a thing and they are often useful as a simple means of bundling related data together. In this case, it'd still provide a purpose if it still inherits from Drawable f.ex. But yes, if the drawing functionality is extracted elsewhere, then as it currently stands, this type would not have much remaining use.

Also, regarding encapsulation/invariants with this type, there's almost none of it already as it stands. There's no data validation between the vector and the primitive type, and there is no effective protection against any states at all. The user have complete transparent access to set/get both the vector/primitive type in any way they want, except with hurdles since the API is limiting as discussed above. This is exactly why I feel this API is useless.

Maybe this type could still be useful, as an aggregate, as part of the other discussions around batching. I'm still thinking that it's useful for a library that deals with graphics to have a primitive building block that expresses vertex geometry, that is the vertex data + primitive type, and this type seems fitting for that purpose. But yeah, that involves the discussion on what kind of functionality would be exposed for the purpose of batching. Maybe a sf::GeometryBuilder that could be given drawables and then yields std::vector<sf::VertexArray> where the VertexArray would be as proposed, an aggregate of primitve type and vertices. This depends a lot on the vision for batching fundamentals.

[HerrNamenlos123]

My take on this topic is: I think the best option would be to make it a struct or remove it completely. Inheriting from std::vector is very strange and it really put me off, reading this discussion. I think hardly anyone would expect something like that. Inheriting from std::vector (even though it's strange and probably not recommended) might, if at all, be acceptable if the class is just a container that serves the pure purpose of storing an array - basically an std::vector but with more.

This class also contains a vertex type, which is not something you would find in a vector/array/container, so it wouldn't make any sense to combine it.

On the other hand, having no container at all is also a bit strange, I personally wouldn't feel comfortable working with it if I have to manually create a vector of vertices and then pass on a pointer. It feels very hacky and like if I'm doing something wrong.

Based on that the struct solution with two members is by far the best solution IMO, as it allows complete control, while not violating any standards or inheriting types that are not supposed to, and it is also nice for the user of the library because there is a predefined container, which means it feels right to use it.

[ChrisThrasher]

https://github.com/SFML/SFML/compare/master...ChrisThrasher:SFML:vertex_array

I reimplemented sf::VertexArray to have all public data and it has some nice upsides. We delete 200 lines of code and can now leverage std::vector features that were previously inaccessible. Ultimately wasn't as invasive of a change as I expected. I think this is a logical step forward from the current implementation of sf::VertexArray.

[ChrisThrasher]

In the Discord server Exploiter expressed how he wants to maintain the status quo where sf::VertexArray wraps the std::vector interface and keeps data private. That means I have to close this issue. He's still open to adding more member functions to sf::VertexArray to mirror more of std::vector's interface. I expect https://github.com/SFML/SFML/pull/2587 will still get merged.

[eXpl0it3r]

The goal sf::VertexArray currently fulfills is stringing together vertex data with a primitive type, while providing the sf::Drawable interface for ease of use.

Once you get deeper into the weeds of rendering vertex data, you may encounter, that sf::VertexArray doesn't provide enough manipulation tools for the vertex data. This then leads to two options:

1. Build your own time with direct access to the vertex data
2. Expand the SFML API a) Add more functions to the API b) Derive from std::vector c) Provide direct access (read/write) to the vertex data d) Converting to data struct type

1) Build your own

As we as a library can't possibly foresee all the ways one could/would want to use the vertex data, it's recommended to just create your own abstraction with std::vector<sf::Vertex> that gives you endless opportunities.

2a) Add more functions to the API

In my opinion, we shouldn't build "generic" container types or replicate the std::vector API. If someone is need of complex data access then option 1) exists already. But we might consider certain functions to be added. Here I think we'll need to define criteria what does and doesn't go into the API.

My personal base-line right now would be, that anything that's purely a technical/container function (e.g. reserve(), shrink_to_fit(), etc.) might not be a fit, while functions that also have a use-case with the "graphical" object VertexArray itself, might stand a chance.

2b) Derive from std::vector

To begin with, it's generally not recommended to derive from std::vector. It's not feasible for SFML anyways, as it would mix function naming schemes that are incompatible (camelCase vs snake_case). And as said in 2a) I don't think we should implement the full std::vector interface.

2c) Provide direct access (read/write) to the vertex data

If the goal is to expose the vertex data (read & write) via API, then we're breaking encapsulation, which is in my opinion a no-go. Providing access to internal data is only acceptable in a data-only type, which then would generally be implemented as struct.

2d) Converting to data struct type

Converting the class into a (data-only) struct type, would remove the sf::Drawable interface, which destroys the stipulated goal. It would also break the current "Drawable" design principle when looking at the types in sfml-graphics.
Besides that, writing a five line struct, really doesn't require expert knowledge and thus I'd recommend 1) in those cases.

Conclusion

I don't see us converting sf::VertexArray into a plain struct. If we'd approach that direction, I'd rather just remove the type as @vittorioromeo said.

Hope I covered everything, given that it's already late... 😄

---

He's still open to adding more member functions to sf::VertexArray to mirror more of std::vector's interface. I expect https://github.com/SFML/SFML/pull/2587 will still get merged.

Within limits, see 2a).
reserve() would by my current "base line" not make the cut.