3
\$\begingroup\$

I am making a 2d graphics engine. I am somewhat new to JavaScript and have been winging this project. So far it creates a background (a tilemap made of smaller images in a grid) and sprites ( a small image with a higher z index). I ommited my test code, as I didn't care for that to be reviewed, but if it needs to be included I can add it.

I am looking for best practice advice for javascript or in general. One specific question is: Are the OLNs (sprites and tilemap) used correctly or should something else be used?

js:

'use strict'; const TILEMAPSIZE = 160; // Total number of tiles displayed. Must be a multiple of TILECOLUMNS. const TILECOLUMNS = 16; // Number of columns let tileSet = []; // Set of all tiles to be used let tilemapList = []; // The order in which the tiles are used via references to elements in tileSet let spriteSet = []; // Set of all sprites to be used loadResources(); let sprites = (function (){ function createSprites(spritesList) { let spriteCount = 0; spritesList.forEach(x => { let sprite = document.createElement('img'); sprite.src = x; sprite.className = "sprites"; sprite.id = "sprite" + spriteCount; sprite.style.visibility = "hidden"; ++spriteCount; document.getElementById("window").appendChild(sprite); }); } function show(index) { const spriteStyle = document.getElementById("sprite" + index).style spriteStyle.visibility = "visible"; } function hide(index) { spriteStyle = document.getElementById("sprite" + index).style spriteStyle.visibility = "hidden"; } function reflectY(spriteIndex) { const sprite = "sprite" + spriteIndex; const transform = document.getElementById(sprite).style.transform; if (transform === "scale(-1, 1)"){ document.getElementById(sprite).style.transform = "scale(1)"; } else if (transform === "scale(1, -1)"){ document.getElementById(sprite).style.transform = "scale(-1)"; } else if (transform === "scale(-1)"){ document.getElementById(sprite).style.transform = "scale(1, -1)"; } else { document.getElementById(sprite).style.transform = "scale(-1, 1)"; } } function reflectX(spriteIndex) { const sprite = "sprite" + spriteIndex; const transform = document.getElementById(sprite).style.transform; if (transform === "scale(-1, 1)"){ document.getElementById(sprite).style.transform = "scale(-1)"; } else if (transform === "scale(1, -1)"){ document.getElementById(sprite).style.transform = "scale(1)"; } else if (transform === "scale(-1)"){ document.getElementById(sprite).style.transform = "scale(-1, 1)"; } else { document.getElementById(sprite).style.transform = "scale(1, -1)"; } } function reflect(spriteIndex, dX, dY) { const sprite = "sprite" + spriteIndex; if (dX === true && dY === true){ document.getElementById(sprite).style.transform = "scale(1)"; } else if (dX === true && dY === false){ document.getElementById(sprite).style.transform = "scale(1, -1)"; } else if (dX === false && dY === true){ document.getElementById(sprite).style.transform = "scale(-1, 1)"; } else if (dX === false && dY === false){ document.getElementById(sprite).style.transform = "scale(-1)"; } } function translate(spriteIndex, dX, dY) { const sprite = "sprite" + spriteIndex; const posLeft = parseInt(document.getElementById(sprite).style.left.slice(0, -2)) + dX; const posTop = parseInt(document.getElementById(sprite).style.top.slice(0, -2)) + dY; document.getElementById(sprite).style.left = posLeft + "px"; document.getElementById(sprite).style.top = posTop + "px"; } function setPosition(spriteIndex, dX, dY) { const sprite = "sprite" + spriteIndex; document.getElementById(sprite).style.left = dX +"px"; document.getElementById(sprite).style.top = dY + "px"; } function getPosition(spriteIndex) { const sprite = "sprite" + spriteIndex; return { x: parseInt(document.getElementById(sprite).style.left.slice(0, -2)), y: parseInt(document.getElementById(sprite).style.top.slice(0, -2)) }; } function getOrientation(spriteIndex) { const sprite = "sprite" + spriteIndex; const transform = document.getElementById(sprite).style.transform; let pair = {}; if (transform === "scale(1)"){ pair = {x: 1, y: 1}; } else if (transform === "scale(-1)"){ pair = {x: -1, y: -1}; } else if (transform === "scale(1, -1)"){ pair = {x: 1, y: -1}; } else if (transform === "scale(-1, 1)"){ pair = {x: -1, y: 1}; } else { pair = {x: 0, y: 0}; } return pair; } return{ setPosition: setPosition, getPosition: getPosition, getOrientation: getOrientation, translate: translate, reflect: reflect, reflectX: reflectX, reflectY: reflectY, createSprites: createSprites, show: show, hide: hide }; }()) let tilemap = (function (){ function create(){ // create the tile map for (let i = 0; i < TILEMAPSIZE; ++i){ let tile = document.createElement('div'); tile.className = "tile"; tile.id = "tile" + i; document.getElementById("tilemap").appendChild(tile); }; // set the number of columns let value = ""; for (let i = 0; i < TILECOLUMNS; ++i){ value += "auto "; } document.getElementById("tilemap").style.gridTemplateColumns = value; // insert images let index = 0; tilemapList.forEach(x => { let img = document.createElement('img'); img.src = x; img.className = "tileImage"; document.getElementById("tile" + index).appendChild(img); ++index; }) } function show() { spriteStyle = document.getElementById("tilemap").style spriteStyle.visibility = "visible"; } function hide() { spriteStyle = document.getElementById("tilemap").style spriteStyle.visibility = "hidden"; } function translate(dX, dY) { const posLeft = parseInt(document.getElementById("viewport").style.left.slice(0, -2)) + dX; const posTop = parseInt(document.getElementById("viewport").style.top.slice(0, -2)) + dY; document.getElementById("viewport").style.left = posLeft + "px"; document.getElementById("viewport").style.top = posTop + "px"; } function setPosition(dX, dY) { document.getElementById("viewport").style.top = dY + "px"; document.getElementById("viewport").style.left = dX + "px"; } function getPosition() { return { x: parseInt(document.getElementById("viewport").style.left.slice(0, -2)), y: parseInt(document.getElementById("viewport").style.top.slice(0, -2)) }; } return{ setPosition: setPosition, getPosition: getPosition, translate: translate, show: show, hide: hide, create: create}; }()) tilemap.setPosition(0, 0); tilemap.create(); sprites.createSprites(spriteSet); sprites.setPosition(0, 235, 240); sprites.show(0); function loadResources(){ const img1 = 'https://georgec0stanza.github.io/2DgraphicsEngine/images/ferns.jpg'; const img2 = 'https://georgec0stanza.github.io/2DgraphicsEngine/images/tulips.jpg'; const img3 = 'https://georgec0stanza.github.io/2DgraphicsEngine/images/water.jpg'; const sprite0 = 'https://georgec0stanza.github.io/2DgraphicsEngine/images/daffodil.jpg'; const sprite1 = 'https://georgec0stanza.github.io/2DgraphicsEngine/images/eagle.jpg'; spriteSet = [sprite0, sprite1]; tileSet = [img1, img2, img3]; tilemapList = [tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] ,tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] ,tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] ,tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] ,tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] ,tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] ,tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] ,tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] ,tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] ,tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] ,tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] ,tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] ,tileSet[2], tileSet[1] , tileSet[1] , tileSet[0], tileSet[0] , tileSet[2], tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] ,tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] ,tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] ,tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] ,tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] ,tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] ,tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] ,tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] ,tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] ,tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] ,tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] ,tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] ,tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] , tileSet[2], tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] ,tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] ,tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] ,tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] ,tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] ,tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] ,tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] ,tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] ,tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] ,tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] ,tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] ,tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] ,tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] , tileSet[2]]; }

