Sign in to follow this  

Any way to improve my balls? (No, not 'my', but 'my ping pong' ))

This topic is 4397 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

Right now for my ping pong clone I have a MAXBALLS constant that is 10. Then I create an array with MAXBALLS CBalls - which inherit from CSprite. Now what I'm worried about is that I'm looping a LOT. Once in the beginning to set everything:
for (int i = 0; i < MAXBALLS; i++)
    {
        if (allBalls[i].SetSprite("Data/ball.bmp") == false)
            return false;

        allBalls[i].Reset();

        SDL_SetColorKey(allBalls[i].GetSprite(),
                        SDL_SRCCOLORKEY,
                        SDL_MapRGB(allBalls[i].GetSprite()->format, 255, 255, 255));

        allBalls[i].BallActive = false;
    }

Once in the updating to update the balls:
for (int i = 0; i < MAXBALLS; i++)
    {
        if (!allBalls[i].BallActive)
            continue;

        if (allBalls[i].BallLeft)
            allBalls[i].Setx(allBalls[i].Getx() - allBalls[i].GetSpeed());
        if (!allBalls[i].BallLeft)
            allBalls[i].Setx(allBalls[i].Getx() + allBalls[i].GetSpeed());

        if (allBalls[i].BallUp)
            allBalls[i].Sety(allBalls[i].Gety() - allBalls[i].GetSpeed());
        if (!allBalls[i].BallUp)
            allBalls[i].Sety(allBalls[i].Gety() + allBalls[i].GetSpeed());

        if (allBalls[i].Getx() < 0)
        {
            allBalls[i].Setx(0);

            allBalls[i].BallLeft = false;
        }
        if (allBalls[i].Getx() + allBalls[i].GetSprite()->w > SCREEN_W)
        {
            allBalls[i].Setx(SCREEN_W - allBalls[i].GetSprite()->w);

            allBalls[i].BallLeft = true;
        }

        if (allBalls[i].Collide(player1))
            allBalls[i].BallUp = true;
        if (allBalls[i].Collide(player2))
            allBalls[i].BallUp = false;

        if (allBalls[i].Gety() < 0 || allBalls[i].Gety() + allBalls[i].GetSprite()->h > SCREEN_H)
            allBalls[i].Reset();

        if (allBalls[i].GetSpeed() > MAXSPEED)
            allBalls[i].SetSpeed(MAXSPEED);
        if (allBalls[i].GetSpeed() < MINSPEED)
            allBalls[i].SetSpeed(MINSPEED);
    }

And last, but not least, in the drawing:
for (int i = 0; i < MAXBALLS; i++)
    {
        if (allBalls[i].BallActive == false)
            continue;

        allBalls[i].Draw(mainApp.GetScreen());
    }

However, is this slow? That's why I added the continues, so if the ball isn't active it doesn't do anything, but still, it might affect something. So my question is, is there anywhere where I can improve my code?

Share this post


Link to post
Share on other sites
The first thing that I see you can do is to share the image itself rather than load and make a new surface for each one. Since the balls will not change in apperence or anything, you can use that one image instead and save yourself some performance there. To get that done all you would need to do is load the ball image once into some master SDL_Surface, then assing each of the ball's SDL_Surface's to point to that image. That way each ball has it's own position and stuff, but shares the image.

Next, the line:
allBalls[i].Setx(allBalls[i].Getx() - allBalls[i].GetSpeed());

Is the root of a lot of evil right there! Why not something like this:
allBalls[i].UpdateX();
Where that function would look something like:

UpdateX()
{
x += speed;
}

And in this case, your speed has direction, so it'll need to take on - values for when its going left and positive when going right.

Each time you call that GetX, then GetSpeed, then the SetX, that's a WHOLE lot of unnecessary operations thats slowing you down. As soon as you make that change, you should see a significant performance improvement. That's all I have time for now, exam soon, so I'll check back again later with more suggestions. Good luck!

Share this post


Link to post
Share on other sites
Quote:
Original post by Drew_Benton
The first thing that I see you can do is to share the image itself rather than load and make a new surface for each one. Since the balls will not change in apperence or anything, you can use that one image instead and save yourself some performance there. To get that done all you would need to do is load the ball image once into some master SDL_Surface, then assing each of the ball's SDL_Surface's to point to that image. That way each ball has it's own position and stuff, but shares the image.

Next, the line:
allBalls[i].Setx(allBalls[i].Getx() - allBalls[i].GetSpeed());

