patterncppMinor
N-body sim with Barnes-Hut - Follow up
Viewed 0 times
hutwithbarnesfollowbodysim
Problem
Link to previous question: Barnes-Hut N-body simulator
It still has issues I'm sure.
Node.h
Node.cpp
```
#include "Node.h"
#include
#define _MAX_DEPTH 100
Node::Node(std::vector pBodies, float pwidth, float pheight, float px, float py, unsigned int pDepth)
{
SetParam(pBodies, pwidth, pheight, px, py, pDepth);
}
void Node::SetParam(std::vector pBodies, float pwidth, float pheight, float px, float py, unsigned int pDepth)
{
HasChildren = false;
Depth = pDepth;
unsigned int size = pBodies.size();
Bodies = pBodies;
posX = px;
posY = py;
width = pwidth;
height = pheight;
float mass = 0;
double Centerx = 0;
double Centery = 0;
for (unsigned int i = 0; i mass;
Centerx += pBodies[i]->posX;
Centery += pBodies[i]->posY;
}
TotalMass = mass;
if (size > 0)
{
CenterOfMassx = static_cast(Centerx / size);
CenterOfMassy = static_cast(Centery / size);
}
else
{
CenterOfMassx = 0;
CenterOfMassy = 0;
}
if (size > 1 && Depth q1Bodies, q2Bodies, q3Bodies, q4Bodies;
for (unsigned int i = 0; i posX posY posY Reset();
delete
It still has issues I'm sure.
Node.h
#pragma once
#include
struct Body
{
double posX, posY; //position x and y
double velX, velY; //velocity x and y
double AccelX, AccelY; //force acting on object since last frame
float mass; //mass of object
};
class Node
{
public:
std::vector Bodies;
std::vector Child;
bool HasChildren;
float posX, posY;
float width, height;
float TotalMass;
float CenterOfMassx;
float CenterOfMassy;
unsigned int Depth;
Node();
Node(std::vector pBodies, float pwidth, float pheight, float px = 0, float py = 0, unsigned int pDepth = 0);
~Node();
void GenerateChildren();
void SetParam(std::vector pBodies, float pwidth, float pheight, float px = 0, float py = 0, unsigned int pDepth = 0);
void Reset();
};Node.cpp
```
#include "Node.h"
#include
#define _MAX_DEPTH 100
Node::Node(std::vector pBodies, float pwidth, float pheight, float px, float py, unsigned int pDepth)
{
SetParam(pBodies, pwidth, pheight, px, py, pDepth);
}
void Node::SetParam(std::vector pBodies, float pwidth, float pheight, float px, float py, unsigned int pDepth)
{
HasChildren = false;
Depth = pDepth;
unsigned int size = pBodies.size();
Bodies = pBodies;
posX = px;
posY = py;
width = pwidth;
height = pheight;
float mass = 0;
double Centerx = 0;
double Centery = 0;
for (unsigned int i = 0; i mass;
Centerx += pBodies[i]->posX;
Centery += pBodies[i]->posY;
}
TotalMass = mass;
if (size > 0)
{
CenterOfMassx = static_cast(Centerx / size);
CenterOfMassy = static_cast(Centery / size);
}
else
{
CenterOfMassx = 0;
CenterOfMassy = 0;
}
if (size > 1 && Depth q1Bodies, q2Bodies, q3Bodies, q4Bodies;
for (unsigned int i = 0; i posX posY posY Reset();
delete
Solution
Here are some observations that may help you improve your code.
Refine your objects
The
Only expose the minimally sufficient interface to an object
Rather than exposing all data members, it's better to encapsulate data members and functionality within a class and to only make
Use
Many of the function calls don't alter one or more of their parameters. Unaltered parameters and functions that don't alter the underlying object should be declared
Name things consistently
Using a consistent naming scheme makes it easier to understand your code. I generally like to reserve uppercase words like
Use constructors
The
Use references to pass complex objects
Most of the functions in
Use "range-for" to simplify your code
If your compiler fully supports the C++11 standard, you can use a "range-for" to simplify your code. This is often better, in terms of both readability and performance. If your compiler does not support C++11, I would strongly encourage you to upgrade to one that does.
Order conditionals for performance
The code currently contains this line
Due to short-circuit evaluation, if the first clause is
Note also that rather than comparing to
Use all required
The program uses
Consider using a better random number generator
If you are using a compiler that supports at least C++11, consider using a better random number generator. In particular, instead of
Refine your objects
The
Node and Body objects both have exclusively public members, which is not very good design. Every piece of code that interacts with them then reaches inside and manipulates the object members directly which is prone to error and makes maintenance difficult. Also, the entire simulation should be encapsulated into its own object.Only expose the minimally sufficient interface to an object
Rather than exposing all data members, it's better to encapsulate data members and functionality within a class and to only make
public those functions and data members that are required for the interface.Use
const where practicalMany of the function calls don't alter one or more of their parameters. Unaltered parameters and functions that don't alter the underlying object should be declared
const to make clear what the interface may and may not do. This helps the compiler as well as the human reading the code.Name things consistently
Using a consistent naming scheme makes it easier to understand your code. I generally like to reserve uppercase words like
Node and Body as class names and member functions and data are lowercase. Also, one common scheme is to put a trailing underscore after member data names. That makes is easy to spot which ones are member variables and which are not.Use constructors
The
CreateBody function in main should instead be a constructor for Body. That way you can either create one on the stack or on the heap using new. It also means that any Body will be well-formed.Use references to pass complex objects
Most of the functions in
main which take a std::vector as the first argument should instead be declared to take a reference instead. Doing so allows the compiler to avoid creating a copy of the variable. It is also better than using a raw pointer because unlike a raw pointer, a reference cannot dereference nullptr.Use "range-for" to simplify your code
If your compiler fully supports the C++11 standard, you can use a "range-for" to simplify your code. This is often better, in terms of both readability and performance. If your compiler does not support C++11, I would strongly encourage you to upgrade to one that does.
Order conditionals for performance
The code currently contains this line
if (widthSqr / distanceSqr HasChildren == false)Due to short-circuit evaluation, if the first clause is
true the second does not need to be evaluated. This can be used for performance improvement by ordering the simpler case first:if (!pNode->HasChildren || widthSqr / distanceSqr < NodeThresholdSqr)Note also that rather than comparing to
false, the boolean not operator ! is used. There is likely no performance difference, but it makes the line shorter and easier to read.Use all required
#includesThe program uses
sqrt but does not #include . Carefully check to assure that your program uses all required #includes and that it does not include ones that are not needed.Consider using a better random number generator
If you are using a compiler that supports at least C++11, consider using a better random number generator. In particular, instead of
rand, you might want to look at std::uniform_real_distribution and friends in the ` header.
Use existing C++ libraries where appropriate
The PopulateVectorDisk routine creates a random distance and random angle and then converts those into discrete orthogonal (x and y) components for both the position and velocity of each point. Rather than doing all of that using primitive function calls with sin and cos, it would be much easier to read and more efficient to use the std::complex routines.
Eliminate memory leaks
There are a number of places where new is called without any corresponding delete calls. This is the recipe for a memory leak. Rational use of destructors can help with this if you've already implemented constructors.
Use calls that are in the std namespace
This code calls sqrt, cos and sin but should instead be using std::sqrt, std::cos and std::sin.
Don't overuse static_cast
In a number of places in the code, many instances are used where they are not needed. It's useful to know the numeric promotion rules so that you don't use too many casts. Doing so can clutter up the code and make it hard to read and maintain.
Use simple data types where possible
The Child member of the Node class is a std::vector<> but it really doesn't need to be. It is always ether empty or has four values, so for efficiency, it should be declared as an array of 4 Nodes.
Avoid calculations where practical
The SetParam routine is often called with the same set of Bodys as already exist within the Node` object. Rather than redundantly copying and recalculating the center of mass and total mass, the code can simply check toCode Snippets
if (widthSqr / distanceSqr < NodeThresholdSqr || pNode->HasChildren == false)if (!pNode->HasChildren || widthSqr / distanceSqr < NodeThresholdSqr)#include "SFML/Graphics.hpp"
#include "Node.h"
#include "Sim.h"
//Width and height of simulation, needs to be large,
//particles outside of this range will not be included in the octree
float const SimWidth = 327680;
float const SimHeight = 327680;
void PollEvent(sf::RenderWindow& pTarget, bool &pIsPaused, sf::View &pSimView, float zoom)
{
sf::Event event;
while (pTarget.pollEvent(event))
{
if (event.type == sf::Event::Closed)
pTarget.close();
if (event.type == sf::Event::KeyPressed)
{
if (event.key.code == sf::Keyboard::Space)
pIsPaused = !pIsPaused;
}
if (event.type == sf::Event::MouseWheelScrolled)
{
zoom *= 1 + (static_cast <float> (-event.mouseWheelScroll.delta) / 10);
pSimView.zoom(1 + (static_cast <float> (-event.mouseWheelScroll.delta) / 10));
}
}
if (sf::Mouse::getPosition().x > (1920 - 20))
pSimView.move(2 * zoom, 0);
if (sf::Mouse::getPosition().x < (0 + 20))
pSimView.move(-2 * zoom, 0);
if (sf::Mouse::getPosition().y > (1080 - 20))
pSimView.move(0, 2 * zoom);
if (sf::Mouse::getPosition().y < (0 + 20))
pSimView.move(0, -2 * zoom);
pTarget.setView(pSimView);
}
void SetView(sf::View &pView, sf::RenderWindow& pTarget, float pViewWidth, float pViewHeight)
{
pView.reset(sf::FloatRect(SimWidth / 2 - pViewWidth / 2, SimHeight / 2 - pViewHeight / 2, pViewWidth, pViewHeight));
pView.setViewport(sf::FloatRect(0.f, 0.f, 1.f, 1.f));
pTarget.setView(pView);
}
int main()
{
float const ViewWidth = 1920;
float const ViewHeight = 1080;
float const DiskRadiusMax = 20000;
float const DiskRadiusMin = 50;
float const GalacticCenterMass = 1000000;
float const ObjectMassMax = 2;
float const ObjectMassMin = 1;
unsigned int const NumParticles = 10000;
float zoom = 1;
bool IsPaused = false;
sf::RenderWindow window(sf::VideoMode(1920, 1080), "N-Body simulation");
sf::View SimulationView;
Sim sim(NumParticles, SimWidth, SimHeight, DiskRadiusMin,
DiskRadiusMax, ObjectMassMin, ObjectMassMax, GalacticCenterMass);
SetView(SimulationView, window, ViewWidth, ViewHeight);
while (window.isOpen())
{
PollEvent(window, IsPaused, SimulationView, zoom);
if (!IsPaused)
++sim;
sim.Render(window, zoom);
}
}#ifndef SIM_H
#define SIM_H
#include "SFML/Graphics.hpp"
#include "Node.h"
#include <vector>
class Sim {
public:
Sim(unsigned int pParticlesCount, float pWidth, float pHeight, float pMaxDist, float pMinDist, float pMinMass, float pMaxMass, float pGalaticCenterMass);
Sim &operator++() {
attractToCenter();
updateBodies();
globalNode_.setParam(bodies_, simWidth, simHeight);
octreeBodyAttraction();
return *this;
}
void Render(sf::RenderWindow& pTarget, float zoom) const;
~Sim();
private:
void attractToCenter() {
for (auto &pB : bodies_)
pB->calculateAccel(center_);
}
void updateBodies() {
for (auto &pB : bodies_) {
pB->update(simWidth, simHeight);
pB->resetAccel();
}
}
void octreeBodyAttraction() {
for (auto &pB : bodies_)
globalNode_.checkNode(*pB);
}
float simWidth, simHeight, galacticCenterMass;
Body center_;
std::vector<Body*> bodies_;
Node globalNode_;
};
#endif // SIM_H#pragma once
#include <vector>
#include <cmath>
extern const float _GRAV_CONST;
class Node;
class Body
{
public:
Body(float px, float py, float pmass, float pvx=0, float pvy=0) :
posX_(px),
posY_(py),
velX_(pvx),
velY_(pvy),
accelX_(0),
accelY_(0),
mass_(pmass)
{}
// updates accel based on other body
void calculateAccel(const Body& bj);
// updates accel based on other node
void calculateAccel(const Node& bj);
void resetAccel() { accelX_ = accelY_ = 0; }
void update(float SimWidth, float SimHeight);
float dist2(const Node& bj) const;
float mass() const { return mass_; }
float posX() const { return posX_; }
float posY() const { return posY_; }
float accel() const { return std::sqrt(accelX_*accelX_ + accelY_*accelY_); }
private:
void calculateAccel(double x, double y, double mass);
float posX_, posY_; //position x and y
float velX_, velY_; //velocity x and y
double accelX_, accelY_; //accel acting on object since last frame
float mass_; //mass of object
};
class Node
{
public:
Node() :
child_{nullptr, nullptr, nullptr, nullptr},
depth_{0},
hasChildren_{false}
{}
Node(unsigned int pDepth) :
child_{nullptr, nullptr, nullptr, nullptr},
depth_{pDepth},
hasChildren_{false}
{}
void setParam(const std::vector<Body*> &pBodies, float pwidth, float pheight, float px = 0, float py = 0);
bool hasChildren() const { return hasChildren_; }
~Node() {
hasChildren_ = false;
delete child_[0];
delete child_[1];
delete child_[2];
delete child_[3];
child_[0] = nullptr;
child_[1] = nullptr;
child_[2] = nullptr;
child_[3] = nullptr;
}
void checkNode(Body& pBody);
friend class Body;
private:
void generateChildren();
Node* child_[4];
float posX_, posY_;
float width_, height_;
std::vector<Body*> bodies_;
float totalMass_;
float centerOfMassx_;
float centerOfMassy_;
unsigned int depth_;
bool isUsed_; //For testing, delete this later
bool hasChildren_;
};Context
StackExchange Code Review Q#96050, answer score: 5
Revisions (0)
No revisions yet.