VERY weird error. Structs in std::vector being updated more than once

Started by
7 comments, last by _the_phantom_ 9 years, 4 months ago

Hello, I am writing a particle system, and I am trying to load particle objects into a vector.

Everytime I create a particle, it starts with a lifetime of 50.0f. The particle also starts at 0,0,0.

Every frame the particle increases the y value by 0.01f.

Also, all the particle are iterated through, and if the lifetime is <= 0.0f, then I swap and pop it from the list.

At first it seems like it should work without any problems at all, but...

for some strange reason, the y values seem to be doubled practicly as soon as the 50th particle is created and the particles are updated. It's like every particle that gets created asap, gets pushed above the 0,0,0 point, and new ones get pushed even farther. It's soooo weird D:!

Here is the code for the particles:


	while(!glfwWindowShouldClose(window))
	{
		particle particle1;
		particle1.alive = true;
		particle1.lifeTime = 50.0f;
		particle1.x = 0.0f;
		particle1.y = 0.0f;
		particle1.z = 0.0f;

		particleList.push_back(particle1);
		MessageBox(0, "m", "m", MB_OK);

		unsigned int currentSize = particleList.size();

		for(unsigned int iterator = 0;
			iterator < currentSize;
			iterator++)
		{
			particleList[iterator].lifeTime -= 1.0f;
			if(particleList[iterator].lifeTime <= 0.0f)
			{
				std::swap(particleList[iterator], particleList[particleList.size() - 1]);
				particleList.pop_back();
				iterator--;
			}
			else
			{
			}

			particleList[iterator].y += 0.01f;

			currentSize = particleList.size();
		}

I don't know why this is happening tongue.png. It doesn't make any sense at all and I can't think of anything.

Here is the rendering code, although this is more of a general programming question I believe, but just in case:


		glClearColor(0.1f, 0.2f, 0.3f, 1.0f);
		glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT);

		glm::mat4 streamedPositions[1000];

		glm::vec4 streamedColors[1000];

		//MessageBox(0, "l", "l", MB_OK);

		for(unsigned int pIndex = 0; pIndex < particleList.size(); pIndex++)
		{
			translationMatrix = glm::translate(glm::mat4(1.0f),
			glm::vec3(particleList[pIndex].x, particleList[pIndex].y, -10.0f));

			worldMatrix = translationMatrix;

			streamedColors[pIndex] = glm::vec4(1.0f, 1.0f, (float)particleList[pIndex].lifeTime / 50.0f, 1.0f);

			streamedPositions[pIndex] = worldMatrix;
		}

		glBindBuffer(GL_ARRAY_BUFFER, streamedColorBuffer);
		glBufferData(GL_ARRAY_BUFFER, sizeof(float) * 4 * 1000, streamedColors, GL_STREAM_DRAW);
		glBufferSubData(GL_ARRAY_BUFFER, 0, sizeof(float) * 4 * particleList.size() , streamedColors);

		glVertexAttribPointer(2, 4, GL_FLOAT, GL_FALSE, 0, (void*)0);

		glBindBuffer(GL_ARRAY_BUFFER, streamedPositionBuffer);
		glBufferData(GL_ARRAY_BUFFER, sizeof(float) * 16 * 1000, (GLfloat*)&streamedPositions, GL_STREAM_DRAW);
		glBufferSubData(GL_ARRAY_BUFFER, 0, sizeof(float) * 16 * particleList.size(), (GLfloat*)&streamedPositions);

		for(unsigned int i = 0; i < 4; i++)
		{
			glVertexAttribPointer(3+i, 4, GL_FLOAT, GL_FALSE, sizeof(float)*16.0f, (void*)0 + ((sizeof(float) * 4 * i)));
		}

		glUseProgram(testProgram);

		location = glGetUniformLocation(testProgram, "worldMatrix");
		glUniformMatrix4fv(location, 1, GL_FALSE, (GLfloat*)&worldMatrix);

		location = glGetUniformLocation(testProgram, "viewMatrix");
		glUniformMatrix4fv(location, 1, GL_FALSE, (GLfloat*)&viewMatrix);

		location = glGetUniformLocation(testProgram, "projectionMatrix");
		glUniformMatrix4fv(location, 1, GL_FALSE, (GLfloat*)&projectionMatrix);

		location = glGetUniformLocation(testProgram, "diffuse");
		glUniform1i(location, 0);

		glVertexAttribDivisor(0, 0);
		glVertexAttribDivisor(1, 0);
		glVertexAttribDivisor(2, 1);
		glVertexAttribDivisor(3, 1);
		glVertexAttribDivisor(4, 1);
		glVertexAttribDivisor(5, 1);
		glVertexAttribDivisor(6, 1);

		glDrawArraysInstanced(GL_TRIANGLE_STRIP, 0, 4, particleList.size());

		glfwSwapBuffers(window);
		glfwPollEvents();

