2
\$\begingroup\$

I have made my first physics engine for simulating circles in a plane. Let me know what you think about it.

//main file
#include "col_resolver.cpp"   // collision detector
#include<windows.h>
int main()
{
    int n=0,t=0;                   // number of bodies(n) and t is the number of instants we are going //to simulate
    int x=0;                   //misclaneous use
    cin>>n>>t;
    body B[n];                //array of bodies we are dealing with
    for(int h=0;h<n;++h)    //for loop to input data
    {
        cin>>B[h].pos.i>>B[h].pos.j>>B[h].vel.i>>B[h].vel.j>>B[h].r>>B[h].m;
    }
    for(int i=0;i<t;++i)      //for loop to simulate t times
    {
        for(int j=0;j<n;++j)    //for loop to update position of bodies in next instant of time
        {
            pos_update(B[j]);
        }
        for(int k=0;k<n;++k)   //for loop to check for collisions
        {
            for(int l=(k+1);l<n;++l) //nested for to detect collisions
            {
                x=col_checker(B[k],B[l]);
                if(x==1)
                {
                    col_res(B[k],B[l]);              //resolving collisions
                }
            }
        }
        cout<<"\n instant "<<i+1;
        for(int m=0;m<n;++m)
        {
            cout<<"\n position of body "<<m+1<<"\n"<<B[m].pos.i<<" "<<B[m].pos.j;   //giving output for each body
        }
    }
    system("pause");
    return 0;
}

//collision resolver
#include"collision_detector.cpp"
using namespace std;
void col_res(body& A, body& B);      //function to resolve collision
/*int main()                     //MAIN FUNCTION FOR DEBUGGING PURPOSE
{

}*/
void col_res(body& A,body& B)
{   
    vec ua,ub,va,vb;                          //initial and final velocities of A and B in the rotated coordinate system
    float tan,sin,cos;               //variables to hold sin cos and tan values for the angle through which we rotate our axis
    if(A.pos.j==B.pos.j)
    {
        sin=0;
        cos=1;
    }
    else if(A.pos.i==B.pos.i)
    {
        sin=1;
        cos=0;
    }
    else
    {
        tan=(A.pos.j-B.pos.j)/(A.pos.i-B.pos.i);   // tan calculation by slope method
        sin=tan/(sqrt(1+(tan*tan)));
        cos=sin/tan;
    }
    //we are now rotating our axes to make the line through centre of the bodies the x axis
    ua.i=(A.vel.i*cos)+(A.vel.j*sin);
    ua.j=(A.vel.j*cos)-(A.vel.i*sin);                       //components in rotated system
    ub.i=(B.vel.i*cos)+(B.vel.j*sin);
    ub.j=(B.vel.j*cos)-(B.vel.i*sin);
    va.j=ua.j;                                //collision is along x axis
    vb.j=ub.j;
    va.i=((ua.i*(A.m-B.m))+(2*B.m*ub.i))/(A.m+B.m);  //resolving collision along new x axis
    vb.i=((ub.i*(B.m-A.m))+(2*A.m*ua.i))/(A.m+B.m);
    // now we will convert va and vb back to original coordinate system
    A.vel.i=(va.i*cos)-(va.j*sin);
    B.vel.i=(vb.i*cos)-(vb.j*sin);
    A.vel.j=(va.i*sin)+(va.j*cos);
    B.vel.j=(vb.i*sin)+(vb.j*cos);
}

//collision detector
#include "kinematics.cpp"
#include<cmath>
using namespace std;
int col_checker(body, body);        //function to check if 2 bodies are colliding
/*int main()                            //MAIN FUNCTION FOR DEBUGGING
{
    body A,B;
    int a;
    cin>>A.pos.i>>A.pos.j>>B.pos.i>>B.pos.j>>A.r>>B.r;
    a=col_checker(A,B);
    cout<<a;
    return 0;
}*/
int col_checker(body A, body B)
{
    int a;                                 // a is 1 if there is collision and 0 if no collision
    if(sqrt(((A.pos.i-B.pos.i)*(A.pos.i-B.pos.i))+((A.pos.j-B.pos.j)*(A.pos.j-B.pos.j)))<=(A.r+B.r))     //applying distance formula
    {
        a=1;
    }
    else
    {
        a=0;
    }
    return a;
}

//kinematics simulator
#include "vectors.cpp"        // vector manager file included
struct body               //structure for velocity and position of a rigid circular body
{
    vec pos,vel;       // position of centre of circle and velocity vector
    float r,m;              //radius of circular body  m for mass
};
void pos_update(body&); // function for updating the postion by taking body structure as argument
/*int main()                                   
{                                                   //MAIN FUNCTION FOR DEBUGGING PURPOSE
    body a;
    cin>>a.pos.i>>a.pos.j>>a.vel.i>>a.vel.j;  // taking everyting in input
    a=pos_update(a);                           // updating position
    cout<<a.pos.i<<" "<<a.pos.j;          //giving new position as output
    return 0;   
}*/
void pos_update(body& x)           // taking position and velocities as arguments
{
    x.pos=addition(x.pos,x.vel);           // adding the 2 vectors because we are considering for unit time
}

