Deleting an object from a vector

[DylanMorgan]

Hey all, I feel like such an idiot having to post this problem again. I have looked it up but can't seem to find the exact answer for the error I am having. The program runs completely fine, I then make the ball hit any block and the program crashes. It throws an exception saying:

Microsoft Visual Studio C Runtime Library has detected a fatal error in Pong.exe.



Press Break to debug the program or Continue to terminate the program.

I ran it in debug and the screen attatched popped up on my screen

I am not sure what this even means, below is the code for anything I think could be relevant, if you want to see anything else just ask and I'll edit this to include it. Thank you for helping anyone!

containerOfBlocks.h

#pragma once

class ContainerOfBlocks
{
public:
        ContainerOfBlocks(int yPosition, sf::Color colour);
        ~ContainerOfBlocks();

        std::vector<Block>& getContainer();
       
        void drawContainer(sf::RenderWindow& window);
        void deleteObjectFromVector(int objectNumber);

private:
        std::vector<Block> blockContainer;
};

containerOfBlocks.cpp

ContainerOfBlocks::ContainerOfBlocks(int yPosition, sf::Color colour)
{
        blockContainer.push_back(Block(2, 2));
        blockContainer.push_back(Block(104, 2));
        blockContainer.push_back(Block(206, 2));
        blockContainer.push_back(Block(308, 2));
        blockContainer.push_back(Block(410, 2));
        blockContainer.push_back(Block(512, 2));
        blockContainer.push_back(Block(614, 2));
        blockContainer.push_back(Block(716, 2));
        blockContainer.push_back(Block(818, 2));
        blockContainer.push_back(Block(920, 2));

        for (unsigned int i = 0; i < 10; ++i)
        {
                blockContainer[i].setPosition(yPosition);
        }

        for (unsigned int i = 0; i < 10; ++i)
        {
                blockContainer[i].setColour(colour);
        }
}

std::vector<Block>& ContainerOfBlocks::getContainer()
{
        return blockContainer;
}

void ContainerOfBlocks::drawContainer(sf::RenderWindow& window)
{
        for (unsigned int i = 0; i < 10; ++i)
        {
                blockContainer[i].draw(window);
        }
}

void ContainerOfBlocks::deleteObjectFromVector(int objectNumber)
{
        blockContainer.erase(blockContainer.begin() + 2);
}

main.cpp