The rendering code works fine as far as I can tell, and I am using instanced rendering.

View my game dev blog here!

Advertisement

        for(unsigned int iterator = 0;
            iterator < currentSize;
            iterator++)
        {
            particleList[iterator].lifeTime -= 1.0f;
            if(particleList[iterator].lifeTime <= 0.0f)
            {
                std::swap(particleList[iterator], particleList[particleList.size() - 1]);
                particleList.pop_back();
                iterator--;
            }
            else
            {
            }

            particleList[iterator].y += 0.01f;

            currentSize = particleList.size();
        }

You should consider using your debugger and stepping through the code. I think I have a handle on what might be going on though. First, let's say you have particleList[0].lifeTime == 1.0f and then you enter this loop. iterator is 0 so particleList[0].lifeTime will decrement to lifeTime 0.0f so you swap and pop the element then decrement interator to -1. Then you access particleList[-1].y which doesn't exist so who knows what your program will do if it tries to access that memory.

Now, I also think this bug is causing your issues with the weird multiple increases. If particleList[0].lifeTime == 2.0f and particleList[1].lifeTime == 1.0f then at the start of the loop when iterator == 0, particleList[0] will remain in the list and particleList[0].y will increment by 0.01f. iterator will increment to 1. So next particleList[1] will be swapped and deleted because its lifetime reaches 0. When this happens, iterator is set to 0 and particleList[0].y will increment by 0.01f again for 0.02f total.

So, exercise for the reader: solve this bug!

C++: A Dialog | C++0x Features: Part1 (lambdas, auto, static_assert) , Part 2 (rvalue references) , Part 3 (decltype) | Write Games | Fix Your Timestep!

Ok, well I managed to fix it by using


        for(unsigned int iterator = 0;
            iterator < currentSize;
            iterator++)
        {
            particleList[iterator].lifeTime -= 1.0f;
            if(particleList[iterator].lifeTime <= 0.0f)
            {
                std::swap(particleList[iterator], particleList[particleList.size() - 1]);
                particleList.pop_back();
                iterator--;
            }
            else
            {
            }

            particleList[iterator].y += 0.01f;

            currentSize = particleList.size();
        }

You should consider using your debugger and stepping through the code. I think I have a handle on what might be going on though. First, let's say you have particleList[0].lifeTime == 1.0f and then you enter this loop. iterator is 0 so particleList[0].lifeTime will decrement to lifeTime 0.0f so you swap and pop the element then decrement interator to -1. Then you access particleList[-1].y which doesn't exist so who knows what your program will do if it tries to access that memory.

Now, I also think this bug is causing your issues with the weird multiple increases. If particleList[0].lifeTime == 2.0f and particleList[1].lifeTime == 1.0f then at the start of the loop when iterator == 0, particleList[0] will remain in the list and particleList[0].y will increment by 0.01f. iterator will increment to 1. So next particleList[1] will be swapped and deleted because its lifetime reaches 0. When this happens, iterator is set to 0 and particleList[0].y will increment by 0.01f again for 0.02f total.

So, exercise for the reader: solve this bug!

Ah, I thought that it would increment as soon as it re-entered the for loop. I guess I dun goofed. I think I have to use ++iterator instead of iterator++. Either way I solved it by using particleList.erase(). I will try the ++ method thoush. Update: it didn't work :p.

View my game dev blog here!

