CircleShape movement not working??

[aj1204]

Hello,

I am trying to generate 10 circles at random positions and locations across the screen, and for some reason the movement isn't working (not sure if the use of rand is causing an issue).

The events are supposed to represent 2 different modes, where in one the circles should bounce off the screen bounds, and in the second they should reappear at a random position and velocity. The circles simply flicker and then reappear at random locations, and never more than 5-6 of them.

I've tried both drawing the circles inside and outside of the events thinking I wasn't understanding the process. I've also seen many of you keep track of time for movement, but I've written another fully functional SFML program that works almost exactly the same without any use of time or the clock, the only difference being there's only one circle being moved. I'm a complete beginner and have looked at this so many times that my eyes are glazing over, so sorry if this is absurdly obvious.

using namespace std;

float SIZE = 750.f;

float const stoneRadius = 30.f;

struct Stone
{
    sf::CircleShape st;
    sf::Vector2f velocity;

    Stone(float x, float y, float p1, float p2)
    {

        sf::Vector2f velocity (x,y);
        st.setRadius(stoneRadius);
        st.setFillColor(sf::Color::Black);
        st.setOrigin(stoneRadius, stoneRadius);
        st.setPosition(p1, p2);
    }


    void update_bounce()
    {

        st.move(velocity);

        if (st.getPosition().x - stoneRadius <= 0)
            velocity.x = -velocity.x;
        else if ((st.getPosition().x + stoneRadius) >= SIZE)
            velocity.x = -velocity.x;


        if (st.getPosition().y - stoneRadius <= 0)
            velocity.y = -velocity.y;
        else if (st.getPosition().y + stoneRadius >= SIZE)
            velocity.y = -velocity.y;
    }


    void update()

    {
        srand ( time(NULL) );

        st.move(velocity);

        if ( (st.getPosition().x - stoneRadius) <= 0 || (st.getPosition().x + stoneRadius) <= SIZE
                || (st.getPosition().y - stoneRadius) <= 0 || (st.getPosition().y - stoneRadius >= SIZE) )
        {

            float v = rand() % 4 + (6);
            float p1 = rand () % 1500 + (-750);
            float p2 = rand () % 1500 + (-750);

            velocity.x = v;
            velocity.y = v;
            st.setPosition(p1, p2);
        }

    }
};


int main()
{
    srand ( time(NULL) );

    sf::RenderWindow window(sf::VideoMode(SIZE, SIZE), "SFML Window");

    window.setFramerateLimit(30);


    sf::CircleShape house[4];
    sf::Color colors[4] = {sf::Color::Blue, sf::Color::White, sf::Color::Red, sf::Color::White};
    float sizes[4] = {300, 200, 100, 25};

    for(int i=0; i<4; i++)
    {
        house[i].setPosition(SIZE/2.f, SIZE/2.f);
        house[i].setRadius(sizes[i]/2.f);
        house[i].setFillColor(colors[i]);
        house[i].setOrigin(sizes[i]/2.f, sizes[i]/2.f);
    }

    vector<Stone> gameStones;

    for (int x = 0; x< 10; x++)
    {
        float v =  rand() % 12 + (-6);
        float p1 = rand() % 1500 + (-750);
        float p2 = rand() % 1500 + (-750);
        gameStones.push_back(Stone(v,v,p1,p2));
    }


    while(window.isOpen())
    {

        sf::Event event;

        while (window.pollEvent(event))

        {

            if (event.type == sf::Event::KeyPressed && event.key.code == sf::Keyboard::Q)

            {

                window.close();
                break;
            }

            else if (event.type == sf::Event::KeyPressed && event.key.code == sf::Keyboard::R)

            {

                for (int i=0; i<10; i++)
                    gameStones[i].update();


                }


            else if (event.type == sf::Event::KeyPressed && event.key.code == sf::Keyboard::B)

            {

                for (int i=0; i<10; i++)
                    gameStones[i].update_bounce();


            }

        }

        window.clear(sf::Color::White);

        for (auto& gameStone : gameStones)
            window.draw(gameStone.st);

        for (int i=0; i<4; i++)
            window.draw(house[i]);

        window.display();

    }

    return 0;

}
 