int main()
{
        float windowWidth = 1022;
        float windowHeight = 648;

        sf::RenderWindow window(sf::VideoMode(windowWidth, windowHeight), "pong");
        window.setPosition(sf::Vector2i(125, 0));

        ContainerOfBlocks blockContainer(2, sf::Color::Red);
        ContainerOfBlocks blockContainer2(29, sf::Color::Yellow);

        while (window.isOpen())
        {
                sf::Event event;
                while (window.pollEvent(event))
                {
                        if (event.type == sf::Event::Closed)
                                window.close();        
                }

                for (unsigned int i = 0; i < 10; i++)
                {
                        if (ball.getPosition().intersects(blockContainer.getContainer()[i].getPosition()))
                        {
                                // testing
                                std::cout << "Hit 1st row, block: " << i;

                                // Reverse ball
                                ball.reboundSurface();

                                // Delete block
                                blockContainer.deleteObjectFromVector(i);
                                break;
                        }
                        else if (ball.getPosition().intersects(blockContainer2.getContainer()[i].getPosition()))
                        {
                                // testing
                                std::cout << "Hit 2nd row, block: " << i;

                                // Reverse ball
                                ball.reboundSurface();

                                // Delete Block
                                blockContainer2.deleteObjectFromVector(i);
                                break;
                        }
                }

[cvkfak]

Code: [Select]

void ContainerOfBlocks::deleteObjectFromVector(int objectNumber)
{
    blockContainer.erase(blockContainer.begin() + 2);
}
In "blockContainer.begin() + 2", i think is the problem ( im not sure if is on porpuse)
Anyways, if you do a for funtion to delete a objet from a vector, you need to do a i-- in case some one is erased, so... the fixed code is:

Code: [Select]

void ContainerOfBlocks::deleteObjectFromVector(int objectNumber)
{
    blockContainer.erase(blockContainer.begin() + objectNumber);
}

// On main.cpp replace the code of the for funtion by this
for (unsigned int i = 0; i < 10; i++)
        {
            if (ball.getPosition().intersects(blockContainer.getContainer()[i].getPosition()))
            {
                // testing
                std::cout << "Hit 1st row, block: " << i;

                // Reverse ball
                ball.reboundSurface();

                // Delete block
                blockContainer.deleteObjectFromVector(i);
                i--;  // Fix... i think
                break;
            }
            else if (ball.getPosition().intersects(blockContainer2.getContainer()[i].getPosition()))
            {
                // testing
                std::cout << "Hit 2nd row, block: " << i;

                // Reverse ball
                ball.reboundSurface();

                // Delete Block
                blockContainer2.deleteObjectFromVector(i);
                i--;   // Fix
                break;
            }
PD: sorry for my bad english
PD 2: i not have test it

[eXpl0it3r]

So you randomly delete the object that is two positions away fron the beginning? I assume this not intended?

You should make use of iterators instead of integer index and you should really get rid of these magic numbers. Don't randomly iterate 10 times, but iterate until you reach the container's size or with iterators until you hit the container's end().
If you wrap a container in a class, you shouldn't expose it to the outside. Instead implement enumerator functions on the class, that will apply a certain check or condition to all the container's elements within the class.

[DylanMorgan]

So you randomly delete the object that is two positions away fron the beginning? I assume this not intended?

Nope, this was not intended, I changed it for testing purposes. Not quite sure why, I only wanted to check if I could delete the individual element so I did not add it in then. I have changed it now to have the size checked and also have a function that returns the size of the vector so that it can be used outside of the class.

If you wrap a container in a class, you shouldn't expose it to the outside. Instead implement enumerator functions on the class, that will apply a certain check or condition to all the container's elements within the class.

Not quite sure what you mean by this, can you explain please?

Anyways, if you do a for funtion to delete a objet from a vector, you need to do a i-- in case some one is erased, so... the fixed code is:

I added this code and still get the same error. However, the block does now delete when it is hit (image attatched) yet the whole program still breaks when it hits the object.

[cvkfak]

Oh i did't noticed the random i<10 xD

Code: [Select]

for (unsigned int i = 0; i < blockContainer.getContainer.size() ; i++)   // Fix, not sure at all :P
        {
            if (ball.getPosition().intersects(blockContainer.getContainer()[i].getPosition()))
            {
                // testing
                std::cout << "Hit 1st row, block: " << i;

                // Reverse ball
                ball.reboundSurface();

                // Delete block
                blockContainer.deleteObjectFromVector(i);
                i--;  // Fix... i think
                break;
            }
            else if (ball.getPosition().intersects(blockContainer2.getContainer()[i].getPosition()))
            {
                // testing
                std::cout << "Hit 2nd row, block: " << i;

                // Reverse ball
                ball.reboundSurface();

                // Delete Block
                blockContainer2.deleteObjectFromVector(i);
                i--;   // Fix
                break;
            }
Anyways you need to split the for funtion for the two vectors

[DylanMorgan]

Oh i did't noticed the random i<10 xD

Yeah eXpl0it3r pointed it out so it has been changed now.

Yes I realized I needed to split them once I had to change the size. Didn't do it though, I have done it now and it works perfectly thank you both for the help!

[eXpl0it3r]

Not quite sure what you mean by this, can you explain please?

Basically every time you see getContainer()  you know that the design principle of encapsulation and abstraction has been broken. Because your class gives out details about its implementation and gives direct access to its inner structure.
So one way to solve this design issue is to create enumeration functions, that means create functions on the class, which will take some parameters and apply something to all elements. For example: Say you want to do collision checks on all elements, instead of returning the container, you pass in the ball's position to the function and do the intersection test within the class and then return a meaningful result.
That way the container never leaves that class and the interaction between that class and your other objects will become more expressive and easier to understand, because of better abstraction and better encapsulation.

[Hapax]

If you wrap a container in a class, you shouldn't expose it to the outside. Instead implement enumerator functions on the class, that will apply a certain check or condition to all the container's elements within the class.

Not quite sure what you mean by this, can you explain please?
[/quote]
I suppose the question to ask yourself here is why the blockContainer vector is private if it's completely accessible to the public anyway. Yes, I'm aware that I helped you with providing this access

It should either be private and have public functions that do the things to the container that you need it to do (as eXpl0it3r mentioned) or it should just be public. If it's public, though, why do you need the container when you can do the same thing with just a vector (and a simple one- or two-line piece of code to draw them)