strange vao error

Good day everybody, i have a strange error which pops up at times, i have a resource manager for models which load a mesh when its needed, the mesh contains a vao used for rendering.
Everything is running fine but sometimes i get the wrong mesh rendered, for example i have a robot made up of cylinders, boxes and piramids, from time to time, i get pyramids instead of boxes and viceversa
rendered with incorrect number of vertices. I think the problem is in the vao creation function , if i put the vbo and ibo as members of the class, the problem is permanent, if i move the vbo and ibo variables
inside the vao creation function the problem is mitigated, but pops up once in a while. I do not think its a problem of shared pointers,or memory management from my side, but i am running out of ideas.
Can someome help me to undersatnd why ? i think i create the vao correctly.

Are you unbinding? Specifically, if you unbind your GL_ELEMENT_ARRAY_BUFFER while your VAO is still bound, you can have problems. You shouldn’t be unbinding anyway, but in conjunction with VAOs it’s asking for trouble.

[QUOTE=vincent71;1291210]Good day everybody, i have a strange error which pops up at times, i have a resource manager for models which load a mesh when its needed, the mesh contains a vao used for rendering.
Everything is running fine but sometimes i get the wrong mesh rendered, for example i have a robot made up of cylinders, boxes and piramids, from time to time, i get pyramids instead of boxes and viceversa
rendered with incorrect number of vertices. I think the problem is in the vao creation function , if i put the vbo and ibo as members of the class, the problem is permanent, if i move the vbo and ibo variables
inside the vao creation function the problem is mitigated, but pops up once in a while. I do not think its a problem of shared pointers,or memory management from my side, but i am running out of ideas.
Can someome help me to undersatnd why ? i think i create the vao correctly.[/QUOTE]

It sounds to me like this is a classic case of bad cpu-side code.

Most likely you are reading in your files wrong or something

I’ve usually found that similar errors with inconsistent results stem from the loader class being instantiated with bad member variable values or my loader having a fatal flaw

I recommend you debug your program with a debugger like gdb

Yes i unbind after drawing using glelement and i see that this is a common way to render data using opengl, i unbind after creation of vbo also could this be the cause of the problem ?

Unbinding the VAO should be OK, although I generally would not recommend it (there are better alternatives; it may be a common pattern in tutorials but if you compare the old OpenGL wiki common mistakes pages with the old NeHe tutorials, you’ll learn quickly enough that even well-regarded tutorials can teach bad practices).

Unbinding the buffers after drawing will break your VAO. If that’s what you’re doing, you should stop it.

Usually i bind, draw, then unbind, is that a bad practice ?

Nevermind.

Followed adivces, the problem still persists, i do not think its a cpu-side problem , because even when i render a debug bounding box, the mesh gets rendered using the vao of another mesh. the problem is more prominent when i move the indexbufferobjets gluint variables ( vbo and ibo ) in the class definition, if i move them inside the vbocreation function, the problem pops up less frequently. There is another question i’d like to ask becasue i cannot get a precise answer, do i need to delete vbo and ibos after i delete a vao ?

You need to start posting some code.

VAOs and VBOs don’t have the kind of relationship you think they do.

A VAO contains state.
A VBO contains data.

A VAO does not contain VBOs; it just contains bindings.
The same VBO can be used by multiple VAOs.
You can switch VBO multiple times within the same VAO.

There is no tight connection between them and you shouldn’t program on the assumption that there is.

The best alternative to unbinding is to simply bind everything you need before doing any operation. If you make a draw call, bind everything you need before doing so. If you modify a buffer, bind it before doing so (and you can use VAO 0 for this). Make no state changes after drawing, and assume that on entry to any drawing routine current state is unknown so bind and set-up everything, then draw and get out.

So, unbinding stuff is bad, but rebinding everything is good? I’m not sure I follow your rationale. I can’t imagine how rebinding state will be faster than not rebinding state.

It should also be noted that many of your examples conflate “binding” with “attachment”. This is a common misconception, since the functions that attach buffers to VAOs tend to have the word “bind” in them. That is, the unbinding in the cases you present are wrong, not because unbinding is inherently wrong, but because those cases aren’t “binding” at all; they’re attachment. Which is not something you would undo in the same circumstances as binding.

The best alternative to unbinding is to know what you’re doing. Not to merely copy and paste bits of code around, but to structure your code in a reasonable way, based on what the intent of your program is trying to achieve. Rebinding everything creates safety only for those who don’t have the discipline to know what state their programs are in.

This is the model that modern APIs (Vulkan etc) use, and by emulating this model in GL code you’ll stand the best chance of your code working the way your graphics hardware would like it to work, and you’ll find that your code is also a whole bunch cleaner, simpler and more robust.

That is completely incorrect. Vulkan does not force you to rebind everything. It preserves state within a command buffer. Not just pipeline state, but descriptor set bindings, push constant values, and so forth.

For performance reasons, what you are expected to do is structure your code so that you’re not constantly rebinding the same pipelines, descriptors, and the like. You bind a pipeline for use, and then you use it for some period of time. If you want maximum performance, you don’t rebind the same pipeline over and over for each object. You gather together all objects that use the same pipeline and bind it once. Your layer your descriptor sets based on frequency, changing only certain ones on a per-object basis.