html:

<!DOCTYPE html> <html> <head> <meta charset="utf-8" /> <meta http-equiv="X-UA-Compatible" content="IE=edge"> <title>Page Title</title> <meta name="viewport" content="width=device-width, initial-scale=1"> <link rel="stylesheet" type="text/css" media="screen" href="main.css" /> <script src="main.js" defer></script> </head> <body> <div id="window"> <div id="viewport"> <div id ="tilemap"> </div> </div> </div> </body> </html>

css:

:root { --TILESIZE: 50px; /* Size of tiles */ --SPRITESIZE: 50px; /* Size of sprites */ --WINDOWIDTH: 650px; --WINDOWHEIGHT: 500px; --WINDOWBACKGROUND: white; } body { background-color: black; } img { display: block; } #window { overflow: hidden; margin: 0 auto; position: relative; width: var(--WINDOWIDTH); height: var(--WINDOWHEIGHT); background: var(--WINDOWBACKGROUND); } #viewport { position: absolute; } #tilemap { display: grid; z-index: -1; } .tile { text-align: center; } .tileImage { height: var(--TILESIZE); width: var(--TILESIZE); } .sprites { position: absolute; height: var(--SPRITESIZE); width: var(--SPRITESIZE); z-index: 1; transform: scale(1); } ```
\$\endgroup\$