[kitteh-warrior]

http://en.sfml-dev.org/forums/index.php?topic=5559

A couple things I noticed at a quick glance at your code:
-The structure of your event loop should be using a switch statement, or at least a nested if.
-On line 128, you have

window.display();

Why? Unless some of your omitted code draws something, but then that should be omitted too? But it wouldn't matter, because right after that, the screen gets cleared and drawn again... but the limit of 30 FPS would make the main function halt there until 1/30 of a second passes, so this makes no sense to me.

[aj1204]

Ok, have fixed the nested if and removed the display. Same issue - only 1 stone displaying now and no reaction to key presses.

Re: the link, sorry, what should I fix to improve? Length? Location? I didn't think this was related to a specific module since I don't know where the problem originates. Should I move to Window?

[kitteh-warrior]

Ok, have fixed the nested if and removed the display. Same issue - only 1 stone displaying now and no reaction to key presses.

Exactly how did you do the nested if?
I meant it in the way of:

Re: the link, sorry, what should I fix to improve?

As stated in the link:

Identify the problem (a.k.a "complete and minimal code")

It is not complete and minimal, because just by looking at it I know that I will get a lot of errors by copying and pasting it into Notepad++ and compiling it. Reasons:
Use of vector<Stone> without any "using namespace"'s anywhere (which should be avoided anyway, in my opinion anyway).
The ellipses.

[Hapax]

I can't fully tell due to your indentation being inconsistent but it looks like your drawing (clear, draw, display) is in the event loop. It should not be there; it should be outside of the event loop and inside the main loop (window.isOpen).

[Arcade]

As kitteh-warrior mentioned, it is usually easier to help spot the problems if you can reduce your code to only what is necessary to see the problem, but still compilable. That way we can copy and compile it to see exactly what is going wrong.

Anyway, I did go through what you posted and, in addition to what Hapax said, this looks like it could be at least part of your problem:

(st.getPosition().x - stoneRadius) <= 0 || (st.getPosition().x + stoneRadius) <= SIZE

This is in your update function. Should that second comparison instead be ">=" ?

Edit: Also, this is likely supposed to be an addition, not subtraction

(st.getPosition().y - stoneRadius >= SIZE)

[Hapax]

I didn't look closely enough to see the line that Arcade mentioned but, if SIZE is the width (and therefore diameter) of the stone, then both of those comparisons are identical and therefore pointless. If my assumption about SIZE is correct, changing it to >= instead would cause this if statement to always evaluate to true.

Also, ew:

        window.draw(gameStones[0].st);
                window.draw(gameStones[1].st);
                window.draw(gameStones[2].st);
                window.draw(gameStones[3].st);
                window.draw(gameStones[4].st);
                window.draw(gameStones[5].st);
                window.draw(gameStones[6].st);
                window.draw(gameStones[7].st);
                window.draw(gameStones[8].st);
                window.draw(gameStones[9].st);

(click to show/hide)

Try these. If one doesn't work, try the next:

for (auto& gameStone : gameStones)
    window.draw(gameStone.st);

for (vector<Stone>::iterator it = gameStones.begin(); it != gameStones.end(); ++it)
    window.draw(it->st);

for (unsigned int gameStoneIndex = 0; gameStoneIndex < gameStones.size(); ++gameStoneIndex)
    window.draw(gameStones[gameStoneIndex].st);

[Arcade]

I was interpreting SIZE as the dimensions of the window instead of the size of the stone. I was guessing that the code was trying to check if the stone is outside the window, and if so, evaluate to true to therefore move the stone to a new random position.

Edit: Also, your stones aren't moving because you only move them in the Update functions, but you only call those functions when you press R or B. You need to call them every iteration of your loop, not just when you press a button. You will need some other mechanism to determine if update() or update_bounce() should be called.

[Hapax]

