function animateSky() { //animate the sky function drawground() { //draw the ground function drawpyr1() { //draw pyramid1 function animateSky() { //animate the sky function drawground() { //draw the ground function drawpyr1() { //draw pyramid1 function animateSky() { //animate the sky function drawground() { //draw the ground function drawpyr1() { //draw pyramid1 Rewrite in ES6
Similarly to the previous remark, youYou mix single (' ') and double (" ") quotation marks. Use single ones as they minimize cognitive load, unless that would force you to use character escaping.
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 drawPyramide1drawPyramid1.
drawsun() don'tdoesn't need the parameters
The following parameters could be hardcoded into function they are passed too.:
// jshint esnext: true // noprotect 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> Similarly to the previous remark, you mix single (' ') and double (" ") quotation marks.
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 drawPyramide1.
drawsun() don't need the parameters
The following parameters could be hardcoded into function they are passed too.
// jshint esnext: true // noprotect 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> You mix single (' ') and double (" ") quotation marks. Use single ones as they minimize cognitive load, unless that would force you to use character escaping.
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.
drawsun() doesn't need the parameters
The following parameters could be hardcoded into function they are passed too:
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>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
Similarly to the previous remark, you mix single (' ') and double (" ") quotation marks.
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 drawPyramide1.
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() don'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.
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.
// jshint esnext: true // noprotect 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>default