// vector manager
#include<iostream>
#include<cmath>
using namespace std;
struct vec                  // structure for storing a 2d vector   
{
    float i,j,k;     // 3 components,i,j and k       k component is included to store magnitude of torque
};
vec addition(vec,vec);      //addition of 2 vectors
vec subtraction(vec,vec) ;    //subtraction of 2 vectors
vec scalarmultiply(float,vec);  // multiplying a scalar and vector
float dotproduct(vec,vec)    ;     //dot product of 2 vectors
vec crossproduct(vec,vec)  ;    //cross product of 2 vectors
float mag(vec);               //magnitude of vector
/*int main()
{
    vec a,b,c;
    int d;
    char o;             //operation to be performed
    cin>>o;
    switch(o)
    {
    case 'a':        //addion case
        cin>>a.i>>a.j>>b.i>>b.j;      // taking inout in sequence
        c=addition(a,b);                //performing action
        cout<<c.i<<" "<<c.j;          // output
        break;
    case 's':             // subtraction case
        cin>>a.i>>a.j>>b.i>>b.j;      // taking inout in sequence
        c=subtraction(a,b);
        cout<<c.i<<" "<<c.j;
        break;
    case 'm':                 //scalar multiply
        cin>>d>>b.i>>b.j;
        c=scalarmultiply(d,b);                                                   //MAIN FUNCTION FOR DEBUGGING PURPOSE
        cout<<c.i<<" "<<c.j;          // output
        break;
    case 'd':                        //dotproduct
        cin>>a.i>>a.j>>b.i>>b.j;      // taking inout in sequence
        d=dotproduct(a,b);
        cout<<d;
        break;
    case 'c' :            //crossproduct
        cin>>a.i>>a.j>>b.i>>b.j;      // taking inout in sequence
        c=crossproduct(a,b);
        cout<<c.k;
        break;
    }
    return 0;       
}*/
vec addition(vec a,vec b)          //addition function
{
    vec c;                           //final vector
    c.i=a.i+b.i;                    // adding 'i' components
    c.j=a.j+b.j;                 // adding 'j' components
    return c;                    // returning final vector
}
vec subtraction(vec a,vec b)           //subtraction function
{
    vec c;
    c.i=a.i-b.i;                  //subtracting 'i' components
    c.j=a.j-b.j;                    //subtracting 'j' components
    return c;                    //returning final vector
}
vec scalarmultiply(float a, vec b)       // multiplying vector by a scalar    a is scalar and b is vector
{
    vec c;
    c.i=a*b.i;                       //formula applied
    c.j=a*b.j;
    return c;
}
float dotproduct(vec a,vec b)              //scalar product of 2 vectors, result is a scalar
{
    float c;                                  // resultant scalar
    c=(a.i*b.i)+(a.j*b.j);      // application of formula       
    return c;
}
vec crossproduct(vec a,vec b)      //vector product of 2 products
{
    vec c;
    c.i=0;             // i and j components are 0 as cross product is perpendicular to the plane containing the original vectors
    c.j=0;
    c.k=(a.i*b.j)-(a.j*b.i); //applying formula
    return c;
}
float mag(vec a)
{
    float m;
    m=sqrt((a.i*a.i)+(a.j*a.j));
    return m;
}
\$\endgroup\$
  • 2
    \$\begingroup\$ All in all that looks more like c code than c++. \$\endgroup\$ – πάντα ῥεῖ May 12 '17 at 16:37
  • 2
    \$\begingroup\$ Most of your comments are devoid of sense or just restate the code. In other words, they are worse than useless. \$\endgroup\$ – Deduplicator May 12 '17 at 16:46
3
\$\begingroup\$

Holy moly, there is some really mean stuff in here.

To reiterate the obvious that has already been mentioned

  1. Do not include headers
  2. Do no use namespace std;
  3. Encapsulate your data

  4. Lines are cheap so use it. Stuff like float i,j,k; is a bad practice.

  5. Characters are cheap so use them. Names like float i,j,k; are terrible because noone that read some code knows what it is. If you name the variable torque rather than k it is not necessary to write a comment about it and the user sees the purpose whenever the variable is in the code.

  6. Have a look at the standard constructs like operators.

For example you should create a class for your vector:

class myVector {
public:    
    // Default to the standard constructors
    myVector() = default;
    myVector(const myVector&) = default;
    myVector(myVector&&) = default;

    // Defaulting to the standard assignment operators
    myVector& operator=(const myVector&) = default;
    myVector& operator=(myVector&&) = default;

