11 hours ago, CrazyCdn said:

Why are you hard coding your vertex/index buffers but allowing unlimited textures?  You're already using std::vectors, let's make the class even more general.

struct QuadData
{
	GLfloat verts[4] = { 0.0f };
	GLuint	indicies[4] = { 0 };
};

std::vector<QuadData*> quadData;

I don't think this is an improvement - indices in QuadData is redundant (all quads have the same indices), and storing pointers to QuadData in the vector is going to lead to fussy allocations and thrash the poor caches when copying vertex data.

Alright, I have followed your approach and created a struct for each Quad. If it's not performant with cache and stuff I can change it in the future, but I will want to make it work with this first because this code is as clear as it can get.

Right now, everything but rendering same textures.

This is the Quad struct:

#ifndef QUAD_H
#define QUAD_H
#include "Texture.h"
struct Quad {
	GLfloat vertices[100] = { 0.0f };
	GLuint indices[600] = { 0, 1, 3, 1, 2, 3 };
	Texture *texture;
	glm::mat4 transform;
};
#endif

Don't know how hight to set the array size. And this is my SpriteBatch Header:

#ifndef SPRITEBATCH_H
#define SPRITEBATCH_H

#include <glm/glm.hpp>
#include "Texture.h"
#include <GL/glew.h>
#include "Camera.h"
#include "Shader.h"
#include <vector>
#include "Quad.h"


class SpriteBatch
{
public:
	SpriteBatch(Shader& shader, Camera &camera);
	~SpriteBatch();

	void draw(Texture *texture, GLfloat x, GLfloat y, GLfloat width, GLfloat height);
	void flush();
private:
	GLuint VBO = 0, VAO = 0, EBO = 0;
	GLint transformShaderLocation, viewShaderLocation, projectionShaderLocation;
	Shader *shader;
	Camera *camera;
	std::vector<std::vector<Quad>> batchedQuads;
	std::vector<Quad> combinedQuads;
	std::vector<int> batchedQuadsContentQuadAmount;
	glm::mat4 projection, view;
	int currentIndex{ 0 };
};
#endif

And class:

#include "SpriteBatch.h"



SpriteBatch::SpriteBatch(Shader& shader, Camera &camera)
{
	this->shader = &shader;
	this->camera = &camera;

	glGenVertexArrays(1, &VAO);
	glGenBuffers(1, &VBO);
	glGenBuffers(1, &EBO);
}


SpriteBatch::~SpriteBatch()
{
	glDeleteBuffers(1, &VBO);
	glDeleteBuffers(1, &EBO);
	glDeleteBuffers(1, &VAO);
}

void SpriteBatch::draw(Texture *texture, GLfloat x, GLfloat y, GLfloat width, GLfloat height)
{
	Quad quad;
	
	quad.texture = texture;

	quad.transform = glm::translate(quad.transform, glm::vec3(x, y, 0));

	quad.vertices[0] = width/2 ;
	quad.vertices[1] = height/2;
	quad.vertices[2] = 0.0f;
	quad.vertices[3] = 1.0f;
	quad.vertices[4] = 1.0f;
	
	quad.vertices[5] = width / 2;
	quad.vertices[6] = -height / 2;
	quad.vertices[7] = 0.0f;
	quad.vertices[8] = 1.0f;
	quad.vertices[9] = 0.0f;
	
	quad.vertices[10] = -width / 2;
	quad.vertices[11] = -height / 2;
	quad.vertices[12] = 0.0f;
	quad.vertices[13] = 0.0f;
	quad.vertices[14] = 0.0f;
	
	quad.vertices[15] = -width / 2;
	quad.vertices[16] = height / 2;
	quad.vertices[17] = 0.0f;
	quad.vertices[18] = 0.0f;
	quad.vertices[19] = 1.0f;

	//Sort this quad into either a new list of quads which should be rendered or adds it to the last list if the texture is the same, so that they can be rendered together
	if (currentIndex != 0) {
		if (batchedQuads.at(currentIndex - 1).at(0).texture == texture) {
			batchedQuads.at(currentIndex - 1).push_back(quad);
		}
		else {
			batchedQuadsContentQuadAmount.push_back(batchedQuads.at(currentIndex - 1).size());
			std::vector<Quad> anotherQuadVector;
			anotherQuadVector.push_back(quad);
			batchedQuads.push_back(anotherQuadVector);
			currentIndex += 1;
		}
	}
	else {
		std::vector<Quad> firstQuadVector;
		firstQuadVector.push_back(quad);
		batchedQuads.push_back(firstQuadVector);
		currentIndex += 1;
	}

}

