12
\$\begingroup\$

I am designing a planetary system simulator. The coordinates used are standard for JPanel (0,0 in the upper left corner).

My model is based on a class Body:

public class Body {
    private double x, y; // position
    private double w, h; // dimensions for drawing
    private double vx = 0, vy = 0; // velocities along x, y axes
    private double ax = 0, ay = 0; // accelerations along x, y axes
    private double mass = 1; // default = 1
    private boolean stationary = false; // stationary body is centered during simulation

    /* ... not showing all the setters and getters */

Because the force of gravity is the same for any two interacting bodies, I designed the methods to take another body as an argument, so we can call planet1.interact(planet2)

Distance calculation:

public double calculateDistX(Body other) {
    double x1 = this.getX();
    double x2 = other.getX();
    return Math.sqrt(Math.pow(x2 - x1, 2));
}

public double calculateDistY(Body other) {
    double y1 = this.getY();
    double y2 = other.getY();
    return Math.sqrt(Math.pow(y2 - y1, 2));
}

public static double calculateDistance(double dist_x, double dist_y) {
    return Math.sqrt(Math.pow(dist_x, 2) + Math.pow(dist_y, 2));
}

Finally, the method that calculates gravitational force:

public void interact(Body other) {
    double x = calculateDistX(other);
    double y = calculateDistY(other);
    double r = calculateDistance(x, y);

    double force = (this.getMass() * other.getMass()) / Math.pow(r, 2); 
    double force_x = force * (x / r); // force * cos
    double force_y = force * (y / r); // force * sin

    /* calculate accelerations for both bodies, set vector orientation */
    if (other.getX() > this.getX()) {
        this.setAx(force_x / this.getMass());
        other.setAx(-force_x / other.getMass());
    } else {
        this.setAx(-force_x / this.getMass());
        other.setAx(force_x / other.getMass());
    }

    if (other.getY() > this.getY()) {
        this.setAy(force_y / this.getMass());
        other.setAy(-force_y / other.getMass());
    } else {
        this.setAy(-force_y / this.getMass());
        other.setAy(force_y / other.getMass());
    }

    /* calculate velocities for both bodies */
    this.setVx(this.getVx() + this.getAx());
    this.setVy(this.getVy() + this.getAy());

    other.setVx(other.getVx() + other.getAx());
    other.setVy(other.getVy() + other.getAy());

    /* calculate positions for both bodies */
    this.setX(this.getX() + this.getVx());
    this.setY(this.getY() + this.getVy());

    other.setX(other.getX() + other.getVx());
    other.setY(other.getY() + other.getVy());
}

I don't really know how good/bad this model is. The section in interact that sets the vector orientation looks like it can be simpler, but I can't figure it out.

Also, I have doubts about this method, because calling:

planet1.interact(planet2);
planet1.interact(planet3);
planet2.interact(planet3);

Makes planet2 and planet3 interact AFTER the original position of planet1 and planet2 has changed, and it's not how it's hapenning in reality. I don't know how important that would be, though.

\$\endgroup\$
  • 2
    \$\begingroup\$ Welcome to Code Review! This looks like a great first question, I'm sure you'll get some equally great answers from our Java pros! \$\endgroup\$ – Phrancis May 1 '15 at 17:27
  • \$\begingroup\$ I don't know if this is appropriate for Code Review, but if the accuracy of the simulation is important to you, you may want to consider implementing a more sophisticated numerical integrator. What you're doing essentially amounts to Euler's method... \$\endgroup\$ – David Zhang May 2 '15 at 17:18
  • \$\begingroup\$ ...which, being only a first-order method, can be horribly inaccurate when the forces involved change rapidly over time. On the other hand, if you don't need high accuracy and only want your simulation to look convincing for simple systems, the added complexity is probably not worth it. \$\endgroup\$ – David Zhang May 2 '15 at 17:22
  • \$\begingroup\$ I was thinking about implementing the Runge-Kutta method or Verlet algorithm. I think this does involve changing my interact(Body) method and passing time to the method somehow? Right now, I have a Timer in my drawing method that calls the interact(Body) for every Body each time. \$\endgroup\$ – RK1 May 2 '15 at 21:41
14
\$\begingroup\$

Math.pow(x, 2) -> x*x

A pet peeve of mine is people calling Math.pow() just to square a number. To me it seems faster and simpler to just multiply the value by itself.

Shadowing variable names

In interact(), you use the variables x and y. I would avoid using variable names that shadow instance variables. I would call these variables dx and dy instead.

X and Y distances

Look closely at your x/y distance calculations functions:

public double calculateDistX(Body other) {
    double x1 = this.getX();
    double x2 = other.getX();
    return Math.sqrt(Math.pow(x2 - x1, 2));
}

It actually just does this:

public double calculateDistX(Body other) {
    return Math.abs(this.getX() - other.getX());
}

This shorter version is also less prone to roundoff errors. But in fact, I would even remove this function, because it will be more useful to use to preserve the sign of the subtraction than to hide it (read below)

Splitting up interact and update

Your interaction function should do no more than modify the acceleration of the object. Once the body has interacted with all other bodies, its acceleration will have the final acceleration for that timeslice. After that, you should have an update function which takes the acceleration and applies it to the velocity, and then applies the velocity to the position. I'll show an example of that below.

Simplifying the interaction function

Looking at your interaction function, there are a lot of places that can be simplified. First, if you retained the sign of the x/y distances, you wouldn't need all those if statements. Second, a lot of the force calculations are related to each other and can be merged into a smaller set of calculations. In fact, when I rewrote your function, it became this simple:

public void interact(Body other) {
    double dx = other.getX() - x;
    double dy = other.getY() - y;
    double r  = calculateDistance(dx, dy);
    double r3 = r * r * r;

    /* calculate accelerations for both bodies */
    ax += other.getMass() * dx / r3;
    ay += other.getMass() * dy / r3;
    other.addToAx(mass * -dx / r3);
    other.addToAy(mass * -dy / r3);
}

public void update() {
    vx += ax;
    vy += ay;

    ax = 0;
    ay = 0;

    x += vx;
    y += vy;
}

Further simplifications

If you don't actually need the current velocity of the object other than to update the position (e.g for collisions), you can get rid of the acceleration entirely and operate directly on the velocity.

Edit: Also, I think that multiplications are faster than divisions, so you could also invert r3 so that you only need to do one divide. You can also precompute dx/r^3 and dy/r^3 to reduce the number of multiplies:

public void interact(Body other) {
    double dx = other.getX() - x;
    double dy = other.getY() - y;
    double r  = calculateDistance(dx, dy);
    double inv_r3 = 1.0 / (r * r * r);

    /* Precalculate force component (1/r^2) times direction (dx/r) = dx / r^3 */
    dx *= inv_r3;
    dy *= inv_r3;

    /* calculate accelerations for both bodies */
    vx += other.getMass() * dx;
    vy += other.getMass() * dy;
    other.addToVx(mass * -dx);
    other.addToVy(mass * -dy);
}

public void update() {
    x += vx;
    y += vy;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ The amount of things I learned and noticed thanks to your answer is incredible. Just a quick fix, its r^2, not r^3. Thank you! \$\endgroup\$ – RK1 May 1 '15 at 18:56
  • 5
    \$\begingroup\$ @RK1 I think it's r^3 because you divide by r^2 to get the force, and then you multiply by (dx/r) to get the direction, thus introducing another r to the denominator. Also look at my edit. I now compute 1.0/(r*r*r) to reduce the number of divisions. \$\endgroup\$ – JS1 May 1 '15 at 19:11
  • 1
    \$\begingroup\$ Matt.pow makes intent clearer. \$\endgroup\$ – Caridorc May 1 '15 at 19:47
  • 1
    \$\begingroup\$ I wonder my aversion to Math.pow() is related to the fact that I use C as my main language. In C, I would never call pow() to take something to the 2nd or 3rd power. Reasons: 1) It's slower. 2) You need to include math.h and link libm.a. But if you guys insist that Math.pow() is better in Java, I believe you. But I still wouldn't use it myself :). Does the java runtime optimize Math.pow(2) and (3) to do it the easy way? \$\endgroup\$ – JS1 May 1 '15 at 20:36
  • 1
    \$\begingroup\$ I'd have to disagree that Math.pow is clearer. I actually share this pet peeve with JS1. I prefer repeated multiplication for small powers (I'd say less than about 4). Combined with the appropriate variable name, I'd say the repeated multiplication is perfectly clear in intent. I dislike premature optimization as much as the next guy, but the repeated multiplication is almost surely faster and is used somewhere that will likely be a performance bottleneck. Since I'd argue there's no loss in clarity (or developer time), I'd go for multiplication. \$\endgroup\$ – Kyle May 2 '15 at 0:29
