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>

Improve this question
\$\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\$
    – le_m
    Apr 15, 2017 at 19:03

2 Answers 2

Reset to default
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;
Improve this answer
\$\endgroup\$
6
  • \$\begingroup\$ I'm not sure about the last bullet, but +1 for the other points :-) \$\endgroup\$
    – Mathieu Guindon
    Apr 15, 2017 at 4:52
  • \$\begingroup\$ Thanks @mat's mug, can you tell me why you are not sure? :) \$\endgroup\$
    – Tweakimp
    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
    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
    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
    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>

Improve this answer
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.