5
\$\begingroup\$

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>

Jamal
35.2k13 gold badges134 silver badges238 bronze badges
asked Apr 14, 2017 at 23:21
\$\endgroup\$
1
  • \$\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\$ Commented Apr 15, 2017 at 19:03

2 Answers 2

3
\$\begingroup\$

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;
Mathieu Guindon
75.5k18 gold badges194 silver badges467 bronze badges
answered Apr 15, 2017 at 4:50
\$\endgroup\$
6
  • \$\begingroup\$ I'm not sure about the last bullet, but +1 for the other points :-) \$\endgroup\$ Commented Apr 15, 2017 at 4:52
  • \$\begingroup\$ Thanks @mat's mug, can you tell me why you are not sure? :) \$\endgroup\$ Commented 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\$ Commented 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\$ Commented 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\$ Commented Apr 15, 2017 at 5:16
2
\$\begingroup\$

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>

answered Apr 15, 2017 at 14:38
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.