void SpriteBatch::flush()
{
	if (currentIndex == 0) return; //Ensures that there are sprites added

	for (std::vector<Quad> quadBatch : batchedQuads) {
		Quad combinedQuad;
		int processedQuads{ 0 };
	
		for (Quad quad : quadBatch) {
			
			combinedQuad.vertices[processedQuads * 20]     = quad.vertices[0];
			combinedQuad.vertices[processedQuads * 20 + 1] = quad.vertices[1];
			combinedQuad.vertices[processedQuads * 20 + 2] = quad.vertices[2];
			combinedQuad.vertices[processedQuads * 20 + 3] = quad.vertices[3];
			combinedQuad.vertices[processedQuads * 20 + 4] = quad.vertices[4];

			combinedQuad.vertices[processedQuads * 20 + 5] = quad.vertices[5];
			combinedQuad.vertices[processedQuads * 20 + 6] = quad.vertices[6];
			combinedQuad.vertices[processedQuads * 20 + 7] = quad.vertices[7];
			combinedQuad.vertices[processedQuads * 20 + 8] = quad.vertices[8];
			combinedQuad.vertices[processedQuads * 20 + 9] = quad.vertices[9];

			combinedQuad.vertices[processedQuads * 20 + 10] = quad.vertices[10];
			combinedQuad.vertices[processedQuads * 20 + 11] = quad.vertices[11];
			combinedQuad.vertices[processedQuads * 20 + 12] = quad.vertices[12];
			combinedQuad.vertices[processedQuads * 20 + 13] = quad.vertices[13];
			combinedQuad.vertices[processedQuads * 20 + 14] = quad.vertices[14];

			combinedQuad.vertices[processedQuads * 20 + 15] = quad.vertices[15];
			combinedQuad.vertices[processedQuads * 20 + 16] = quad.vertices[16];
			combinedQuad.vertices[processedQuads * 20 + 17] = quad.vertices[17];
			combinedQuad.vertices[processedQuads * 20 + 18] = quad.vertices[18];
			combinedQuad.vertices[processedQuads * 20 + 19] = quad.vertices[19];


			combinedQuad.indices[processedQuads * 6]     = 0 + processedQuads * 4;
			combinedQuad.indices[processedQuads * 6 + 1] = 1 + processedQuads * 4;
			combinedQuad.indices[processedQuads * 6 + 2] = 3 + processedQuads * 4;
			combinedQuad.indices[processedQuads * 6 + 3] = 1 + processedQuads * 4;
			combinedQuad.indices[processedQuads * 6 + 4] = 2 + processedQuads * 4;
			combinedQuad.indices[processedQuads * 6 + 5] = 3 + processedQuads * 4;

			processedQuads += 1;
		}
		combinedQuad.texture = quadBatch.at(0).texture;
		combinedQuad.transform = quadBatch.at(0).transform;
		combinedQuads.push_back(combinedQuad);
	}
	batchedQuadsContentQuadAmount.push_back(batchedQuads.at(batchedQuads.size()-1).size());
	batchedQuads.clear();


	int drawCalls{ 0 };

	for (Quad combinedQuad : combinedQuads) {
		shader->Use();
		glBindTexture(GL_TEXTURE_2D, combinedQuad.texture->texture);

		view = camera->getView();
		projection = camera->getProjection();


		glBindVertexArray(VAO);

		//Bind vertices
		glBindBuffer(GL_ARRAY_BUFFER, VBO);
		glBufferData(GL_ARRAY_BUFFER, sizeof(combinedQuad.vertices), combinedQuad.vertices, GL_STATIC_DRAW);

		//Bind indices
		glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, EBO);
		glBufferData(GL_ELEMENT_ARRAY_BUFFER, sizeof(combinedQuad.indices), combinedQuad.indices, GL_STATIC_DRAW);

		//Position
		glVertexAttribPointer(0, 3 , GL_FLOAT, GL_FALSE, 5 * sizeof(GLfloat), (GLvoid*)0);
		glEnableVertexAttribArray(0);
		// TexCoord
		glVertexAttribPointer(1, 2 , GL_FLOAT, GL_FALSE, 5 * sizeof(GLfloat), (GLvoid*)(3 * sizeof(GLfloat)));
		glEnableVertexAttribArray(1);
		//VAO
		glBindBuffer(GL_ARRAY_BUFFER, 0);
		glBindVertexArray(0);
		//Shader locations
		transformShaderLocation = glGetUniformLocation(shader->program, "transform");
		viewShaderLocation = glGetUniformLocation(shader->program, "view");
		projectionShaderLocation = glGetUniformLocation(shader->program, "projection");


	

		// Pass them to the shaders
		glUniformMatrix4fv(transformShaderLocation, 1, GL_FALSE, glm::value_ptr(combinedQuad.transform));
		glUniformMatrix4fv(viewShaderLocation, 1, GL_FALSE, glm::value_ptr(view));
		glUniformMatrix4fv(projectionShaderLocation, 1, GL_FALSE, glm::value_ptr(projection));



		//Draw VAO
		glBindVertexArray(VAO);
		glDrawElements(GL_TRIANGLES, 6 * batchedQuadsContentQuadAmount.at(drawCalls), GL_UNSIGNED_INT, 0);
		glBindVertexArray(0);
		drawCalls += 1;
	}
	
	//Sets index to 0 and clears combinedQuads to welcome new sprites. Batchedquads has already been cleared.
	currentIndex = 0;
	combinedQuads.clear();
}