++iterator won't fix it. The problem is you use iterator in the same loop iteration in which you decremented iterator. It causes you to go back to the previous iteration value too early. By the time the execution reaches the top of the loop the error has already occurred. Using erase would fix the issue, but erase might be slow if you have a large number of particles. Your swap and pop method is superior from a performance perspective. Try this:


        for(unsigned int iterator = 0;
            iterator < currentSize;
            iterator++)
        {
            particleList[iterator].lifeTime -= 1.0f;
            if(particleList[iterator].lifeTime <= 0.0f)
            {
                std::swap(particleList[iterator], particleList[particleList.size() - 1]);
                particleList.pop_back();
                iterator--;
                currentSize = particleList.size();
            }
            else
            {
                particleList[iterator].y += 0.01f;
            }
        }

I moved currentSize so it only gets calculated when you update the size. The more important change is that since you either want to delete the current particle or you want to increment the y value so I put the y increment in the else statement which you already had!

And slight addendum to my '-1' bug comment. You use an unsigned int so it wouldn't be negative, it would be very large.

edit: edited for clarity.

C++: A Dialog | C++0x Features: Part1 (lambdas, auto, static_assert) , Part 2 (rvalue references) , Part 3 (decltype) | Write Games | Fix Your Timestep!

++iterator won't fix it. The problem is you use iterator in the same loop iteration in which you decremented iterator. It causes you to go back to the previous iteration value too early. By the time the execution reaches the top of the loop the error has already occurred. Using erase would fix the issue, but erase might be slow if you have a large number of particles. Your swap and pop method is superior from a performance perspective. Try this:


        for(unsigned int iterator = 0;
            iterator < currentSize;
            iterator++)
        {
            particleList[iterator].lifeTime -= 1.0f;
            if(particleList[iterator].lifeTime <= 0.0f)
            {
                std::swap(particleList[iterator], particleList[particleList.size() - 1]);
                particleList.pop_back();
                iterator--;
                currentSize = particleList.size();
            }
            else
            {
                particleList[iterator].y += 0.01f;
            }
        }

I moved currentSize so it only gets calculated when you update the size. The more important change is that since you either want to delete the current particle or you want to increment the y value so I put the y increment in the else statement which you already had!

And slight addendum to my '-1' bug comment. You use an unsigned int so it wouldn't be negative, it would be very large.

edit: edited for clarity.

Unfortunately I tried this and it didn't work either. I tried different combinations and tried placing the statements in different places. I have an itching feeling that it may be an issue with the swap function or the pop function., cuz I never have problems doing these kind of loops in python or other languages, where I decrement the iterator inside the loop so I don't skip elements.

View my game dev blog here!

The standard C++ swap() and pop_back() functions are not broken. I strongly suggest you take the advice from earlier in the thread and walk through your code with a debugger.

Wielder of the Sacred Wands
[Work - ArenaNet] [Epoch Language] [Scribblings]

I officially got it right this time:


		for(std::vector<particle>::iterator iterator = particleList.begin();
			iterator != particleList.end();
			iterator++)
		{
			if(iterator->lifeTime <= 0.0f)
			{
				std::vector<particle>::iterator myAss2 = particleList.begin() + (particleList.size() - 1);
				std::iter_swap(iterator, myAss2);
				//particleList.erase(iterator);
				particleList.pop_back();
				iterator--;
				//outFile << "removed one";
			}
			else
			{
				iterator->y += 0.1f;
				//outFile << iterator->y;
				//outFile << " ";
				iterator->lifeTime -= 1.0f;
			}
			//outFile << "\n";
		}

I debugged it and now I am getting the correct information. The only problem seems to be the rendering, which is a separate issue.

View my game dev blog here!

Consider:

std::swap(*iterator, particleList.back());

Less code and much more clearly communicates your intent.

Another (minor) idea is to use a while loop, and only increment the iterator in your current "else" block, rather than moving it backwards as you currently do.

Another, slightly more major idea, is to use the remove/erase idiom to deal with the removal and std::foreach to deal with

[source]
particleList.erase(std::remove_if(std::begin(particleList), std::end(particleList), [](particleType const &p) { return p->lifetime < 0.0f }), std::end(particleList));
std::for_each(std::begin(particleList), std::end(particleList), [](particleType &p) { p->y += 0.1f});
[/source]
Lambdas can, of course, be replaced with other function object implementations.

This topic is closed to new replies.

Advertisement