current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

This code basically makes the dot follow a certain point on the screen. It consumes a significant amount of computing time in my game, so I'd like it to be best optimized. I wrote this code after quickly reading some pages in a high school mathematics book, so it's unlikely to be a computationally efficient solution.

The mj_ prefix is just what I use instead of a namespace in my C code.

Suggest some good possible optimizations.

struct mj_dot {
    double x;
    double y;
    double th;
};

void mj_dot_rotate(struct mj_dot *, double, double);
void mj_dot_move(struct mj_dot *, double, double, double, double);

static double mj_distance(double x, double y, double x2, double y2) {
    return sqrt(pow(x - x2, 2) + pow(y - y2, 2));
}

void mj_dot_rotate(struct mj_dot *this, double x, double y) {
    double dx = x - this->x;
    double dy = y - this->y;
    this->th = atan(dy / dx);
    if (isnan(this->th)) {
        this->th = 0;
        return;
    }
    if (dx < 0) {
        this->th += MJ_MATH_PI;
    }
}

void mj_dot_move(struct mj_dot *this, double x, double y, double speed, double seconds) {
    double d = speed * seconds;
    if (d >= mj_distance(this->x, this->y, x, y)) {
        this->x = x;
        this->y = y;
    } else {
        mj_dot_rotate(this, x, y);
        this->x += d * cos(this->th);
        this->y += d * sin(this->th);
    }
}
share|improve this question
    
Please add some comments on what exactly are you doing in which function. It's really unclear what are you trying to achieve. –  sha Jun 12 at 18:07

3 Answers 3

up vote 4 down vote accepted

Simplification

There's actually no need for the function mj_dot_rotate() with its complicated trigonometry. That function does lot of work to find an angle which isn't ever needed. In fact, it would be most efficient to inline the code for mj_distance() as well since you can reuse parts of it in mj_dot_move(). The key thing to realize is that you can determine the amounts to move in each direction purely as a function of dx, dy, and distance. Like this:

void mj_dot_move(struct mj_dot *this, double x, double y, double speed, double seconds) {
    double d  = speed * seconds;
    double dx = x - this->x;
    double dy = y - this->y;
    double distance = sqrt(dx*dx + dy*dy);
    if (d >= distance) {
        this->x = x;
        this->y = y;
    } else {
        double fraction = d / distance;
        this->x += dx * fraction;
        this->y += dy * fraction;
    }
}

Variable naming

I agree with the other reviewer who said not to use this as a variable name. I thought for a second that you were using C++ and I started to look for a class definition.

share|improve this answer
up vote 8 down vote

pow(x,2) is less efficient than simply multiplying with itself:

static double mj_distance(double x, double y, double x2, double y2) {
    double dx = x - x2;
    double dy = y - y2;
    return sqrt(dx*dx + dy*dy);
}

However because you just compare it against a distance you can do away with the square root and compare it against the square of the distance:

double d = speed * seconds;
if (d*d >= mj_distance_squared(this->x, this->y, x, y)) {

there is a atan2 function which takes a delta y and delta x to return the correct angle.

void mj_dot_rotate(struct mj_dot *this, double x, double y) {
    double dx = x - this->x;
    double dy = y - this->y;
    this->th = atan2(dy, dx);
}
share|improve this answer
    
Your answer was great and very helpful, especially for the atan2 function. It just wasn't the fastest solution :) –  xiver77 Jun 13 at 10:27
up vote 4 down vote

Overall, this looks pretty reasonable. @ratchet freak's suggestions are pretty good, too. Here are a few other suggestions:

Use Typedefs

Why not make the struct a legitimate type?

typedef struct mj_dot {
    double x;
    double y;
    double th;
} mj_dot;

Then it can be used like this:

void mj_dot_rotate(mj_dot *this, double x, double y) {...

It's less junk to type and to read.

Name function arguments

The function prototypes just have types and not names. Why? What are you trying to save by doing that? The only possible outcome is that you'll look at it and not know the order of your parameters and have to scroll to (or worse open the file with) the actual functions and see what the order is. Just write it out and it's self-documenting:

void mj_dot_rotate(struct mj_dot *this, double x, double y);
void mj_dot_move(struct mj_dot *this, double x, double y, double speed, double seconds);

Also, the name this is a reserved word in C++ and likely to confuse others maintaining the code. It's also not at all descriptive. I'd name it something like dot or point or position.

In one function the variable d represents a delta and in another it's a distance. Why not just call the variables deltaX, deltaY and distance? It'll be easier to read and maintain that way.

Name Functions Appropriately

The mj_dot_rotate() function doesn't rotate anything. It calculates an angle. It should be called something like mj_dot_calculate_angle() instead. And it should probably return the angle rather than modifying its argument. The caller can decide if they want the struct modified or not. Something like this:

double mj_dot_calculate_angle(const mj_dot* position, const double x, const double y)
{
    double deltaX = x - position->x;
    double deltaY = y - position->y;
    return atan2(deltaY, deltaX);
}

Either that, or change the function so it actually does rotate the dot. That might be better since mj_dot_move() does actually move the dot you pass in. They should be consistent.

A question - should the distance function mj_distance() take 2 mj_dot structures instead of just 2 x,y combos? For that matter, should all of the functions that take an x and a y take an mj_dot structure instead? It's not clear how they're called, so without more info, I can't say, but it's worth thinking about.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.