For me, all is logical and does its job as intended, but same textures still don't work.

Here is the main class:

#include "main.h"
#define STB_IMAGE_IMPLEMENTATION
#include <stb_image.h>


int main(int argc, char* argv[]) {
	Main main;
	return main.init();
}

int Main::init() {

	display = new Display(800, 600, "OpenGL" );

	shader = new Shader("Shaders/default.vert", "Shaders/default.frag");

	texture1 = new Texture("Textures/libGDX.png", STBI_rgb_alpha);
	texture2 = new Texture("Textures/textuuur.png", STBI_rgb);
	_sprite1 = new Sprite(150.f, 150.f, 100.0f, 100.0f, *shader, texture1);
	_sprite2 = new Sprite(150.f, 150.f, 100.0f, 100.0f, *shader, texture2);

	if (_test3D) {
		camera = new Camera(PERSPECTIVE, display);
	}
	else {
		camera = new Camera(ORTHOGRAPHIC, display);
	}
	
	if (_test3D) {
		glEnable(GL_DEPTH_TEST);
	}

	this->spriteBatch = new SpriteBatch(*shader, *camera);


	while (!display->isClosed()) {
		update();
	}
	delete _sprite1, _sprite2;
	delete texture1, texture2;
	delete shader;
	delete camera;
	delete display;
	delete spriteBatch;
	return 0;
}

void Main::update() {
	draw();
	display->Update();
}

void Main::draw() {
	glClearColor(0.0f, 0.3f, 0.5f, 1.0f);
	if (_test3D) {
		glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT);
	}
	else {
		glClear(GL_COLOR_BUFFER_BIT);
	}

	//_sprite1->draw(*camera);
    //_sprite2->draw(*camera);

	spriteBatch->draw(texture1, _sprite1->x, _sprite1->y, _sprite1->width, _sprite1->height);
	spriteBatch->draw(texture2, _sprite1->x + 600, _sprite1->y + 10, _sprite1->width, _sprite1->height);
	spriteBatch->draw(texture2, _sprite1->x + 400, _sprite1->y + 10, _sprite1->width, _sprite1->height);

	spriteBatch->flush();
}

