patternjavascriptMinor
2D Gravity Simulator-like game
Viewed 0 times
simulatorgamegravitylike
Problem
This project is pretty much like a 2D gravity simulator. You click and it creates a circle that attracts other circles. The result of a collision is a bigger circle that is a sum of the masses. If you don't know what I'm talking about, here's a gif:
The game was made using CreateJS, but you don't need to know CreateJS to understand the code.
My biggest concern is performance, but I care a lot for code clarity too.
The scene starts with some pre-made planets, and there's some commented-out code that creates different setups for testing. It may be interesting to review functions inside the
Engine.js:
```
var SDU = SDU || {};
SDU.GameScene = function(canvas) {
this.canvas = document.getElementById(canvas);
this.SDUScene_constructor(canvas);
this.bodies = [];
this.collisions = [];
this.garbage = [];
this.palette = {
dark: ["#FF4650", "#FFD939", "#97FB32", "#32CEF4", "#FE60D6"],
light: ["#CF7758", "#9FB6A3", "#A878A6", "#7D8FA6", "#B04C56", "#3D4754", "#3D4754"]
};
SDU.GameScene.self = this;
this.setup();
this.listen();
};
createjs.extend(SDU.GameScene, SDU.Scene);
SDU.GameScene.prototype.setup = function() {
/*for (var i1 = 0; i1 utils.sqr(obj1.getRadius() + obj2.getRadius())) {
dist = Math.sqrt(distSquare);
if (obj1.affectedByGravity) {
var force1 = obj2.getMass() / distSquare * obj2.attraction;
obj1.vx += force1 * diffX / dist;
obj1.vy += force1 * diffY / dist;
}
if (obj2.affectedByGravity) {
var force2 = obj1.getMass() / distSquare * obj1.attraction;
obj2.vx -= force2 * diffX / dist;
obj2.vy -= force2 * diffY / dist;
The game was made using CreateJS, but you don't need to know CreateJS to understand the code.
My biggest concern is performance, but I care a lot for code clarity too.
The scene starts with some pre-made planets, and there's some commented-out code that creates different setups for testing. It may be interesting to review functions inside the
Utils.js file, principally those related to the utils.distToSegmentSquared() function, which I've got from StackOverflow and is dreadfully expensive.Engine.js:
```
var SDU = SDU || {};
SDU.GameScene = function(canvas) {
this.canvas = document.getElementById(canvas);
this.SDUScene_constructor(canvas);
this.bodies = [];
this.collisions = [];
this.garbage = [];
this.palette = {
dark: ["#FF4650", "#FFD939", "#97FB32", "#32CEF4", "#FE60D6"],
light: ["#CF7758", "#9FB6A3", "#A878A6", "#7D8FA6", "#B04C56", "#3D4754", "#3D4754"]
};
SDU.GameScene.self = this;
this.setup();
this.listen();
};
createjs.extend(SDU.GameScene, SDU.Scene);
SDU.GameScene.prototype.setup = function() {
/*for (var i1 = 0; i1 utils.sqr(obj1.getRadius() + obj2.getRadius())) {
dist = Math.sqrt(distSquare);
if (obj1.affectedByGravity) {
var force1 = obj2.getMass() / distSquare * obj2.attraction;
obj1.vx += force1 * diffX / dist;
obj1.vy += force1 * diffY / dist;
}
if (obj2.affectedByGravity) {
var force2 = obj1.getMass() / distSquare * obj1.attraction;
obj2.vx -= force2 * diffX / dist;
obj2.vy -= force2 * diffY / dist;
Solution
and there's some commented-out code that creates different setups for testing
Automate this! Or better yet, create a default config that you can tweak instead of adding/removing code.
-
As far as I understand, this just runs at setup, which shouldn't be affecting performance. However, you could substitute your nested loop into just one loop that does
-
You're comparing with
-
-
The
-
I would recommend a
-
Haven't read through your code for the calculations (nose is bleeding with math). But your loop for appears to be
For instance, [1, 2, 3]. Your loop runs like
-
I also notice a
-
Same as above, don't modify things you don't own.
-
You might want to use a pre-build UA detection library like https://github.com/faisalman/ua-parser-js
Automate this! Or better yet, create a default config that you can tweak instead of adding/removing code.
/*for (var i1 = 0; i1 < 10; ++i1) {
for (var i2 = 0; i2 < 10; ++i2) {
var planet = new SDU.Planet(50 + i1 * 25, 50 + i2 * 25, 5, this.palette.light.getRandomItem(), {
density: 1,
attraction: 1
}, this);
}
}*/-
As far as I understand, this just runs at setup, which shouldn't be affecting performance. However, you could substitute your nested loop into just one loop that does
i1 x i2 iterations.-
You're comparing with
10. Better move that to a config object or some "constant" (just a variable acting like a constant) for you to easily configure this value.-
50 + i1 25, 50 + i2 25 - they don't seem to mean anything. What is this equation exactly? If at all possible, move this to a function which you can just call. And while you're there, name it according to what it does.//var p1 = new SDU.Planet(50, 50, 5, this.palette.getRandomItem(), {}, this);
//var p2 = new SDU.Planet(100, 50, 10, this.palette.getRandomItem(), {}, this);- Same here, what are these numbers exactly? Move to config or variable that names their purpose.
var _this = this,
preview;-
The
_ has a special meaning in programming. It implies that the thing with _ is private. Since this is not a private variable, nor are you exposing anything public in the function, then don't use the _. Common names include instance, that, and self. I prefer instance though, as it implies that we're in a method, the function should be used as a method, and that this is an object instance and not some randomly assigned context.-
I would recommend a
var per variable. I looked at stagemousedown and found preview with no var. It was hard to spot where it was defined because it didn't have var when it was declared.function _listenMouseMove() {
if (_this.mouse.down) {}
}- Dead code. :P
for (i1 = 0; i1 < this.bodies.length; ++i1) {
obj1 = this.bodies[i1];
for (i2 = i1 + 1; i2 < this.bodies.length; ++i2) {
obj2 = this.bodies[i2];-
Haven't read through your code for the calculations (nose is bleeding with math). But your loop for appears to be
O(n^2). Appears to be redundant.For instance, [1, 2, 3]. Your loop runs like
[[1,1],[1,2],[1,3],[2,1],[2,2],[2,3],[3,1],[3,2],[3,3]]. If you could find a way, remove the redundant comparisons and the loop would be something like [[1,2],[1,3],[2,3]]. It's a great reduction in iterations.-
I also notice a
while inside there. That complicates things. As much as possible, try avoiding nested loops.this.collisions = [];- If you're clearing the
collisionsarray, just set the length to zero (this.collisions.length = 0) instead of assigning a new array. It avoids creating too many objects.
radius * 2
return this.getArea() * this.density;
return Math.PI * this.radius * this.radius;- As mentioned in a comment, this appears constant most of the time. Might want to cache these values and just recalculate them when any dependents change. Saves you from computing them all the time.
return Math.floor(Math.random() * (max - min + 1)) + min;- You can use
| 0to clip off the decimals from a number, instead ofMath.floor. In some engines, bitwise operations are faster thanMathoperations. But mileage may vary.
Array.prototype.getRandomItem = function() {
var i = utils.getRandomInt(0, this.length - 1);
return this[i];
};- Don't modify the prototype. In general, don't modify objects you don't own. Create your own function that accepts an array and returns a random item instead.
navigator.sayswho = (function() {
var ua = navigator.userAgent,
tem,
M = ua.match(/(opera|chrome|safari|firefox|msie|trident(?=\/))\/?\s*(\d+)/i) || [];
if (/trident/i.test(M[1])) {
tem = /\brv[ :]+(\d+)/g.exec(ua) || [];
return 'IE ' + (tem[1] || '');
}
if (M[1] === 'Chrome') {
tem = ua.match(/\b(OPR|Edge)\/(\d+)/);
if (tem != null) return tem.slice(1).join(' ').replace('OPR', 'Opera');
}
M = M[2] ? [M[1], M[2]] : [navigator.appName, navigator.appVersion, '-?'];
if ((tem = ua.match(/version\/(\d+)/i)) != null) M.splice(1, 1, tem[1]);
return M.join(' ');
})();-
Same as above, don't modify things you don't own.
-
You might want to use a pre-build UA detection library like https://github.com/faisalman/ua-parser-js
Code Snippets
/*for (var i1 = 0; i1 < 10; ++i1) {
for (var i2 = 0; i2 < 10; ++i2) {
var planet = new SDU.Planet(50 + i1 * 25, 50 + i2 * 25, 5, this.palette.light.getRandomItem(), {
density: 1,
attraction: 1
}, this);
}
}*///var p1 = new SDU.Planet(50, 50, 5, this.palette.getRandomItem(), {}, this);
//var p2 = new SDU.Planet(100, 50, 10, this.palette.getRandomItem(), {}, this);var _this = this,
preview;function _listenMouseMove() {
if (_this.mouse.down) {}
}for (i1 = 0; i1 < this.bodies.length; ++i1) {
obj1 = this.bodies[i1];
for (i2 = i1 + 1; i2 < this.bodies.length; ++i2) {
obj2 = this.bodies[i2];Context
StackExchange Code Review Q#104387, answer score: 3
Revisions (0)
No revisions yet.