Is the root of a lot of evil right there! Why not something like this:
allBalls[i].UpdateX();
Where that function would look something like:

UpdateX()
{
x += speed;
}

And in this case, your speed has direction, so it'll need to take on - values for when its going left and positive when going right.

Each time you call that GetX, then GetSpeed, then the SetX, that's a WHOLE lot of unnecessary operations thats slowing you down. As soon as you make that change, you should see a significant performance improvement. That's all I have time for now, exam soon, so I'll check back again later with more suggestions. Good luck!


Thanks, I'll do that. Also, can't I just use a static for the 'master'?

Share this post


Link to post
Share on other sites
your single SDL_SetColorKey function sticks out like a sore thumb. perhaps if it was included in setSprite it would look better (codewise)

sharing the sufaces, go for it!

Quote:

Thanks, I'll do that. Also, can't I just use a static for the 'master'?


master?...

Share this post


Link to post
Share on other sites
Quote:
Original post by agi_shi
Also, can't I just use a static for the 'master'?


You could if you want, but there's no real need, since you are working with pointers. I'd say you have a global image manager that you could load images once, then retrieve that pointer. Of course you could also just make a global SDL_Surface* for it as well. Your call, the manager would take longer, but provides a valuable reusable component.

Share this post


Link to post
Share on other sites
Note: The compiler can, in release mode, optimize all that get/set stuff away to be effectively as fast as the proposed Update() method anyway. BUT, you should use the Update() method anyway, because that is how OO is supposed to work - the ball is not "a thing that has an x and a speed"; it is "a thing that can move".

Thus:


void Ball::move() {
if (!BallActive) return; // equivalent to the old 'continue'.
// Actually, you should not bother with BallLeft; all you need to do
// is 'x += speed', and set speed negative when you want to move left.
if (BallLeft) { x -= speed; }
else { x += speed; }
// Similarly y. Ok, maybe you'll want separate speed vars for horizontal
// and vertical movement - that will get you movement on arbitary angles
// "for free" too :) I would normally just call them 'dx' and 'dy', because
// I have a physics background. ;)
if (BallUp) { y -= speed; }
else { y += speed; }

// Handle reflections.
// As another optimization, you can pull this constant out - if all balls are
// the same size, you could make it a static const member of the class...
const int max_w = SCREEN_W - sprite->w;
const int max_h = SCREEN_H - sprite->h;
if (x < 0) { x = 0; BallLeft = false; }
if (x > SCREEN_W) { x = SCREEN_W; BallLeft = true; }
if (Collide(player2)) { BallLeft = false; }
if (Collide(player1)) { BallLeft = true; }
if (y < 0 || y > SCREEN_H) { Reset(); }

// Limit speed.
if (speed > MAXSPEED) { speed = MAXSPEED; }
if (speed < MINSPEED) { speed = MINSPEED; }
}


As for your "once in the beginning to set everything", that's what constructors are for:


Ball::Ball(Sprite* s) {
sprite = s;
Reset();
SDL_SetColorKey(sprite, SDL_SRCCOLORKEY,
SDL_MapRGB(sprite->format, 255, 255, 255));
BallActive = false;
}
// Now we don't provide SetSprite(), but instead create the single Sprite
// instance externally, and call constructors with a pointer to it.
// (There's no reason they can't share the same Sprite instance, I assume.
// If there is, then create separate ones external to the constructor.)
// In that case, the Ball would probably want to clean up its own Sprite
// in the destructor:
// Ball::~Ball() { SDL_FreeSurface(sprite); } // if it's an SDL_Surface*
// Ball::~Ball() { delete sprite; } // if it's something of your own that
// has a proper destructor of its own


And naturally, drawing can do its own checks too:


void Ball::draw(const Screen& context) {
if (!BallActive) return;
// do what you would have done otherwise
}


And the calling code is now much as you would expect:


Sprite* foo = createMyBallSpriteSomehow();
for (int i = 0; i < MAXBALLS; ++i) {
allBalls[i] = Ball(foo); // not 'new'; this is an array of instances, not
// of pointers.
}
// later update:
for (int i = 0; i < MAXBALLS; ++i) {
allBalls[i].move();
}
// later draw:
for (int i = 0; i < MAXBALLS; ++i) {
allBalls[i].draw();
}


There are cuter ways to do the actual looping, but they boil down to effectively the same thing in the end, and are probably not called for here. :)

Share this post


Link to post
Share on other sites

This topic is 4397 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this