I was interpreting SIZE as the dimensions of the window instead of the size of the stone. I was guessing that the code was trying to check if the stone is outside the window

This looks more likely. With SIZE being 750, it does look more like a square window size or, at least, a square boundary size. My bad 

[aj1204]

Sorry for my delayed response. I hear you all and have updated the original code to be as minimal as possible and compilable. There's a lot of code re: collision checking and other features that had to be cut out so sorry for the original mistakes.

As is, you can compile and see that the stones aren't displaying in the correct number before the events, and are moving when events are called but very sporadically with only 1-2 circles show at a time. The draw and display ARE outside of the event loop, sorry it was hard to tell. I actually don't want the stones to move or display unless either event is called, so the starting screen should just be the target and the draw/update functions should probably all be inside events. But for purposes of testing I've left them outside of  events here. It may just be my numbers that are screwed up and I've looked at it for too long to tell.

[Arcade]

I see you updated your code in your first post, but did you see my first reply? You still have those same logic errors in there that I pointed out. In addition to those, here are some other problems I noticed:

 Stone(float x, float y, float p1, float p2)
    {

        sf::Vector2f velocity (x,y);
        ...
 

You are creating this velocity variable and never using it. You likely meant to set the Stone's velocity member variable.

float p1 = rand () % 1500 + (-750);
 

You do things like this in several places. This will generate a number between -750 and 750. However, your window is only 750 pixels across. You should probably be generating numbers between 0 and 750. Why not just rand()%750?

   void update()

    {
        srand ( time(NULL) );
        ...
 

Do not call srand(time(NULL)) here. You are already doing so in your main function. srand(time(NULL)) only needs to be called once to set up the psuedo random number generator. As you have it now, if Update() gets called more than once in a second (which is very likely), then time(NULL) will evaluate to the same seed, therefore giving you the same random numbers. Basically, if 2 or more of your stones go off the screen at the same time then they will be put at the same random location with the same random velocity. They will overlap making it look like some have disappeared.

For future reference, I highly recommend learning how to use a debugger. A lot of these problems can be found quickly using one.

[Hapax]

First, this is not compilable code, simply because you haven't included libraries it depends on. Yes, we all know it requires SFML (Graphics only, I assume), but if you want people to help you, you should make it easier for them, not harder. You adding that line could make the difference between someone helping you and someone giving up.
Imagine it yourself:
Someone says "This is compilable. Help me!"
You paste it and it fails to compile.
It could be enough to just say "Meh! If they're not willing to actually provide compilable code, why should I help them?"

Second, PLEASE include a way to quit through normal channels. e.g. clicking the close on the window should close it.
I had to force-close it because I didn't know that you'd set it up to only close when you press a specific key that you obviously favoured for this task.

Thirdly, I highly recommend not using rand() but using std::random instead. Sure, it can be a little more complicated to set up but it's relatively reliable and unbiased. Saying that, for a test program such as this, rand() can be fine temporarily.

Fourthly, you have no need to "use namespace std". Fully qualifying them (adding std:: before them) is simple, especially since you've already done that with SFML (sf::).

Fifthly, global variables here are unnecessary and they don't help anything. Declare them where they're needed and pass them to functions/classes that need them.

Sixthly (no more "n-thly" after this one!), the event loop is fully processed every frame and every frame should also be processed every frame (this should be obvious).
If you want something to happen when an event happens, you should only "start" it happening (or just signal that it should start) in the event loop. Process the "something to happen" outside of that loop.
In a simple project, it could just be a few bools that keep track of the state you want to be in.

I'm now going to look at your code 

Why don't you make a std::vector of houses too, in the similar way you make a vector of stones. Iterating through them (for example, to draw them all), would be able to be done in the same way as gameStones e.g. (for (auto& house : houses)
For that matter, why not use that iteration (for (auto& stone : gameStones)) elsewhere instead of for (i = 0; i<10; i++)?

If you want your stones to be moving every frame, they should be updated every frame, not once - whenever you press a key.
Move those updates out of the event loop 

If you don't want to allow a stone to be at least a "stoneRadius" away from the edges of the window, you shouldn't be creating them there. e.g. If you create one randomly at (10, 10), how does your code deal with this?

You should read our previous posts. There are important things that you haven't fixed that we told you about.
Fixing some of the things we mentioned (including the thing at the bottom of this post), I managed to get a bunch of circles bouncing around (while holding B - I didn't fix the update-in-event-loop problem).

You are creating this velocity variable and never using it. You likely meant to set the Stone's velocity member variable.

I assume that they think this:

                st.move(velocity);

is using it.

You are creating a temporary sf::Vector2f with the same name as the one in the class and assigning x and y to that one.
You are not setting x and y of the velocity in the class.

Just so you know, this is the error that, once fixed, makes everything do what you think it does (assuming that you fixed what we said and read what we wrote about events)


EDIT: I'm sure Jesper would want you to consider looking at this working example of a bouncing ball on the SFML Wiki.

[aj1204]

Thank you both and sorry for being such an amateur! I left the libraries out intentionally because I wasn't sure if they'd be annoying. Thank you for all your comments - I've been consolidating and playing with all the suggestions before replying. This is a big help and thank you for the link to the bouncing ball.

A couple of questions:

1) Why do you prefer random to rand?
2) Great point about not allowing a stone to be created less than Radius distance from a wall. Relatedly, I'm now wondering how I could make sure stones don't generate at a point where another stone is (at least in the initial positions)?
3) Is there a way to set up event triggers as a "key last pressed" instead of having to have the key remain pressed?
4) I've tried and failed to add a texture to the balls. From the documentation, it seems like the problem might be the template becomes disassociated with the object upon its return to main. Is writing a copy constructor inside Stone with the template the best solution to this?

