API defect in Buffer types

[csp256]

VertexBuffer::Create() expects its second argument to be in bytes, while IndexBuffer::Create() expects its second argument to be in element count. Seems like a pretty clear "gotcha" to me. I want to have to remember as few things as possible when I am calling a function.

To streamline things I'd like to see functions similar to these:

template <typename Container>
static VertexBuffer* 
Create(Container& v) 
{ 
	return Create(
		(float*) v.data(), 
		(uint32_t) v.size() * sizeof(Container::value_type)); 
}

and

template <typename Container>
static IndexBuffer* 
Create(Container& v) 
{ 
	return Create(
		(uint32_t*) v.data(), 
		(uint32_t) v.size()); 
}

Which allow me to replace this:

float squareVertices[5 * 4] = {
			-0.5f, -0.5f, 0.0f, 0.0f, 0.0f,
			 0.5f, -0.5f, 0.0f, 1.0f, 0.0f,
			 0.5f,  0.5f, 0.0f, 1.0f, 1.0f,
			-0.5f,  0.5f, 0.0f, 0.0f, 1.0f};

Hazel::VertexBuffer::Create(squareVertices, sizeof(squareVertices));

With the more clear and clean:

std::vector<std::array<float, 5>> squareVertices = {
			-0.5f, -0.5f, 0.0f, 0.0f, 0.0f,
			 0.5f, -0.5f, 0.0f, 1.0f, 0.0f,
			 0.5f,  0.5f, 0.0f, 1.0f, 1.0f,
			-0.5f,  0.5f, 0.0f, 0.0f, 1.0f};

Hazel::VertexBuffer::Create(squareVertices);

I understand that template over-use is a reasonable concern in this type of project, but even without templates I see no reason to not directly support std::vector<float> and std::vector<uint32_t> at the moment.

If this has any interest I'll gladly create a PR.

[CleverSource]

You should make a PR for this. But it should be optional to do this, possibly having two create functions, one taking in your change, and the current one. Forcing people to use std::vector is not a viable option but this definitely feels nicer.

[Jack-Punter]

This may be to do with the fact that the index buffer will always be the same type, whereas it may be conceivable to want to have a vertex array of double's rather than floats? although in this case having a count may actually be easier anyway. Not sure, just a thought.

[LovelySanta]

Another way I was thinking about is to just create an array as we do now, and overload the Create operators (so we have int*, float*, double*, ...). This way we can pass in the count. However, this limits us to predefined types.

[csp256]

@Jack-Punter The type could in principle be something else, like a uint16_t, but my point is broader than that: inconsistency and requiring the user to be detailed-oriented is a recipe for nothing good.

@LovelySanta Explicitly enumerating the types Create can take does avoid using templates, but only in a pyrrhic way.

It also doesn't make much sense from the vertex buffer point of view, where you may (probably) want to have an AOS and say "now put this on the GPU" with a minimum of boilerplate. I understand there are layout concerns with this, but it has been my experience that in practice it works really well. Furthermore, making the typical case easy and safe doesn't prevent you from having some (perhaps less elegant) way of handling the tricky edge cases.

It seems like an antipattern / code-smell to put the onus on the caller to explicitly provide vec.data(), sizeof(decltype(vec)::value_type), and vec.size() in the function call, instead of just providing the container and encapsulating the implementation details. I understand the underlying graphics API requires you to do things that way, but I think it would be a mistake for the engine wrapping it to take the same approach.

[LovelySanta]

I like to keep a raw barebone call in there, we can always overload the operator (for example a vector of arrays) and let the that function translate it into the barebone call... There are for sure ways to avoid/improve, but take in mind that we're still just setting everything up and getting it working... I think once we go deeper into the 2D renderer we'll come across some uses of them maybe? Who knows...