That is the model you’re expected to follow; the OpenGL equivalent is laid down by the AZDO presentation. And that presentation says nothing about resetting every piece of state whenever you enter some drawing routine.

Here is my vbo creation code


			void Mesh3d::CreateVBO()
				{
					if (Initted == false)
						vml::CMessage::Error(L"CMesh3d : Cannot create vbo, mesh %s is not initted",
											 vml::strings::convert::ToWString(ResourceFileName).c_str());

					// Create the vertex array object for the mesh.
					//
					// The buffer objects that are to be used by the vertex stage of the GL
					// are collected together to form a vertex array object. All state related
					// to the definition of data used by the vertex processor is encapsulated
					// in a vertex array object.
					//
					// Vertex array objects maintain state. Any required vertex and index
					// buffer objects are created and bound to the vertex array object. Then
					// any vertex shader generic shader input variables are mapped to the
					// vertex data stored in the vertex buffer objects bound to the vertex
					// array object. All of this only needs to be done once when using vertex
					// array objects.

					// Clear VAO

					if (VAOid) { glDeleteVertexArrays(1, &VAOid); VAOid = 0; }

					// Create the VAO

					// Map the generic vertex attributes used by the mesh's shader program to
					// the mesh's vertex data stored in the vertex buffer object.

					GLuint AttributePosition = vml::shaders::CShaderProgram::ATTRIBUTE_POSITION;
					GLuint AttributeNormal = vml::shaders::CShaderProgram::ATTRIBUTE_NORMAL;
					GLuint AttributeTexCoord = vml::shaders::CShaderProgram::ATTRIBUTE_TEXCOORD;
					GLuint AttributeColor = vml::shaders::CShaderProgram::ATTRIBUTE_COLOR;

					GLuint	VertexBufferObject; // vertex buffer object
					GLuint	IndexBufferObject;	// surface index buffer object

					glGenVertexArrays(1, &VAOid);
					glBindVertexArray(VAOid);

					// Create the Vertex Buffer Object 

					glGenBuffers(1, &VertexBufferObject);
					glBindBuffer(GL_ARRAY_BUFFER, VertexBufferObject);
					glBufferData(GL_ARRAY_BUFFER, Vertices * sizeof(Vertex3d), &VertexArray[0], GL_STATIC_DRAW);

					// Create the Index Buffer Object 

					glGenBuffers(1, &IndexBufferObject);
					glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, IndexBufferObject);
					glBufferData(GL_ELEMENT_ARRAY_BUFFER, Indices * sizeof(unsigned int), &SurfaceIndices[0], GL_STATIC_DRAW);
					
					// specify vertex attributes

					glEnableVertexAttribArray(AttributePosition);
					glEnableVertexAttribArray(AttributeNormal);
					glEnableVertexAttribArray(AttributeTexCoord);
					glEnableVertexAttribArray(AttributeColor);

					glVertexAttribPointer(AttributePosition, 4, GL_FLOAT, GL_FALSE, sizeof(Vertex3d), (GLubyte*)0 + 0 * sizeof(GLfloat));
					glVertexAttribPointer(AttributeNormal, 3, GL_FLOAT, GL_FALSE, sizeof(Vertex3d), (GLubyte*)0 + 4 * sizeof(GLfloat));
					glVertexAttribPointer(AttributeTexCoord, 2, GL_FLOAT, GL_FALSE, sizeof(Vertex3d), (GLubyte*)0 + 7 * sizeof(GLfloat));
					glVertexAttribPointer(AttributeColor, 4, GL_FLOAT, GL_FALSE, sizeof(Vertex3d), (GLubyte*)0 + 9 * sizeof(GLfloat));

					// unbinds buffers

					glBindVertexArray(0);
					glBindBuffer(GL_ARRAY_BUFFER, 0);
					glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, 0);

					// do i need to do this ???
					
					glDisableVertexAttribArray(AttributePosition);
					glDisableVertexAttribArray(AttributeNormal);
					glDisableVertexAttribArray(AttributeTexCoord);
					glDisableVertexAttribArray(AttributeColor);
					
					vml::bits32::SetToFalse(Flags, DO_NOT_CREATE_VBOS);
			
				}