7
\$\begingroup\$

Your doubts are correct, and separating your interact method into two would solve this problem. Let interact handle acceleration and velocities, and make a new method that sets the positions of all of the planets after they have interacted.

 public void interact(Body other) {
    double x = calculateDistX(other);
    double y = calculateDistY(other);
    double r = calculateDistance(x, y);

    double force = (this.getMass() * other.getMass()) / Math.pow(r, 2); 
    double force_x = force * (x / r); // force * cos
    double force_y = force * (y / r); // force * sin

    /* calculate accelerations for both bodies, set vector orientation */
    if (other.getX() > this.getX()) {
        this.setAx(force_x / this.getMass());
        other.setAx(-force_x / other.getMass());
    } else {
        this.setAx(-force_x / this.getMass());
        other.setAx(force_x / other.getMass());
    }

    if (other.getY() > this.getY()) {
        this.setAy(force_y / this.getMass());
        other.setAy(-force_y / other.getMass());
    } else {
        this.setAy(-force_y / this.getMass());
        other.setAy(force_y / other.getMass());
    }

    /* calculate velocities for both bodies */
    this.setVx(this.getVx() + this.getAx());
    this.setVy(this.getVy() + this.getAy());

    other.setVx(other.getVx() + other.getAx());
    other.setVy(other.getVy() + other.getAy());
}