The first texture draws, but the second or third doesn't. Only one of the two same textures actually is drawn on the screen, the other is not.

Now, this 

Quote

thrash the poor caches when copying vertex data.

sounds pretty bad, how do I improve that. Do I have to write that without the Quad struct again?

Also, strangely with this:

        Quad combinedQuad;
        int processedQuads{ 0 };
        combinedQuad.texture = quadBatch.at(0).texture;
        combinedQuad.transform = quadBatch.at(0).transform;
        for (Quad quad : quadBatch) {
            
            combinedQuad.vertices[processedQuads * 20]     = quad.vertices[0];
            combinedQuad.vertices[processedQuads * 20 + 1] = quad.vertices[1];
            combinedQuad.vertices[processedQuads * 20 + 2] = quad.vertices[2];
            combinedQuad.vertices[processedQuads * 20 + 3] = quad.vertices[3];
            combinedQuad.vertices[processedQuads * 20 + 4] = quad.vertices[4];

            combinedQuad.vertices[processedQuads * 20 + 5] = quad.vertices[5];
            combinedQuad.vertices[processedQuads * 20 + 6] = quad.vertices[6];
            combinedQuad.vertices[processedQuads * 20 + 7] = quad.vertices[7];
            combinedQuad.vertices[processedQuads * 20 + 8] = quad.vertices[8];
            combinedQuad.vertices[processedQuads * 20 + 9] = quad.vertices[9];

            combinedQuad.vertices[processedQuads * 20 + 10] = quad.vertices[10];
            combinedQuad.vertices[processedQuads * 20 + 11] = quad.vertices[11];
            combinedQuad.vertices[processedQuads * 20 + 12] = quad.vertices[12];
            combinedQuad.vertices[processedQuads * 20 + 13] = quad.vertices[13];
            combinedQuad.vertices[processedQuads * 20 + 14] = quad.vertices[14];

            combinedQuad.vertices[processedQuads * 20 + 15] = quad.vertices[15];
            combinedQuad.vertices[processedQuads * 20 + 16] = quad.vertices[16];
            combinedQuad.vertices[processedQuads * 20 + 17] = quad.vertices[17];
            combinedQuad.vertices[processedQuads * 20 + 18] = quad.vertices[18];
            combinedQuad.vertices[processedQuads * 20 + 19] = quad.vertices[19];


            combinedQuad.indices[processedQuads * 6]     = 0 + processedQuads * 4;
            combinedQuad.indices[processedQuads * 6 + 1] = 1 + processedQuads * 4;
            combinedQuad.indices[processedQuads * 6 + 2] = 3 + processedQuads * 4;
            combinedQuad.indices[processedQuads * 6 + 3] = 1 + processedQuads * 4;
            combinedQuad.indices[processedQuads * 6 + 4] = 2 + processedQuads * 4;
            combinedQuad.indices[processedQuads * 6 + 5] = 3 + processedQuads * 4;

            processedQuads += 1;
        }

        combinedQuads.push_back(combinedQuad);

combinedQuad.texture will turn into a read access violation at combinedQuads.push_back(combinedQuad);

If I put 

		combinedQuad.texture = quadBatch.at(0).texture;
		combinedQuad.transform = quadBatch.at(0).transform;

after the loop / before the push_back, it will be fine.

Why?

Sorry for the billion edits.

I still see a lot of errors, both technical and conceptual, in the code. I'm not sure if the code included in your last post is still up to date, but regardless, I think if the code is working now, it's probably only working incidentally and is likely to fail under different circumstances. There's also a number of efficiency issues that could easily offset any performance gains related to batching.