and the rendering function


			void DrawMeshPointLight(vml::models::CModel3d *model, vml::views::CView *view)
			{
				if (view == nullptr)
					vml::CMessage::Error(L"Null matrix cam for mesh point light rendering");

				// get shader

				vml::shaders::CShaderProgram *shader = vml::ShaderManager.DebugPointLightShader.get();

				GLuint shaderProgram = shader->GetID();

				glUseProgram(shaderProgram);

				// Setup material parameters

				GLuint MaterialAmbientLocation = glGetUniformLocation(shaderProgram, "Material.ambient");
				GLuint MaterialDiffuseLocation = glGetUniformLocation(shaderProgram, "Material.diffuse");
				GLuint MaterialSpecularLocation = glGetUniformLocation(shaderProgram, "Material.specular");
				GLuint MaterialShininessLocation = glGetUniformLocation(shaderProgram, "Material.shininess");

				glUniform4fv(MaterialAmbientLocation, 1, &DebugMaterial->GetAmbient()[0]);
				glUniform4fv(MaterialDiffuseLocation, 1, &DebugMaterial->GetDiffuse()[0]);
				glUniform4fv(MaterialSpecularLocation, 1, &DebugMaterial->GetSpecular()[0]);
				glUniform1f(MaterialShininessLocation, DebugMaterial->GetShininess());

				// set light

				GLuint LightAmbientLocation = glGetUniformLocation(shaderProgram, "PointLight.ambient");
				GLuint LightDiffuseLocation = glGetUniformLocation(shaderProgram, "PointLight.diffuse");
				GLuint LightSpecularLocation = glGetUniformLocation(shaderProgram, "PointLight.specular");
				GLuint LightPosition = glGetUniformLocation(shaderProgram, "PointLight.position");
				GLuint LightPowerLocation = glGetUniformLocation(shaderProgram, "PointLight.power");
				GLuint LightCameraSpaceLocation = glGetUniformLocation(shaderProgram, "PointLight.cameraspaceposition");
				GLuint LightKq = glGetUniformLocation(shaderProgram, "PointLight.kq");

				PointLight.CameraSpacePosition = (view->GetView()*PointLight.Position);

				glUniform4fv(LightAmbientLocation, 1, &PointLight.Ambient[0]);
				glUniform4fv(LightDiffuseLocation, 1, &PointLight.Diffuse[0]);
				glUniform4fv(LightSpecularLocation, 1, &PointLight.Specular[0]);
				glUniform4fv(LightPosition, 1, &PointLight.Position[0]);
				glUniform4fv(LightCameraSpaceLocation, 1, &PointLight.CameraSpacePosition[0]);
				glUniform1f(LightPowerLocation, PointLight.Power);
				glUniform1f(LightKq, PointLight.Kq);

				// set shader locations

				glUniformMatrix4fv(shader->GetViewMatrixLocation(), 1, GL_FALSE, view->GetVptr());
				glUniformMatrix4fv(shader->GetProjectionMatrixLocation(), 1, GL_FALSE, view->GetPptr());
				glUniformMatrix4fv(shader->GetModelMatrixLocation(), 1, GL_FALSE, model->GetMptr());
				glUniformMatrix3fv(shader->GetNormalMatrixLocation(), 1, GL_FALSE, model->GetNptr());
				glUniformMatrix4fv(shader->GetModelViewMatrixLocation(), 1, GL_FALSE, model->GetMVptr());
				glUniformMatrix4fv(shader->GetModelViewProjectionMatrixLocation(), 1, GL_FALSE, model->GetMVPptr());

				// draw mesh
				
				const vml::meshes::Mesh3d *mesh = model->GetMesh();

				glPolygonMode(GL_FRONT_AND_BACK, GL_FILL);

				glBindVertexArray(mesh->GetVAOId());

				glDrawElements(
									GL_TRIANGLES,				// mode
									mesh->GetIndicesCount(),	// count
									GL_UNSIGNED_INT,			// type
									(void*)0					// element array buffer offset
								);

				glBindVertexArray(0);
				glBindBuffer(GL_ARRAY_BUFFER, 0);
				glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, 0);
				glUseProgram(0);
				
			}

do i need to do this ???

That is the wrong question to ask. It’s a question that can only be asked if you’re not sure what those functions are actually doing. In which case, the right question is:

What are those functions doing?

Because if you know what they’re doing, then you know whether you need to call them or not.

I think you’re stuck in a high-functioning “copy-and-paste” level of OpenGL coding, where you’re not really sure what functions are doing. You know that you have to call X before Y and Y before Z to get the effect you want. And you have some ideas of what X, Y and Z do, but you don’t really know what they’re doing.

So allow me to rewrite a bit of your code using DSA-style programming, which should make what they’re doing a bit more clear. When using DSA functions, functions that manipulate an object are given that object directly as a parameter.


glDisableVertexArrayAttrib(0, AttributePosition);
glDisableVertexArrayAttrib(0, AttributeNormal);
glDisableVertexArrayAttrib(0, AttributeTexCoord);
glDisableVertexArrayAttrib(0, AttributeColor);

These functions take the VAO which they act on as a parameter. Each of these functions is being given the VAO 0 to manipulate. Why? Because that’s the VAO which you previously bound.

Now you tell me: do you need to change the state of VAO 0 (which in the core profile is not a valid object)?

I suppose not

Hello again, turned out that the offending code wasn’t an opengl error both from my side ( even if riddled with incongruencies ) or from driver side, the problem was in my resource manager, now refactored avoding smart pointer, probably during some copy the vao index had been mangled in some way, this is strange becasue i expressly declared copy constructors private and applied the move paradigm. I need to investigate further on that. For the moment thanks to all who have pointed my mistakes in my opengl code that was usefull to fine tune it.