I'm new to JavaScript, and wrote this little animation in canvas. I'm wondering if anyone has any suggestions as for how to improve it.
When running the code, the JavaScript draws a scene of three pyramids, a sun, and a sky. The sun and gradient of the sky animate over time.
The code is working perfectly, but I would like to improve it and make it more efficient.
//render the canvas at 2x dpi
var canvas = document.getElementById('myCanvas');
canvas.width = 1920;
canvas.height = 816;
canvas.style.width = "960px";
canvas.style.height = "408px";
//rescale the grid system for 2xdpi
var ctx = canvas.getContext('2d');
ctx.scale(2, 2);
//start drawing the scene
var canvasElement = document.querySelector("#myCanvas");
var ctx = canvasElement.getContext("2d");
var skyY = 100;
var skyYDirection = 1;
function drawsky() {
//generate the sky gradient
var sky = ctx.createLinearGradient(0, skyY, 0, 0);
sky.addColorStop(0, "#a7bde0");
sky.addColorStop(1, "#3c68b1");
//draw the sky
ctx.fillStyle = sky;
ctx.fillRect(0, 0, 960, 408);
}
function animateSky() {
//animate the sky
skyY += skyYDirection;
}
function drawground() {
//draw the ground
ctx.fillStyle = "#e9bf83";
ctx.fillRect(0, 297, 960, 111);
}
function drawpyr1() {
//draw pyramid1
//walla
ctx.beginPath();
ctx.moveTo(516, 297);
ctx.lineTo(595, 297);
ctx.lineTo(632, 182);
ctx.closePath();
//fill
ctx.fillStyle = "#3b230b";
ctx.fill();
//wallb
ctx.beginPath();
ctx.moveTo(595, 297);
ctx.lineTo(764, 297);
ctx.lineTo(632, 182);
ctx.closePath();
//fill
ctx.fillStyle = "#d49c5f";
ctx.fill();
}
function drawpyr2() {
//draw pyramid2
//walla
ctx.beginPath();
ctx.moveTo(322, 297);
ctx.lineTo(410, 297);
ctx.lineTo(497, 100);
ctx.closePath();
//fill
ctx.fillStyle = "#3b230b";
ctx.fill();
//wallb
ctx.beginPath();
ctx.moveTo(410, 297);
ctx.lineTo(695, 297);
ctx.lineTo(497, 100);
ctx.closePath();
//fill
ctx.fillStyle = "#d49c5f";
ctx.fill();
}
function drawpyr3() {
//draw pyramid3
//walla
ctx.beginPath();
ctx.moveTo(179, 297);
ctx.lineTo(245, 297);
ctx.lineTo(324, 122);
ctx.closePath();
//fill
ctx.fillStyle = "#3b230b";
ctx.fill();
//wallb
ctx.beginPath();
ctx.moveTo(245, 297);
ctx.lineTo(541, 297);
ctx.lineTo(324, 122);
ctx.closePath();
//fill
ctx.fillStyle = "#d49c5f";
ctx.fill();
}
function clearCanvas() {
//clear the canvas
ctx.clearRect(0, 0, 960, 408);
}
function drawsun(x, y) {
//draw the sun
ctx.beginPath();
ctx.arc(x, y, 80, 0, 2 * Math.PI, false);
ctx.fillStyle = "#fff1dc";
ctx.fill();
}
var sunX = 700;
var sunY = 100;
var xDirection = -1;
var yDirection = 0.2;
function animateSun() {
//animate the sun
sunX += xDirection;
sunY += yDirection;
}
//render the scene
function render() {
//clear canvas
clearCanvas();
//layer / animate objs
drawsky();
animateSky();
drawsun(sunX, sunY);
animateSun();
drawground();
drawpyr1();
drawpyr2();
drawpyr3();
//loop the render process
window.requestAnimationFrame(render);
}
render();
/*nightfall*/
@keyframes nightfall {
from {
filter: brightness(1);
}
to {
filter: brightness(0);
}
}
canvas {
animation: nightfall 20s forwards cubic-bezier(0.730, 0.010, 1.000, 0.300);
}
<html>
<body>
<canvas id="myCanvas"></canvas>
<script src="js/script.js" type="text/javascript"></script>
</body>
</html>
-
\$\begingroup\$ Another performance related improvement: Draw static scenery such as the background or pyramids on its own canvas(es) or provide them as (svg) images and overlay them with another canvas on which you only render the dynamically changing parts of your animation. \$\endgroup\$le_m– le_m2017年04月15日 19:03:57 +00:00Commented Apr 15, 2017 at 19:03
2 Answers 2
Just some random tips in no particular order.
- Consistency in naming: keep using camelCase.
(
drawsun -> drawSun
,drawpyrX -> drawPyrX
,walla -> wallA
, ...) - Redundance in declaring and inconsistent use of
"
and'
.
var canvas = document.getElementById('myCanvas'); var ctx = canvas.getContext('2d'); var canvasElement = document.querySelector("#myCanvas"); var ctx = canvasElement.getContext("2d");
- Put all variables to the top and use
const
for the ones you dont want to change. This makes changes easier and safer, because you cant change the constants by accident.
var sunX = 700; var sunY = 100; const xDirection = -1; const yDirection = 0.2;
-
\$\begingroup\$ I'm not sure about the last bullet, but +1 for the other points :-) \$\endgroup\$Mathieu Guindon– Mathieu Guindon2017年04月15日 04:52:31 +00:00Commented Apr 15, 2017 at 4:52
-
\$\begingroup\$ Thanks @mat's mug, can you tell me why you are not sure? :) \$\endgroup\$Tweakimp– Tweakimp2017年04月15日 04:55:46 +00:00Commented Apr 15, 2017 at 4:55
-
1\$\begingroup\$ I don't do JavaScript, but it's typically advised to declare variables as close as possible to their usage. Yet the
const
point is a good one too, so yeah the third bullet makes a good point as well - it's just the bit about putting all variables at to the top I'm not sure about. Nice answer! \$\endgroup\$Mathieu Guindon– Mathieu Guindon2017年04月15日 04:58:29 +00:00Commented Apr 15, 2017 at 4:58 -
\$\begingroup\$ I dont know what the best practice is, im new and derived this rule by myself. In my opinion it just makes sense to keep them all in on place to know where to look for. All global variables on top of the script and all other ones as high as possible, eg define all variables at the start of a function. You could even use multivars (var a=1,b=3,c=2;) if you like this style. Where do you see downsides in keeping variables as high as possible? \$\endgroup\$Tweakimp– Tweakimp2017年04月15日 05:05:40 +00:00Commented Apr 15, 2017 at 5:05
-
\$\begingroup\$ Constantly having to scroll up and down the code, context-switching all the time... code reads (and debugs, and refactors) much more fluently when related things are together, like a variable and its usages. It's easier to extract a method when the scope starts to get large. \$\endgroup\$Mathieu Guindon– Mathieu Guindon2017年04月15日 05:16:01 +00:00Commented Apr 15, 2017 at 5:16
Unnecessary attribute addition
Those two lines
canvas.width = 1920;
canvas.height = 816;
could be completely removed — width
and height
can be declared in HTML as <cavas>
's attributes.
Hardcoded numbers
It would be better to rewrite
canvas.style.width = "960px";
canvas.style.height = "408px";
as
canvas.style.width = parseInt(canvas.width / 2) + 'px';
canvas.style.height = parseInt(canvas.height / 2) + 'px';
That way you wouldn't have to remember to change all those lines each time you change canvas
's width
and height
.
The same goes for
ctx.clearRect(0, 0, 960, 408);
Quotation marks
You mix single (' '
) and double (" "
) quotation marks. Use single ones as they minimize cognitive load, unless that would force you to use character escaping.
Two canvas
elements
You have two variables referring to the very same <canvas>
:
var canvas = document.getElementById('myCanvas');
var canvasElement = document.querySelector("#myCanvas");
Interestingly, they are referred to using two different methods. In this case the first one is better.
Context redefinition
You define variable ctx
twice:
var ctx = canvas.getContext('2d');
var ctx = canvasElement.getContext("2d");
The second definition is completely unnecessary.
Hardcoded colors
You have total of 6 unique colors referenced in your code, two of which have 3 occurrences. It means that wanting to change some color, you have to not only look for it throughout your entire code, but you also may have to search for all it's occurrences. It would be far better to create enum-like object, like this:
const COLOR = {
LIGHT_BLUE: '#a7bde0', // 1st sky color
DARK_BLUE: '#3c68b1', // 2nd sky color
SAND: '#e9bf83', // Ground color
DARK_BROWN: '#3b230b', // 1st pyramid wall color
LIGHT_BROWN: '#d49c5f', // 2nd pyramid wall color
SUN: '#fff1dc' // Sun color
};
and use it later by writing e.g. COLOR.DARK_BLUE
.
Naming
You mix camelCase (e.g. animateSky
) with all lowercase names (e.g. drawsky
). Also, I think that some names could be better, e.g. drawpyr1
could become drawPyramid1
.
Comments
Some comments are not that helpful:
function animateSky() {
//animate the sky
function drawground() {
//draw the ground
function drawpyr1() {
//draw pyramid1
And other are overly cryptic, e.g. //walla
instead of // 1st wall
. Also, it's usually considered good style, to put space after //
and — arguably —
begin comments with uppercase letter.
Use ES6 (perhaps)
While just a suggestion, because you may want not to do it due to the browser support, you could simplify your code by using ES6.
drawsun()
doesn't need the parameters
The following parameters could be hardcoded into function they are passed too:
drawsun(sunX, sunY);
Generalize drawing pyramids and their walls
The only difference between drawpyr1()
, drawpyr2()
, drawpyr3()
are coordinates. You could create an array of pyramids, with each pyramid (an array too, because why not) containing two arrays for each of two their walls. And walls would be arrays too ― arrays of arrays of two integers representing coordinates of points that the said wall consists of. But the talk is cheap, so here is the code:
const pyramids =
[
[ // 1st pyramid
[ // 1st wall
[516, 297],
[595, 297],
[632, 182]
],
[ // 2nd wall
[595, 297],
[764, 297],
[632, 182]
]
],
[ // 2nd pyramid
[ // 1st wall
[322, 297],
[410, 297],
[497, 100]
],
[ // 2nd wall
[410, 297],
[695, 297],
[497, 100]
]
],
[ // 3rd pyramid
[ // 1st wall
[179, 297],
[245, 297],
[324, 122]
],
[ // 2nd wall
[245, 297],
[541, 297],
[324, 122]
]
],
];
An important thing to note: many people would say that it needs to be broken out, e.g. by creating Wall
object and 3 it's instances. While generally true, in this case I find this code fine.
OK, now that we have a function to create pyramid, we could break it even further since it consists of repeating code as well. Each of those repetitions could simply be a separate functions to create a wall, since the only difference between them, besides coordinates is wall color. This is how I would break them:
const _drawPyramidWall = (pyramidIndex, wallIndex) => {
const pyramidWallPoints = pyramids[pyramidIndex][wallIndex]; // All points of current wall
// Draw
ctx.beginPath();
ctx.moveTo(...pyramidWallPoints[0]);
for (let i = 1; i < pyramidWallPoints.length; i++) { // For all wall points except first
ctx.lineTo(...pyramidWallPoints[i]);
}
ctx.closePath();
// Fill
ctx.fillStyle = wallIndex === 0 ? COLOR.DARK_BROWN : COLOR.LIGHT_BROWN;
ctx.fill();
};
const drawPyramid = (pyramidIndex) => {
// 1st wall
_drawPyramidWall(pyramidIndex, 0);
// 2nd wall
_drawPyramidWall(pyramidIndex, 1);
};
Round coordinates
In this line
ctx.arc(x, y, 80, 0, 2 * Math.PI, false);
y
is a floating point number, which will cause unwanted subpixel antialiasing. To prevent this use efficient (and a bit hackery) method of rounding: (0.5 + number) << 0
. Note, that it may cause small visible sun jumps.
End animation after 20 seconds
Your animation fades to black after 20 seconds, but it's still happening. It would be more efficient for the rest of webpage to stop it after that time.
Rewrite in ES6
Here comes rewrote code in ES6. Note, that there are still some repeating hardcoded magic numbers and few more things to fix. I also did not changed your logic of drawing, just rewrote the code.
const canvas = document.getElementById('canvas'),
ctx = canvas.getContext('2d'),
canvasWidth = parseInt(canvas.width / 2),
canvasHeight = parseInt(canvas.height / 2),
startTime = Date.now(),
twoPI = 2 * Math.PI;
// Rescale the grid system for 2x dpi
canvas.style.width = canvasWidth + 'px';
canvas.style.height = canvasHeight + 'px';
ctx.scale(2, 2);
const settings = {
endTime: startTime + 20000, // Epoch time to end animation
skyYDirection: 1,
skyY: 100,
sunX: 700,
sunY: 100,
sunXDirection: -1,
sunYDirection: 0.2
};
const COLOR = {
LIGHT_BLUE: '#a7bde0', // 1st sky color
DARK_BLUE: '#3c68b1', // 2nd sky color
SAND: '#e9bf83', // Ground color
DARK_BROWN: '#3b230b', // 1st pyramid wall color
LIGHT_BROWN: '#d49c5f', // 2nd pyramid wall color
SUN: '#fff1dc' // Sun color
};
const drawSky = () => {
// Sky gradient
var sky = ctx.createLinearGradient(0, settings.skyY, 0, 0);
sky.addColorStop(0, COLOR.LIGHT_BLUE);
sky.addColorStop(1, COLOR.DARK_BLUE);
// Drawing
ctx.fillStyle = sky;
ctx.fillRect(0, 0, canvasWidth, canvasHeight);
};
const animateSky = () => settings.skyY += settings.skyYDirection;
const drawSun = () => {
ctx.beginPath();
ctx.arc(settings.sunX, settings.sunY, 80, 0, twoPI, false);
ctx.closePath();
ctx.fillStyle = COLOR.SUN;
ctx.fill();
};
const animateSun = () => {
settings.sunX += settings.sunXDirection;
settings.sunY += settings.sunYDirection;
};
const drawGround = () => {
ctx.fillStyle = COLOR.SAND;
ctx.fillRect(0, 297, canvasWidth, 111);
};
const pyramids =
[
[ // 1st pyramid
[ // 1st wall
[516, 297],
[595, 297],
[632, 182]
],
[ // 2nd wall
[595, 297],
[764, 297],
[632, 182]
]
],
[ // 2nd pyramid
[ // 1st wall
[322, 297],
[410, 297],
[497, 100]
],
[ // 2nd wall
[410, 297],
[695, 297],
[497, 100]
]
],
[ // 3rd pyramid
[ // 1st wall
[179, 297],
[245, 297],
[324, 122]
],
[ // 2nd wall
[245, 297],
[541, 297],
[324, 122]
]
],
];
const _drawPyramidWall = (pyramidIndex, wallIndex) => {
const pyramidWallPoints = pyramids[pyramidIndex][wallIndex]; // All points of current wall
// Draw
ctx.beginPath();
ctx.moveTo(...pyramidWallPoints[0]);
for (let i = 1; i < pyramidWallPoints.length; i++) { // For all wall points except first
ctx.lineTo(...pyramidWallPoints[i]);
}
ctx.closePath();
// Fill
ctx.fillStyle = wallIndex === 0 ? COLOR.DARK_BROWN : COLOR.LIGHT_BROWN;
ctx.fill();
};
const drawPyramid = (pyramidIndex) => {
// 1st wall
_drawPyramidWall(pyramidIndex, 0);
// 2nd wall
_drawPyramidWall(pyramidIndex, 1);
};
const clearCanvas = () => ctx.clearRect(0, 0, canvasWidth, canvasHeight);
const render = () => {
clearCanvas();
// Draw and animate objects
drawSky();
animateSky();
drawSun();
animateSun();
drawGround();
drawPyramid(0);
drawPyramid(1);
drawPyramid(2);
// Main loop
if (Date.now() < settings.endTime) { // Animation time limit enforcement
window.requestAnimationFrame(render);
}
};
// Start animation
render();
/* nightfall */
@keyframes nightfall {
from {
filter: brightness(1);
}
to {
filter: brightness(0);
}
}
canvas {
animation: nightfall 20s forwards cubic-bezier(0.730, 0.010, 1.000, 0.300);
}
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
<meta name="viewport" content="width=device-width">
<title>Pyramid animation</title>
</head>
<body>
<canvas id="canvas" height="816" width="1920"></canvas>
</body>
</html>