public void simulate() {
    /* calculate position */
    this.setX(this.getX() + this.getVx());
    this.setY(this.getY() + this.getVy());
}

Then you can do:

planet1.interact(planet2);
planet1.interact(planet3);
planet2.interact(planet3);

planet1.simulate();
planet2.simulate();
planet3.simulate();

This way, the simulation happens after all of the interactions have been calculated.

\$\endgroup\$
3
\$\begingroup\$

Just some notes about sqrt and pow. These are computationally expensive. Use them sparingly. Besides what the others have already pointed out you are also using the distance to calculate force. So in your code you go :

(to abbreviate)
dx     : x1-x2
dy     : y1-y2
dm     : m1-m2
calcDx : sqrt(pow(dx,2))
calcDy : sqrt(pow(dy,2))
calcD  : sqrt( pow(calcDx,2)+pow(calcDy,2) )
calcF  : dm / pow(calcD,2)

You do 3 sqrts and 4 pows. Why not just do this:

calc d2 (d squared): dx*dx + dy*dy
calc F             : dm / d2

I have used zero of the expensive operations. I will be so bold as to say that I bet you can get rid of ALL of your pow() calls and reduce your sqrt() calls to a single call in one strategic spot.

For comparisons sake let's say the simplest operation takes 1 unit of time. Then the operations are roughly like so:

comparison :   1 cycle
+/-        :   1 
multiply   :   4
divide     :  13
sqrt       :  13
pow        : 100

The exact numbers will depend on optimization level and particular architecture, but those are rules of thumb. Using the power operator involves taking logs of each of the operators, multiplying, and then taking the inverse log, each a rather expensive operation in itself.

\$\endgroup\$
  • \$\begingroup\$ Thank you, this is very useful and will certainly keep the simulation smooth when dealing with large amounts of objects. \$\endgroup\$ – RK1 May 1 '15 at 21:50
2
\$\begingroup\$

I actually did exactly this back in the Java 4 days. There are a few things that I might change. Simple code is generally good code; and simple often (but not exclusively) means short. The reason is that often, the bigger concern than computational efficiency is human readability, but I'm sure you'll see what I mean here.

First of all, I notice that you are squaring a value, and then taking the root, to get it as a positive in your calculateDist* methods. This may work on the surface, but aside from introducing a (very mild) inefficiency it also introduces floating point error on each call. It's much easier, and safer, to simply call:

Math.abs(x2 - x1);

As abs() simply adjusts the sign. Mathematically, save for the floating point error, you are correct. An additional slimming down of these methods might lead us to:

public calculateDist*(Body other) {
    return Math.abs(this.get*() - other.get*());
}