1 Answer 1

Reset to default
3
\$\begingroup\$

There are a number of improvements you can make.

Reassigning global variables so that a particular function can proceed to use them (spriteSet, tileSet, tilemapList) is pretty strange. It would make a lot more sense if these were only passed as arguments to the functions that need them instead. In fact, since the tileSet isn't being used anywhere except inside loadResources, that can be made completely local:

const { spriteSet, tilemapList } = loadResources(); tilemap.setPosition(0, 0); tilemap.create(tilemapList); sprites.createSprites(spriteSet);

Always declare variables with const when possible - you don't intend to reassign the sprites or the tilemap object, right? There are a few variables like this - consider using a linter to prompt you to fix these sorts of potential mistakes automatically.

The tilemapList definition is repetitive and difficult to read:

const tilemapList = [tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] ,tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] ,tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] ,tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] ,tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] ,tileSet[2], tileSet[1] , tileSet[1] , tileSet[0] ,tileSet[2], tileSet[1]

Consider defining the layout with a single string instead, where each cell (numeric, indicating the index of the tile in the tileSet) is separated by a space (or few) or newlines, which gets parsed into the array above afterwards - this way, it'll be far more readable and less prone to copy/paste bugs or the like:

const tiles = ` 2 1 1 0 2 1 1 0 2 1 1 0 2 1 1 0 2 1 1 0 2 2 2 0 0 0 2 2 2 0 0 0 2 2 2 0 0 0 1 1 ` .trim() .split(/\s+/) .map(index => tileSet[index]);

Numeric-indexed IDs are never a good idea - IDs are for elements that are absolutely unique in a document, and they create global variables, unfortunately. If you want a way to select the nth sprite in the tilemap, just select the nth child of the tilemap:

tilemap.children[i]

You might put the children into an array, and have the array in a closure or on an instance so it's easily accessible by all methods. (See snippet below for an example)

Having sprites for the main sprite maker object isn't as precise as it could be. I'd expect a variable named sprites to be an array-like collection of sprite objects. Maybe call the main sprite maker SpritesMaker instead? You can also use a class instead of an IIFE in order to have the children as a property of the instance, as mentioned above.

You frequently select elements over and over again:

spritesList.forEach(x => { // do stuff document.getElementById("window").appendChild(sprite);

Consider selecting the element once instead, rather than having to go through the DOM to find the element again each time:

const win = document.getElementById("window"); spritesList.forEach(x => { // do stuff win.appendChild(sprite);

You have a lot of places where the above pattern can make the code a lot cleaner - instead of repeating document.getElementById(x) multiple times, do const someElement = document.getElementById(x) and then repeat someElement instead. Nearly all of the functions in both the big sprites object and the tilemap object can be significantly improved with this technique.

But window is a very weird ID - it's very easily confused with window, the global object. Consider using something else, like main. You might even avoid IDs altogether, since every element with an ID creates an additional global variable of that name, which can occasionally result in confusing weird bugs.

You have

spritesList.forEach(x => {

x isn't very descriptive at all. Best to call a variable what it represents, maybe srcStr.

Since you're appending each sprite to the .main, you may avoid having to set a class name on each sprite element by using the .main > img in your CSS instead.

You have

if (dX === true && dY === true){ style.transform = "scale(1)"; } else if (dX === true && dY === false){

You may use simple truthy/falsey tests instead if you wish:

if (dX && dY){ style.transform = "scale(1)"; } else if (dX && !dY){ style.transform = "scale(1, -1)"; }

Rather than frequently checking and reassigning with

let pair = {}; if (transform === "scale(1)"){ pair = {x: 1, y: 1}; } else if (transform === "scale(-1)"){ pair = {x: -1, y: -1}; } // etc return pair;

You might consider just returning the object instead:

if (transform === "scale(1)"){ return {x: 1, y: 1}; } else if (transform === "scale(-1)"){ return {x: -1, y: -1}; }

If you do want to keep using the IIFE, note that you may use shorthand properties in ES2015, and you're using ES2015 syntax already. The below:

return{ setPosition: setPosition, getPosition: getPosition, getOrientation: getOrientation, // ...

simplifies to

return { setPosition, getPosition, getOrientation, // ...

All together:

'use strict'; const TILEMAPSIZE = 160; // Total number of tiles displayed. Must be a multiple of TILECOLUMNS. const TILECOLUMNS = 16; // Number of columns class SpritesMaker { constructor(spritesList) { const main = document.querySelector('.main'); this.sprites = spritesList.map(srcStr => { const sprite = document.createElement('img'); sprite.src = srcStr; main.appendChild(sprite); return sprite; }); } show(index) { this.sprites[index].style.visibility = 'visible'; } hide(index) { this.sprites[index].style.visibility = 'hidden'; } reflectY(index) { const { style } = this.sprites[index]; const transform = style.transform; if (transform === "scale(-1, 1)"){ style.transform = "scale(1)"; } else if (transform === "scale(1, -1)"){ style.transform = "scale(-1)"; } else if (transform === "scale(-1)"){ style.transform = "scale(1, -1)"; } else { style.transform = "scale(-1, 1)"; } } reflectY(index) { const { style } = this.sprites[index]; const transform = style.transform; if (transform === "scale(-1, 1)"){ style.transform = "scale(-1)"; } else if (transform === "scale(1, -1)"){ style.transform = "scale(1)"; } else if (transform === "scale(-1)"){ style.transform = "scale(-1, 1)"; } else { style.transform = "scale(1, -1)"; } } reflect(index, dX, dY) { const { style } = this.sprites[index]; if (dX && dY){ style.transform = "scale(1)"; } else if (dX && !dY){ style.transform = "scale(1, -1)"; } else if (!dX && dY){ style.transform = "scale(-1, 1)"; } else if (!dX && !dY){ style.transform = "scale(-1)"; } } translate(index, dX, dY) { const { style } = this.sprites[index]; const posLeft = Number(style.left.slice(0, -2)) + dX; const posTop = Number(style.top.slice(0, -2)) + dY; style.left = posLeft + "px"; style.top = posTop + "px"; } setPosition(index, dX, dY) { const { style } = this.sprites[index]; style.left = dX +"px"; style.top = dY + "px"; } getPosition(spriteIndex) { const { style } = this.sprites[index]; return { x: Number(style.left.slice(0, -2)), y: Number(style.top.slice(0, -2)) }; } getOrientation(index) { const { transform } = this.sprites[index].transform; if (transform === "scale(1)"){ return {x: 1, y: 1}; } else if (transform === "scale(-1)"){ return {x: -1, y: -1}; } else if (transform === "scale(1, -1)"){ return {x: 1, y: -1}; } else if (transform === "scale(-1, 1)"){ return {x: -1, y: 1}; } else { return {x: 0, y: 0}; } } } class TileMap { constructor(tilemapList){ this.viewport = document.querySelector('.viewport'); this.tilemapDiv = document.querySelector('.tilemap'); // set the number of columns let value = ""; for (let i = 0; i < TILECOLUMNS; ++i){ value += "auto "; } this.tilemapDiv.style.gridTemplateColumns = value; // create the tile map this.tiles = []; for (let i = 0; i < TILEMAPSIZE; ++i){ this.tiles.push(this.tilemapDiv.appendChild(document.createElement('div'))); }; // insert images tilemapList.forEach((src, i) => { const img = document.createElement('img'); img.src = src; this.tiles[i].appendChild(img); }) } show() { this.tilemapDiv.style.visibility = "visible"; } hide() { this.tilemapDiv.style.visibility = "hidden"; } translate(dX, dY) { const { style } = this.viewport; const posLeft = Number(style.left.slice(0, -2)) + dX; const posTop = Number(style.top.slice(0, -2)) + dY; style.left = posLeft + "px"; style.top = posTop + "px"; } setPosition(dX, dY) { const { style } = this.viewport; style.top = dY + "px"; style.left = dX + "px"; } getPosition() { const { left, top } = this.viewport.style; return { x: style.left.slice(0, -2), y: style.top.slice(0, -2) }; } } const { spriteSet, tilemapList } = loadResources(); const tileMap = new TileMap(tilemapList); tileMap.setPosition(0, 0); const fernsTulipsSprites = new SpritesMaker(spriteSet); fernsTulipsSprites.setPosition(0, 235, 240); fernsTulipsSprites.show(0); function loadResources(){ const img1 = 'https://georgec0stanza.github.io/2DgraphicsEngine/images/ferns.jpg'; const img2 = 'https://georgec0stanza.github.io/2DgraphicsEngine/images/tulips.jpg'; const img3 = 'https://georgec0stanza.github.io/2DgraphicsEngine/images/water.jpg'; const sprite0 = 'https://georgec0stanza.github.io/2DgraphicsEngine/images/daffodil.jpg'; const sprite1 = 'https://georgec0stanza.github.io/2DgraphicsEngine/images/eagle.jpg'; const spriteSet = [sprite0, sprite1]; const tileSet = [img1, img2, img3]; const tilemapList = ` 2 1 1 0 2 1 1 0 2 1 1 0 2 1 1 0 2 1 1 0 2 1 1 0 2 1 1 0 2 1 1 0 2 1 1 0 2 1 1 0 2 1 1 0 2 1 1 0 2 1 1 0 0 2 2 1 1 0 2 1 1 0 2 1 1 0 2 1 1 0 2 1 1 0 2 1 1 0 2 1 1 0 2 1 1 0 2 1 1 0 2 1 1 0 2 1 1 0 2 1 1 0 2 1 1 0 2 2 1 1 0 2 1 1 0 2 1 1 0 2 1 1 0 2 1 1 0 2 1 1 0 2 1 1 0 2 1 1 0 2 1 1 0 2 1 1 0 2 1 1 0 2 1 1 0 2 1 1 0 2 ` .trim() .split(/\s+/) .map(index => tileSet[index]); return { spriteSet, tilemapList }; }
:root { --TILESIZE: 50px; /* Size of tiles */ --SPRITESIZE: 50px; /* Size of sprites */ --WINDOWIDTH: 650px; --WINDOWHEIGHT: 500px; --WINDOWBACKGROUND: white; } body { background-color: black; } img { display: block; } .window { overflow: hidden; margin: 0 auto; position: relative; width: var(--WINDOWIDTH); height: var(--WINDOWHEIGHT); background: var(--WINDOWBACKGROUND); } .viewport { position: absolute; } .tilemap { display: grid; z-index: -1; } .tilemap > div { text-align: center; } .tilemap > div > img { height: var(--TILESIZE); width: var(--TILESIZE); } .main > img { position: absolute; height: var(--SPRITESIZE); width: var(--SPRITESIZE); z-index: 1; transform: scale(1); visibility: hidden; }
<div class="main"> <div class="viewport"> <div class="tilemap"> </div> </div> </div>

\$\endgroup\$
15
  • \$\begingroup\$ So I'm still chewing through this, but you made some classes so we can use ` tilemap.children[i] ` but that returns TypeError: "tileMap.children is undefined". (If i is set and tilemap is tileMap) \$\endgroup\$ Commented May 12, 2020 at 15:16
  • \$\begingroup\$ Also, are classes \$\endgroup\$ Commented May 12, 2020 at 15:21
  • \$\begingroup\$ If you do const tileMap = new TileMap(tilemapList);, you have an instance of the object as tileMap, not the tilemap element - better to decrease these sorts of name collisions when possible. In the code in the snippet, I assigned the element to the tilemapDiv property of the instance, so if you want to get to the element, you have to access that property of the instance. The instance doesn't have a children property. \$\endgroup\$ Commented May 12, 2020 at 15:22
  • \$\begingroup\$ Classes are generally for when you want to tie together persistent data (here, HTML elements) with methods that do stuff with that data (here, modify the elements). Classes are very suited for the job here. \$\endgroup\$ Commented May 12, 2020 at 15:24
  • 1
    \$\begingroup\$ I'd prefer to change the src of the image. this.tiles[0].children[0].src = 'newSrc'. this.tilemapDiv.children will give you the same sort of collection as tileMap.tiles \$\endgroup\$ Commented May 12, 2020 at 17:18

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.