[Jesper Juhl]

1) Why do you prefer random to rand?

I can't explain that better than Stephan T. Lavavej in his rand() Considered Harmful talk, so I'll just advice you to go and watch that.

2) Great point about not allowing a stone to be created less than Radius distance from a wall. Relatedly, I'm now wondering how I could make sure stones don't generate at a point where another stone is (at least in the initial positions)?

There are several ways you could do that. One simple way would be to simply check all existing stones for collision with the new one before adding it and if there is any collission then simply generate a new random location - repeat until you generate a position that does not collide, then add the stone there and proceed.

3) Is there a way to set up event triggers as a "key last pressed" instead of having to have the key remain pressed?

In the event loop when you get a keypress event do something like "keyIsPressed = true", then test "if (keyIsPressed)" outside the event loop. Reset to false when you get a key release event.
You could also look into using the Thor Input module (tutorial here).

4) I've tried and failed to add a texture to the balls. From the documentation, it seems like the problem might be the template becomes disassociated with the object upon its return to main. Is writing a copy constructor inside Stone with the template the best solution to this?

No.
Just keep the texture alive for as long as it is in use.

[Hapax]

1) Why do you prefer random to rand?

it's relatively reliable and unbiased.

The video Jesper posted is much more in depth; it's worth a watch!

3) Is there a way to set up event triggers as a "key last pressed" instead of having to have the key remain pressed?

You could store that in a variable if you require to know the previously pressed key.

sf::Keyboard::Key previouslyPressedKey;

and update it whenever anything is pressed.

previouslyPressedKey = event.key.code; // important! put inside the scope of: if (event.type == sf::Event::Keypressed)

I'm not sure why you need this, though. It would be more clear to store the current 'state' or 'mode', such as

bool isBouncingEnabled = false;

and change that in the event loop, then use that bool to decide how to update outside of the event loop.

In the event loop when you get a keypress event do something like "keyIsPressed = true", then test "if (keyIsPressed)" outside the event loop. Reset to false when you get a key release event.

I wouldn't recommend this. There doesn't seem to be much reason for it; might as well just use sf::Keyboard::isKeypressed() (outside of the event loop).
I do tend to use that technique but only for 'modifier' keys as their state needs to be known at the time of the event.