9
\$\begingroup\$

This is a very lengthy class called Particle, it relies on two other headers, one of them is a simple struct which contains an x and y (Vector2), and the other header gets a material's properties (materialProperties.h). The class is no where near done, and I am struggling to get my collision detection function to work. This means that currently the bounciness property has no real purpose, and odd things will happen in void checkParticleMovement(std::vector< std::vector<Particle> >& particleArray), for example, when there are two particles, one behind the other, moving at the same speed, the previous particle won't move on the grid.

The code has very infrequent comments, so lots of it may not make sense. It also uses a 2D vector grid, because it is much faster than a normal vector. If the code appears to be unclear without the other files, then I will put the code on Github or something (but it uses SFML for drawing).

//Standard C++:
#include <iostream>
#include <string>
#include <vector>
#include <math.h>
//My Headers:
#include "vector2.h"
#include "materialDatabase.h"

class Particle
{
    private:
        //Coords:
        vector2 coords;
        //Velocities:
        vector2 velocity;
        //Material:
        std::string material;
        //Expanding:
        bool fillToolExpands = false;
        //Mass:
        double mass = 0;
        //Bounciness:
        double bounciness = 0.5;
    public:
        //All values:
        void setAllValues(vector2, vector2, std::string, double, double);
        void setEmpty();

        //Copy Particle:
        void copyParticle(Particle&);

        //Coords:
        void setCoords(vector2);
        vector2 getCoords();
        vector2 getPreciseCoords();

        //Velocities:
        void giveVelocity(vector2);
        void setVelocity(vector2);
        vector2 getVelocity();

        //Material:
        void setMaterial(std::string);
        void setMaterialProperties(std::string);
        std::string getMaterial();

        //Expanding:
        void setFillParticle(bool);
        bool isFillParticle();

        //Mass:
        void setMass(double);
        double getMass();

        //Bounciness:
        void setBounciness(double);
        double getBounciness();

        //Gravitational Velocity:
        void calculateGravitationalVelocity(Particle&);

        //Update:
        void update();
};

//Set values:
void Particle::setAllValues(vector2 startCoords, vector2 startVelocity, std::string startMaterial, double startMass, double startBounciness)
{
    coords = startCoords;
    velocity = startVelocity;
    material = startMaterial;
    mass = startMass;
    bounciness = startBounciness;
}
void Particle::setEmpty()
{
    coords = vector2(floor(coords.x), floor(coords.y));
    velocity = vector2(0, 0);
    material = "empty";
    mass = 0;
    bounciness = 0;
}

//Copy Particle:
void Particle::copyParticle(Particle& particleToCopyTo)
{
    particleToCopyTo.setAllValues(coords, velocity, material, mass, bounciness);
}

//Coords:
void Particle::setCoords(vector2 newCoordinates)
{
    coords = newCoordinates;
}
vector2 Particle::getCoords()
{
    vector2 flooredCoords(floor(coords.x), floor(coords.y));
    return flooredCoords;
}
vector2 Particle::getPreciseCoords()
{
    return coords;
}

//Velocities:
void Particle::giveVelocity(vector2 addedVelocity)
{
    velocity.x = velocity.x + addedVelocity.x;
    velocity.y = velocity.y + addedVelocity.y;
}
void Particle::setVelocity(vector2 newVelocity)
{
    velocity = newVelocity;
}
vector2 Particle::getVelocity()
{
    return velocity;
}

//Material:
void Particle::setMaterial(std::string newMaterial)
{
    material = newMaterial;
}
void Particle::setMaterialProperties(std::string newMaterial)
{
    material = newMaterial;
    mass = getMaterialMass(newMaterial);
    bounciness = getMaterialBounciness(newMaterial);
}
std::string Particle::getMaterial()
{
    return material;
}

//Expanding:
void Particle::setFillParticle(bool isFill)
{
    fillToolExpands = isFill;
}
bool Particle::isFillParticle()
{
    return fillToolExpands;
}

//Mass:
void Particle::setMass(double newMass)
{
    mass = newMass;
}
double Particle::getMass()
{
    return mass;
}

//Bounciness:
void Particle::setBounciness(double newBounciness)
{
    bounciness = newBounciness;
}
double Particle::getBounciness()
{
    return bounciness;
}

