6
\$\begingroup\$

I've decided to revisit a chunk of code I wrote a while back, and see what I could do to make some changes. As a result, I know have slow, but working piece of code which is able to simulate gravitational attractions between \$n\$ amount of bodies. Like usual, I'm using the formula described in Newton's Law of Gravitation, as seen below:

$$F=G\frac{m_1m_2}{r^2}$$

using UnityEngine;

/// <summary>
/// Gravity component.
/// </summary>
[RequireComponent(typeof(Rigidbody))]
public class GravityComponent : MonoBehaviour 
{
    public float gravityConstant = 1;
    public Vector3 startingForce;

    /// <summary>
    /// Applies attraction to the object.
    /// </summary>
    public void ApplyAttraction()
    {
        foreach(GameObject body in GameObject.FindGameObjectsWithTag("body"))
        {
            Vector3 force = body.transform.position - this.transform.position;
            float attraction = 
                  (this.gravityConstant * this.rigidbody.mass * body.rigidbody.mass)
                / (Mathf.Pow(force.magnitude, 2));
            this.rigidbody.AddForce(force * attraction);
        }
    }

    public void Start()
    { this.rigidbody.AddForce(this.startingForce); }

    public void FixedUpdate()
    { this.ApplyAttraction(); }
}

Ideally, I'd like to know the following things:

  • Can the performance be better? Right now it runs at an okay speed with about 10 bodies on my laptop, but once it starts getting past that, it begins to slow down rather drastically.
  • Am I doing anything wrong Unity3d-wise?
  • Is my code written in an idiomatic manner?
  • Anything else?

For anyone who might be interested, you can find the repository on GitHub here.

\$\endgroup\$
  • \$\begingroup\$ Um, the gravitational constant is not 1. It's 6.674×10^−11. \$\endgroup\$ – Quill Nov 23 '15 at 1:26
  • \$\begingroup\$ @Quill I'm aware of that. I chose to use 1 for simplicity in this case. \$\endgroup\$ – Ethan Bierlein Nov 23 '15 at 1:27
  • \$\begingroup\$ Your equations are wrong: in AddForce you evaluate force * attraction but in this way you are neglecting the magnitude of the distance between the body (that is in force). You have to normalize the distance vectori before multiplying for the attraction or you can change the equation of attraction using the power of 3 for force.magnitude. Anyway you should change the names of the variable force: it is not a force but a distance vector, call it distance or distanceVector. \$\endgroup\$ – N74 Nov 23 '15 at 9:14
3
\$\begingroup\$

The first thing striking my eye is that you are taking the square of a vectors magnitude/length. Most frameworks offer a property to get the squared length directly (sqrMagnitude in the case of unity). The reason that this property exists is that calculating the length of a vector involves taking a square root and if you need that squared you can skip both taking the square root and the squaring.

float attraction = 
    (this.gravityConstant * this.rigidbody.mass * body.rigidbody.mass)
  / force.sqrMagnitude;

This should already give you a significant speed up.

The next thing to try is caching everything that won't change between calls to ApplyAttraction(). For example, if the number of bodies doesn't change store the result of GameObject.FindGameObjectsWithTag("body") in a field and reuse it

private readonly GameObject[] _bodies;

public GravityComponent()
{
    _bodies = GameObject.FindGameObjectsWithTag("body");
}

...

foreach(GameObject body in _bodies)

If the masses of the bodies are constant you can cache the product of their masses and the gravitational constant in the constructor too.

private readonly GameObject[] _bodies;
private readonly Dictionary<GameObject, float> _massProducts;

public GravityComponent()
{
    _bodies = GameObject.FindGameObjectsWithTag("body");
    _massProducts = _bodies.ToDictionary(
        body => body,
        body => this.gravityConstant * this.rigidbody.mass * body.rigidbody.mass);
}

This reduces the ApplyAttraction() method to

public void ApplyAttraction()
{
    var thisPosition = this.transform.position;
    foreach(var body in _bodies)
    {
        var force = body.transform.position - thisPosition;
        var attraction = _massProducts[body] / force.sqrMagnitude;
        this.rigidbody.AddForce(force * attraction);
    }
}

I don't think you can get any faster than that.

| improve this answer | |
\$\endgroup\$
3
\$\begingroup\$

Disclaimer: I have no idea about Unity3D.

that being said, if there is no reason regarding Unity3D to keep this public

public float gravityConstant = 1;
public Vector3 startingForce;  

then make it private.


I don't like this method writing style

public void Start()
{ this.rigidbody.AddForce(this.startingForce); }

public void FixedUpdate()
{ this.ApplyAttraction(); }  

IMO if something doesn't fit good into the same line, one should use 4 lines of code.


The only obvious place where you can gain some performance boost would be at calculating the attraction.

float attraction = 
              (this.gravityConstant * this.rigidbody.mass * body.rigidbody.mass)
            / (Mathf.Pow(force.magnitude, 2));  

instead of calling Mathf.Pow(force.magnitude, 2) a simple force.magnitude * force.magnitude will be faster, because there won't be done any checks if e.g the power contains a valid value.

| improve this answer | |
\$\endgroup\$

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.