    ~myVector() = default;

    // create the appropriate operators for mathematical operations
    // Addition with a new vector
    myVector operator+(const myVector& otherVec) {
        myVector temp;
        temp.x_coordinate = x_coordinate + otherVec.x_coordinate;
        temp.y_coordinate = y_coordinate + otherVec.y_coordinate;
        return temp;
    }

    // In place addition
    myVector& operator+=(const myVector& otherVec) {
        x_coordinate += otherVec.x_coordinate;
        y_coordinate += otherVec.y_coordinate;
        return *this;
    }

    // Here come the other operations
private:
    double x_coordinate;
    double y_coordinate;
    double torque;
}

Similarly you should create a class for your body

//kinematics simulator
#include "myVector.h"

//structure for velocity and position of a rigid circular body
class myBody {
public:
    myBody() = default;
    myBody(const myBody&) = default;
    myBody(myBody &&) = default;

    // Defaulting to the standard assignment operators
    myBody& operator=(const myBody&) = default;
    myBody& operator=(myBody&&) = default;

    void updatePosition();
    void updateVelocity();

private:
    myVector pos;           // Position on the 2D map
    myVector vel;           // Velocity
    double r;               // Physical radiums of the body
    double m;               // Mass of the body
};
\$\endgroup\$
  • \$\begingroup\$ Naturally, you wouldn't explicitly default any function without good reason... \$\endgroup\$ – Deduplicator May 12 '17 at 19:48
  • \$\begingroup\$ For complex classes obviously not, but for one that holds 3 doulbe, zero initialization is kind of save. Clearly there should also be a constructor that takes values \$\endgroup\$ – miscco May 12 '17 at 19:59
  • 1
    \$\begingroup\$ "do not include headers" I guess you meant source files \$\endgroup\$ – Incomputable May 12 '17 at 20:25
  • \$\begingroup\$ OOps you are obviously right \$\endgroup\$ – miscco May 12 '17 at 21:33
5
\$\begingroup\$

1. Never ever ...

... do this:

#include "col_resolver.cpp"

cpp files aren't intended to be included, but to be compiled separately and linked with your main.cpp.

Separate out function declarations to header files instead, include these, and provide the implementation in translation units (.cpp files).
I'd expect you would get problems with linking, if those sources are managed in a project file of your IDE.

2. Don't use using namespace std;

While that would work in your particular case, it's considered bad practice. Especially when you move out your code to separate header files.

See more details here please:

Why is “using namespace std;” considered bad practice?

3. Use classes to encapsulate your data

Stuff like that

struct body               //structure for velocity and position of a rigid circular body
{
    vec pos,vel;       // position of centre of circle and velocity vector
    float r,m;              //radius of circular body  m for mass
};
void pos_update(body&); // function for updating the postion by taking body structure as argument

// ...

void pos_update(body& x)           // taking position and velocities as arguments
{
    x.pos=addition(x.pos,x.vel);           // adding the 2 vectors because we are considering for unit time
}

should be placed in a class with appropriate member functions instead:

class body
{
    vec pos,vel;       // position of centre of circle and velocity vector
    float r,m;              //radius of circular body  m for mass
public:
    void pos_update(); // function for updating the postion by taking body structure as argument
};

// ...

void body::pos_update()           // taking position and velocities as arguments
{
    pos=addition(pos,vel);           // adding the 2 vectors because we are considering for unit time
}

4. Don't comment the obvious

The comment here (and in other places)

    x.pos=addition(x.pos,x.vel);           // adding the 2 vectors because 

is useless. It's quite ovious what that line of code does.

Rather use comments to document design decisions, that aren't obvious for future readers/maintainers of your code.


Conclusion

Let me know what you think about it.

All in all I believe that your implementation isn't acceptable and valid c++ code.
It looks more like c code that is "furnished up" with some basic c++ standard library functionality.

Though even with the c language you would obviously miss the point of #include statements, and separating declaration done in header files, and definition provided in translation units which will be stiched together in the linking proces.

The best advice I can give is to pick a book from this list and start to learn the basics.
An overworked c++ style code would look fairly different from what you've been presenting here.

\$\endgroup\$
  • 2
    \$\begingroup\$ No matther who writes a C++ review, I always read the same sentence there: don't use using namespace std; I think it should be automatically posted as soon as you add the c++ tag ;-) \$\endgroup\$ – t3chb0t May 12 '17 at 19:42
  • 1
    \$\begingroup\$ @t3chb0t I've been copying that part from one of my earlier answers of course ;-) \$\endgroup\$ – πάντα ῥεῖ May 12 '17 at 20:13
  • \$\begingroup\$ Also, use an existing, well tested linear algebra library like "eigen" instead of writing out all the operations manually. \$\endgroup\$ – Emily L. May 12 '17 at 23:31

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.