//Gravitational Velocity:
void Particle::calculateGravitationalVelocity(Particle& distantParticle)
{
    //Physics constants:
    const double G = 0.00000000006673; //Gravitational Constant (or Big G)

    //Get coords of particle:
    vector2 coords1 = coords;
    //Get coords of particle with gravity:
    vector2 coords2 = distantParticle.getCoords();
    //Get the difference vector:
    vector2 rV(coords2.x - coords1.x, coords2.y - coords1.y);

    //Distances:
    double r = pow(rV.x, 2) + pow(rV.y, 2);
    double r2 = sqrt(r);

    if (r != 0)
    {
        //Normalize the difference vector
        vector2 u(rV.x / r, rV.y / r);
        //Acceleration of gravity
        double a = G * distantParticle.getMass() / r2;
        //Set the velocity:
        velocity.x = velocity.x + (a * u.x / 1000);
        velocity.y = velocity.y + (a * u.y / 1000);
    }
}

//Update:
void Particle::update()
{
    coords.x = coords.x + velocity.x;
    coords.y = coords.y + velocity.y;
}

//Miscellaneous Functions:
void checkParticleMovement(std::vector< std::vector<Particle> >& particleArray)
{
    int vectorWidth = particleArray[0].size();
    int vectorHeight = particleArray.size();
    std::vector< std::vector<bool> > updated(vectorHeight, std::vector<bool> (vectorWidth, 0));

    //Make incrementer:
    int incrementX = 0;
    int incrementY = 0;

    while (incrementY != vectorHeight)
    {
        //Check if it needs to be moved:
        if ((particleArray[incrementY][incrementX].getMaterial() != "empty") && (updated[incrementY][incrementX] == false))
        {
            int coordX = particleArray[incrementY][incrementX].getCoords().x;
            int coordY = particleArray[incrementY][incrementX].getCoords().y;
            //Moving a particle in the grid:
            if ((coordX != incrementX) || (coordY != incrementY))
            {
                if (particleArray[coordY][coordX].getMaterial() == "empty")
                {
                    //Copy Particle:
                    particleArray[incrementY][incrementX].copyParticle(particleArray[coordY][coordX]);
                    //particleArray[coordY][coordX].setCoords(vector2(coordX, coordY));
                    //Delete previous particle:
                    particleArray[incrementY][incrementX].setEmpty();
                    particleArray[incrementY][incrementX].setCoords(vector2(incrementX, incrementY));
                }
            }
            //Make sure the particle can't be updated multiple times:
            updated[coordY][coordX] = true;
        }

        ++incrementX;
        if (incrementX == vectorWidth)
        {
            incrementX = 0;
            ++incrementY;
        }
    }
}