Given some of the issues that still seem to be at play here, I don't think you should worry about things like cache behavior at this point. There are other more important things that need attention. (Regarding your question about the Quad struct though, it's certainly not needed for a basic batching system, and is probably just getting in the way at this point.)

As I mentioned earlier, it might be beneficial to set batching aside and instead focus on getting the basics working, with an emphasis on code quality and technical and conceptual correctness. If that didn't turn out to be performant enough (which may be the case), you could revisit the optimization issue.

As for your current code, I think a thorough analysis would make for too long a post, so I'm just going to pick a few things to point out.

- It looks like you're deleting a VAO with glDeleteBuffers() rather than glDeleteVertexArrays().

- If you're not already, you should probably check for OpenGL errors at least once per frame (and more often if needed), or use a developer tool that will alert you to them.

- Here:

delete _sprite1, _sprite2;

You're only deleting one of the sprites, not both (the comma operator in C++ is a little unintuitive, and may be best avoided unless you have a particular need for it). This should just be two separate delete statements.

- It looks like you're potentially writing past the ends of the arrays in 'combinedQuad' (if it's working now, it's probably because you only have one quad per batch). There seems to be a conceptual error here in that a Quad instance is only intended to store data for a single quad, but you're attempting to store data for multiple quads in a single such instance.

- Similarly, each batch only stores the transform for a single quad, so I think you'll find that batches with multiple quads, where each has a different transform, won't work.

I hope this is at least somewhat helpful

Thanks for the tips.

52 minutes ago, Zakwayda said:

It looks like you're potentially writing past the ends of the arrays in 'combinedQuad'

Yeah my array was too small, I increased it.

52 minutes ago, Zakwayda said:

There seems to be a conceptual error here in that a Quad instance is only intended to store data for a single quad, but you're attempting to store data for multiple quads in a single such instance.

Yeah it was intended to do that but I have to store the combined quads somewhere too

If I store it somewhere else I will end up doing vectors glfloat arrays again which doesn't work.

52 minutes ago, Zakwayda said:

Similarly, each batch only stores the transform for a single quad, so I think you'll find that batches with multiple quads, where each has a different transform, won't work.

Umm what's the purpose of a spritebatch if every sprite is the same and at the same position? I mean I have to change the change the position somehow or else the sprite will just overlap themselves.

I just wanted to have the batching done now as I'm very used to using a batch in libgdx. I just thought it would be good to make provisions against bad performance.

Sorry to keep pointing out issues But, better to deal with them now and build a good foundation.

Quote

Yeah my array was too small, I increased it.

That's probably not the right solution. Any arbitrary size could be theoretically exceeded if enough quads are batched together, and then you're back in the same situation. To avoid that, batches should be split when the capacity of the underlying containers is reached. Also, it doesn't make semantic sense for a 'quad' struct to hold data for multiple quads. And, unless your code has changed, you're now paying the cost of those large arrays for each quad that's stored in a batch.

In short, the storage for individual quad data (if any) and for combined quad data should be separate.

Quote

If I store it somewhere else I will end up doing vectors glfloat arrays again which doesn't work.

If it didn't work, it was just an implementation issue, not something inherent about arrays or std::vector or what have you. Implemented correctly, an approach that accumulates the data in one or more containers (e.g. std::vector) will work fine.

Quote

Umm what's the purpose of a spritebatch if every sprite is the same and at the same position? I mean I have to change the change the position somehow or else the sprite will just overlap themselves.

This is what I was referring to earlier about pre-transforming the vertex positions. The idea is that instead of submitting a model matrix to the shader, you pre-transform the vertex positions yourself in your own code. This is one way to address the problem you mention.

Quote

I just wanted to have the batching done now as I'm very used to using a batch in libgdx. I just thought it would be good to make provisions against bad performance.

Much of this is just my opinion, so make of it what you will. But I'll be forthright here.

I think worrying about performance and batching is a mistake at this point. Maybe you're used to batching from LibGDX, but now that you're implementing things yourself in C++, the context in which you're working has changed entirely.