If it doesn't bother you, you could simply use this:

public calculateDistance(Body body) {
    return Math.sqrt(Math.pow(this.getX() - body.getX(), 2)
        + Math.pow(this.getY() - body.getY(), 2));
}

That is very much up to the comprehension of the math of you, and those who you are working with.

The if/then statements were the first ones that I had to root through. They can make life a lot easier where code-consideration is concerned; but they can also make your code much more verbose.

Given that the only thing that changes, between the two blocks, is the sign of the argument, I would like to introduce this to you.

Math.signum(double d)

This method returns zero for zero, negative one for any number less than zero, and positive one for any number above zero. Using it, your code:

if (other.getX() > this.getX()) {
    this.setAx(force_x / this.getMass());
    other.setAx(-force_x / other.getMass());
} else {
    this.setAx(-force_x / this.getMass());
    other.setAx(force_x / other.getMass());
}

if (other.getY() > this.getY()) {
    this.setAy(force_y / this.getMass());
    other.setAy(-force_y / other.getMass());
} else {
    this.setAy(-force_y / this.getMass());
    other.setAy(force_y / other.getMass());
}

Can be reduced to:

this.setAx(Math.signum(other.getX() - this.getX()) * (force_x / this.getMass()));
other.setAx(-Math.signum(other.getX() - this.getX() * (force_x / other.getMass()));
this.setAy(Math.signum(other.getY() - this.getY()) * (force_x / this.getMass()));
other.setAx(-Math.signum(other.getY() - this.getY() * (force_x / other.getMass()));

Of course you could also pull out the Math.signum() calls and store them in a factor variable; I wouldn't worry too much about redundant calls because one, the optimizer should pull it out for you anyway; and two, it's 2015 and not back-in-the-day. However, the human element might appreciate it.

Lastly, as long as we're talking about "good" (and presumably "ethical") code, it's common practice among Java programmers to choose the verbose path. In the limit of your class or project, it might be clear what "Ax" and "Ay" mean; but to a later programmer, it looks an awful lot like the infamous alphabet-soup antipattern. It might be better to suffix the methods with "AccelerationX" and "AccelerationY", in proper camel case of course. The same goes for velocity. It may seem out-of-sorts to insist on typing the entire method out, but if you're using a utility like Eclipse or NetBeans (which I advise that you do) then autocomplete will do the work for you anyway.

The last points I can make to help you are these. There's a saying in comp science—if you have to do it twice, make a function out of it. Secondly, physics emulations loan well to the Java 8 Stream API; but that's a lot to pick up and at least through Java 7 this is quality code. I do recommend you look into a tutorial on it, though; it greatly simplifies Java code.

Otherwise, excellent work!


Here's an additional suggestion for you. I'm noticing that you're working with time units of call-frequency. That can be fine if you're managing all the math from outside the method, but in this instance it doesn't look like you are. Here's what I recommend.

There's a method called:

long System.nanoTime()

This method returns the difference between the current moment, and some arbitrary (but constant!) point in time, kind of like Epoch time. If this is going to be called repeatedly, I suggest keeping a global variable on hand, along the lines of:

long time = -1;

and at the beginning of your method, insert this:

long newTime = System.nanoTime();
long dT = time != -1 ? time - newTime : 0;
time = newTime;    //update

Remembering that your time variable is in billionths of a second, you can get the number of seconds that have passed with:

(double)time/1000000000.0

You could also declare time as a double, but I find that in most cases just casting it in one place is more efficient. Again, the real concern is your reader.

Later on, when you're dealing with edge cases and collision detection, that field will help.

Also, if this is being run like a game, I would suggest making the class that does the calling implement Runnable, and call Thread.yield() after each run cycle. This is a little out of the way, but it will aid response time for controls, especially if they're from the keyboard.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the input! The code is already working and I'm testing it. There is a major problem, though, which is unphysical accelerations when two bodies get too close. I will be implementing some energy-conserving collision system soon. \$\endgroup\$ – RK1 May 1 '15 at 21:07
  • \$\begingroup\$ I wouldn't call that a mild inefficiency. taking the square root of a number is computationally expensive. I find your distance function is still overly complicated. Keep it simple: dx = x1-x2. Thats it! \$\endgroup\$ – Octopus May 1 '15 at 21:23

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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