//Collision Detection:
void handleCollisionDetection(std::vector< std::vector<Particle> >& particleArray)
{
    int vectorWidth = particleArray[0].size();
    int vectorHeight = particleArray.size();

    double highestVelocity = 0;

    std::vector< std::vector<vector2> > velocities(vectorHeight, std::vector<vector2>(vectorWidth));
    std::vector< std::vector<vector2> > coords(vectorHeight, std::vector<vector2>(vectorWidth));
    std::vector< std::vector<std::string> > materials(vectorHeight, std::vector<std::string>(vectorWidth));

    //FIND THE HIGHEST VELOCITY (TO DIVIDE WITH):
    int incrementX = 0;
    int incrementY = 0;
    while (incrementY != vectorHeight)
    {
        velocities[incrementY][incrementX] = particleArray[incrementY][incrementX].getVelocity();
        if (velocities[incrementY][incrementX].x > highestVelocity) {highestVelocity = ceil(velocities[incrementY][incrementX].x);}
        if (velocities[incrementY][incrementX].y > highestVelocity) {highestVelocity = ceil(velocities[incrementY][incrementX].y);}

        coords[incrementY][incrementX] = particleArray[incrementY][incrementX].getPreciseCoords();

        materials[incrementY][incrementX] = particleArray[incrementY][incrementX].getMaterial();

        ++incrementX;
        if (incrementX == vectorWidth)
        {
            incrementX = 0;
            ++incrementY;
        }
    }

    //Remove minus number
    highestVelocity = fabs(highestVelocity);

    incrementX = 0;
    incrementY = 0;
    while (incrementY != vectorHeight)
    {
        if (materials[incrementY][incrementX] != "empty")
        {
            vector2 dividedVelocityStart = velocities[incrementY][incrementX];
            if (velocities[incrementY][incrementX].x != 0)
            {
                dividedVelocityStart.x = dividedVelocityStart.x / highestVelocity;
                if (std::isnan(dividedVelocityStart.x) == true) {dividedVelocityStart.x = 0;}
            }
            else {dividedVelocityStart.x = 0;}

            if (velocities[incrementY][incrementX].y != 0)
            {
                dividedVelocityStart.y = dividedVelocityStart.y / highestVelocity;
                if (std::isnan(dividedVelocityStart.y) == true) {dividedVelocityStart.y = 0;}
            }
            else {dividedVelocityStart.y = 0;}

            vector2 dividedVelocityIncrement = dividedVelocityStart;
            while (dividedVelocityIncrement <= velocities[incrementY][incrementX])
            {
                int incrementXLowLimit = incrementX - (highestVelocity * 2);
                if (incrementXLowLimit < 0) {incrementXLowLimit = 0;}
                int incrementXHighLimit = incrementX + (highestVelocity * 2);
                if (incrementXHighLimit >= vectorWidth) {incrementXHighLimit = vectorWidth;}

                int incrementX2 = incrementXLowLimit;

                int incrementY2 = incrementY - (highestVelocity * 2);
                if (incrementY2 < 0) {incrementY2 = 0;}
                int incrementYHighLimit = incrementY + (highestVelocity * 2);
                if (incrementYHighLimit >= vectorHeight) {incrementYHighLimit = vectorHeight;}

                while (incrementY2 != incrementYHighLimit)
                {
                    if ((materials[incrementY2][incrementX2] != "empty") && (incrementX != incrementX2) && (incrementY != incrementY2))
                    {
                        vector2 dividedVelocityStart2 = velocities[incrementY2][incrementX2];
                        vector2 dividedVelocityIncrement2 = dividedVelocityStart2;
                        dividedVelocityIncrement2.x = dividedVelocityIncrement2.x / highestVelocity;
                        if (std::isnan(dividedVelocityIncrement2.x) == true) {dividedVelocityIncrement2.x = 0;}
                        dividedVelocityIncrement2.y = dividedVelocityIncrement2.y / highestVelocity;
                        if (std::isnan(dividedVelocityIncrement2.y) == true) {dividedVelocityIncrement2.y = 0;}

                        while (dividedVelocityIncrement2 <= velocities[incrementY2][incrementX2])
                        {
                            if ((floor(coords[incrementY][incrementX].x + dividedVelocityIncrement.x) ==
                                 floor(coords[incrementY2][incrementX2].x + dividedVelocityIncrement2.x))
                            &&  (floor(coords[incrementY][incrementX].y + dividedVelocityIncrement.y) ==
                                 floor(coords[incrementY2][incrementX2].y + dividedVelocityIncrement2.y)))
                            {
                                std::cout << "COLLISION!" << std::endl;
                            }

                            if (dividedVelocityIncrement2.x >= 0) {dividedVelocityIncrement2.x = dividedVelocityIncrement2.x + dividedVelocityStart2.x;}
                            else {dividedVelocityIncrement2.x = dividedVelocityIncrement2.x - dividedVelocityStart2.x;}

                            if (dividedVelocityIncrement2.y >= 0) {dividedVelocityIncrement2.y = dividedVelocityIncrement2.y + dividedVelocityStart2.y;}
                            else {dividedVelocityIncrement2.y = dividedVelocityIncrement2.y - dividedVelocityStart2.y;}

                            //For minus values:
                            if (dividedVelocityIncrement2 <= 0)
                            {
                                if (dividedVelocityIncrement2 >= velocities[incrementY2][incrementX2]) {break;}
                            }
                        }
                    }

                    ++incrementX2;
                    if (incrementX2 == incrementXHighLimit)
                    {
                        incrementX2 = incrementXLowLimit;
                        ++incrementY2;
                    }
                }

                if (dividedVelocityIncrement.x >= 0) {dividedVelocityIncrement.x = dividedVelocityIncrement.x + dividedVelocityStart.x;}
                else {dividedVelocityIncrement.x = dividedVelocityIncrement.x - dividedVelocityStart.x;}

                if (dividedVelocityIncrement.y >= 0) {dividedVelocityIncrement.y = dividedVelocityIncrement.y + dividedVelocityStart.y;}
                else {dividedVelocityIncrement.y = dividedVelocityIncrement.y - dividedVelocityStart.y;}

                //For minus values:
                if (dividedVelocityIncrement <= 0)
                {
                    if (dividedVelocityIncrement >= velocities[incrementY][incrementX]) {break;}
                }
            }
        }

        ++incrementX;
        if (incrementX == vectorWidth)
        {
            incrementX = 0;
            ++incrementY;
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ If the code doesn't work like you expect it to, isn't it off-topic to post it here? \$\endgroup\$ – R Sahu Feb 24 '15 at 5:34
6
\$\begingroup\$

There is quite a bit of code here, so this is by no means a complete review. A few main points that caught my attention:

  • Your class could use some parameterized constructors. That would reduce the need for all the set* methods. The work done by setAllValues(), for instance, should clearly be done by a constructor.

  • copyParticle() is also redundant with operator =, but some might prefer the more C-ish syntax of copying via a function.

  • I would advise keeping function parameter names in the function/method prototypes, as this adds to the documentation of the code.

  • You did not follow Const Correctness for the get*() methods. They are not altering any member state, so should be marked as const.

  • Use <cmath> for C++, <math.h> is actually the C-language header file.

  • fillToolExpands is a strange name. The methods that manipulate it are setFillParticle() and isFillParticle(), so shouldn't it be called something like fillParticle or fillEnabled?

  • Another issue with fillToolExpands: Avoid declaring booleans in the middle of classes. A bool is usually 1 byte in size, so they will break data alignment and force the compiler to pad the bool to the size of a word, making your class larger in terms of memory. Placing bools always at the end of classes/structs will make the need for compiler-generated padding less frequent and won't require padding between fields.

  • Column alignment of similar lines is something that helps me digest code. This is certainly arguable, but I would change a block like this:

    coords = startCoords;
    velocity = startVelocity;
    material = startMaterial;
    mass = startMass;
    bounciness = startBounciness;
    

    To this:

    coords     = startCoords;
    velocity   = startVelocity;
    material   = startMaterial;
    mass       = startMass;
    bounciness = startBounciness;
    
  • Don't use pow() to calculate the square of a number. That will call a function which can be an expensive one. Instead, just multiply the number by itself.

    double r = pow(rV.x, 2) + pow(rV.y, 2);
    double r2 = sqrt(r);
    

    Simpler and faster:

    double length = sqrt((rV.x * rV.x) + (rV.y * rV.y));
    
  • In the mathematical update of the particles, done by calculateGravitationalVelocity(), you use a few single letter variable names. Try to provide better and more descriptive names instead. E.g. r/r2 are actually the length of the vector. a is the acceleration of gravity. Use descriptive names and the comments can even be removed.

  • floor(), ceil(), fabs() and all functions declared by <cmath> are all members of namespace std. Technically, compilers are not required to expose such functions in the global namespace, so for good portability, make sure to always prefix them with the std:: namespace resolution.

\$\endgroup\$
  • \$\begingroup\$ What do you mean by "reduce the need of get/set functions" when you are commenting on my setAllValues function? I understand how the constructor could easily replace setAllValues but how would it help with my abundance of get/set functions? \$\endgroup\$ – Orfby Feb 25 '15 at 13:52
  • \$\begingroup\$ @Orfby, well that actually depends. You didn't provide a usage example, but I assume you create a particle with the default constructor and then call a bunch of set*() methods to set it up properly. You could replace that by one or more parameterized constructors. My wording was unclear. This has no relation with the get*() methods. The getters can only be removed by placing more logic inside the class. \$\endgroup\$ – glampert Feb 25 '15 at 17:32
  • \$\begingroup\$ Ah, that makes much more sense. In my code I have a 2D vector of "empty" particles, then the individual variables are changed every tick, like adding earth gravity using giveVelocity, for example. \$\endgroup\$ – Orfby Feb 25 '15 at 17:40

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.