Regarding performance, there are enough performance issues in your own code (unnecessary copying, redundant arithmetic operations, memory churn, unnecessary processing of quad indices, etc.) that evaluating the performance of your app at this point is probably pointless anyway. Even when implemented correctly batching isn't a panacea, and you may find that your app calls for a different approach entirely. (For example, if it's primarily tile-based, you may want to use static meshes for the environment, and if the number of other entities is small enough you may not need batching at all.)

Lastly, I think getting a better grasp on the basics of C++ will help smooth the way for further development. At minimum, I think you should look at the following topics: value vs. reference semantics, pointers and references and the differences between them, the 'const' keyword and its proper use, basic memory management, object lifetime, and the idiosyncrasies of raw arrays (and why you generally shouldn't use them).

19 hours ago, CrazyCdn said:

What is the point of "batchedQuadsContentQuadAmount"?  Other then to be as long a name as possible? 

It stores the amount of quads which are in one quad struct. I just realized that the amount of vertices %20 would be enough to find it t so I removed it.

19 hours ago, CrazyCdn said:

Why do you have a vector of a vector of quads then a vector of quads? 

I guess the other vector of quads is kind of unnecessary. I will best make a struct for multiple quads.

19 hours ago, CrazyCdn said:

But I think you need to be honest with yourself, I think you need to work on a few smaller projects first.  I'm not sure what your level of programming experience in C/C++ is but you seem fairly new to it.  I would recommend maybe making pong, then breakout type games first.  I forgot you had mentioned you tried to use the godot engine but claimed it had poor performance.  The engine is perfectly fine, it was likely your code to be honest as there are literally hundreds of 2D games made with that engine: https://godotengine.org/showcase and I doubt your engine will be faster then it is anytime soon.

I have worked on bigger 2d games in java and am already experienced it in, but I am fairly new to C++, that's true. Since i wanted to improve my c++  skills I have been looking for a 2d framework close to opengl, so where you draw yourself and have a render loop... But no framework really statisfied me, and when you look at this: http://nivrigdev.tumblr.com/post/137158222896/game-engine-benchmarking you see how badly optimized c++ frameworks are  (I also want to target mobile platforms in the future)

In godot I had a tilemap with like 100 sprites created in the editor and I just coded a camera which moves, and it already started lagging. Quickly did the same thing very dirty in libgdx and it ran much smoother without any optimization. But well godot isn't a framework anyways.

Maybe OpenGL is a bit too hard for me right now.

On 9/5/2018 at 11:30 AM, Zakwayda said:

At minimum, I think you should look at the following topics: ... the idiosyncrasies of raw arrays (and why you generally shouldn't use them).

I think this is pretty bad advice. Sure the rest of the things you listed need to be learned, but come on, don't use arrays? That just sounds like C++ dogma. Besides this bit here, I did like the majority of the rest of the advice in this thread.

Just try to keep opinions on the down-low or at least say they are your opinion clearly. For example: in my personal opinion I think any new C/C++ guy should fully embrace pointers and arrays as a basic necessity. There's a lot of reasons why you have your opinion, and I have mine, and they are probably all off-topic, so just do the thread a favor and distinguish between personal taste and actual errors in the code.

1 hour ago, Randy Gaul said:

I think this is pretty bad advice. Sure the rest of the things you listed need to be learned, but come on, don't use arrays? That just sounds like C++ dogma. Besides this bit here, I did like the majority of the rest of the advice in this thread.

Just try to keep opinions on the down-low or at least say they are your opinion clearly. For example: in my personal opinion I think any new C/C++ guy should fully embrace pointers and arrays as a basic necessity. There's a lot of reasons why you have your opinion, and I have mine, and they are probably all off-topic, so just do the thread a favor and distinguish between personal taste and actual errors in the code.

I think a full response to this would be off topic for the thread, so I'll just say a couple things and leave it at that. First, I think "don't use arrays" is a mischaracterization (I didn't say that). Second, I made it clear I was offering personal opinion in the very post you quoted, so I'm not sure what the rest of your post is about.