1
\$\begingroup\$

I would like to know what you think of this assets manager for a game. If it can be optimized or simplified? It allows you to download sprites, tiles, json files, and other images.

const images = {};
images.tiles = {};

exports.images = images;
/**
* Download an image and store it in the cache
* @param {string} url - Address of the image
* @param {string} name - Name of the image
* @param {function} onImageLoaded -Execute this function when all images are loaded
*/

const loadImage = (url, name, onImageLoaded) => {

    if (images[name]) return onImageLoaded(images[name]);

    const image = new Image();

    image.onload = () => {
        if (image.complete) {
            images[name] = image;
            return onImageLoaded(image);
        }
    };

    image.src = url;
};

/*
* Download all images 
* @param {object} images - 
* @param {function} onImagesLoaded -
*/

const loadImages = (images, onImagesLoaded) => {
    var imagesLoaded = 0;


    const onImageLoaded = () => {
        imageLoaded++;

        if (images.length != imagesLoaded) return ;
        onImagesLoaded();
    }

    for (const image of images)
        loadImage(image.url, image.name, onImageLoaded)
};


/*
* Download all tiles
* @param {object} tiles - Tiles to download 
* @param {function} onTilesLoaded - Function that is executed when the tiles are downloaded
*/

const downloadTiles = (tiles, onTilesLoaded) => {

    const images = tiles.map(tile => {
        const image = {};

        image.name = 'tile_' + tile.id;
        image.url =  'assets/tiles/' + image.name + '.png';

        return image;
    });

    loadImages(images, onTilesLoaded);
};

exports.downloadTiles = downloadTiles;

/*
* Download all the images and store them in cache
* @param {object} sprites - Sprites to download
* @param {function} onSpritesLoaded - Function that is executed when the sprites are downloaded
*/

const downloadSprites = (sprites, onSpritesLoaded) => {
    const images = sprites.map(sprite => {
        const image = {};

        image.name = 'sprite_' + sprite.id;
        image.url = 'assets/sprites/' + image.name + '.png';

        return image;
    })

    loadImages(images, onSpritesLoaded);
}





const toJSON = response => response.json();

/*
* @param {string} url - 
* @param {string} name -
*/

const loadJSON = (url, name) => 
    fetch(url, { method: 'GET' })
        .then(toJSON);

exports.loadJSON = loadJSON;

exports.loadImage = loadImage;
exports.loadImages = loadImages;


const getTileCoordinates = (image, index, width, height) => {
    var row = 0;
    var x = width * index, y;

    while(true){
        if (x < image.width) break ;
        x-=image.width;
        row++;
    };
    y = height * row;

    return {x, y};
};
exports.getTileCoordinates = getTileCoordinates;
\$\endgroup\$
  • \$\begingroup\$ For performance reasons, textures are often grouped together in a single texture map / image. This would improve download speed, too. \$\endgroup\$ – le_m Jan 8 '18 at 10:16
  • \$\begingroup\$ And compared to the code, it seems optimistic? \$\endgroup\$ – ken Jan 8 '18 at 10:28
  • 1
    \$\begingroup\$ @le_m grouping images together in a single image can be a very bad thing in terms of performance if you have a lot of image data. JS 2D API will move image data between GPU and CPU RAM outside your control. If you have one large image that whole image will be moved even if you use only a small part. It is best to keep related images together in power of two sizes near the device display res. Load time is insignificant to play time and duplication of some assets across images to prevent GPU RAM thrashing is worth the load time. \$\endgroup\$ – Blindman67 Jan 8 '18 at 12:12
  • \$\begingroup\$ @Blindman67 So would you still recommend grouping related textures together in let's say 1024x1024 texture maps - but not more than that? \$\endgroup\$ – le_m Jan 8 '18 at 15:46
  • 1
    \$\begingroup\$ @le_m 1024 square (4MB) is a good size but if you are on 4K display device it will easily handle a 4K (64MB) image, though a HD 1080 device may have trouble with a 4K image. It depends on the GPU RAM and the RAM size needed to hold content you are rendering from. The best result is to check device resolution then load the image set that best matches the device. Go 1 byte over available (to the page) GPU RAM and frame rate will drop from 60 to ~10 . Also related should not mean all grass tiles, related should mean likely to be rendered at the same time. \$\endgroup\$ – Blindman67 Jan 8 '18 at 17:19
2
\$\begingroup\$

Naming and Commenting

An important part of being a programmer is good naming.

Looking at your code at first glance it seemed fine, until I started to rewrite it (I always rewrite when reviewing). For good code I should be able to rewrite the code without the need to go back to the original source for clarification. In this example I was constantly going back to try and understand what you were doing.

The main reason was that the name of variables that did not match what the variables contained and conflicted with important global names.

Comments

To make it worse the comments confused things even further

So let's examine the comments

/**
* Download an image and store it in the cache
* @param {string} url - Address of the image
* @param {string} name - Name of the image
* @param {function} onImageLoaded -Execute this function when all images are loaded

Last line is completely wrong, maybe?

  /* @param {function} onImageLoaded - callback for image onload event

3rd line not helpful , maybe?

 /* @param {string} name - Name used to store image in images object

2nd line, address? its a url or more apt a relative path to the image resource

1st line store it in a cache? Can't find any cache, but looking at the code I found the images object which you could call a cache, and download no you are not downloading in the classical sense of the word.

Maybe the following more clearly describes what the function does.

  /* Creates and loads Image adding it as a named object to images

The the second function's comments are as clear as mud

/* Download all images 
 * @param {object} images - 
 * @param {function} onImagesLoaded -

maybe

 /* Create and load images from an array image descriptions
  * @param {object} images - array of image descriptions contains url and name
  * @param {function} onImagesLoaded - Callback function called when all 
  *                                    images in array have loaded.

I have to admit that the very first thing I do when reviewing code is remove all comments (they rarely help), so your comments did not confuse me, only when writing this did I read them.

If you are going to add comments they must make sense, for if they dont they only create confusion, and that will result in problems when you return in months to touch up the code.

Write comments addressed to a 3rd person that is not in your head space. Sure the images object is a cache, you know that but that is not obvious to anyone else.

Names

Names are short.

When you name a variable it is at the point where you define its scope, that scope gives the variable a context. You name a variable in its context and don't add redundant information that is clearly implied from the context.

For example you have the argument onImageLoaded for the function loadImage I would say that the 'Image' part of the name is rather obvious so why add it. onLoaded is shorter, does not create ambiguity.

Names create entities

When you name an object you create an entity in the mind of the person reading your code (that person will be you months from now). When you see that name you associate the entity. Associating a global name with multiple entities leads to confusion.

Global names should be unique across its scope. You used the name images defined as const images = {}; but in parts of the code you also use the name images to represent an array. So you will have to be checking is this the images in global scope, or the images as an array.

Also image is used to represent an Image in some places and an image description in other places, confusing...

General comments

  • Use strict mode it will result in better code and runs faster which is good for games. Add "use strict"; to the top of the JS file.

  • Don't forget semicolons

  • Don't create blockless blocks

//bad
for (const image of images)
    loadImage(image.url, image.name, onImageLoaded)

// good
for (const image of images) {                         // define block start
     loadImage(image.url, image.name, onImageLoaded); // semicolon
}                                                     // define block  end
  • You are not using standard export so that means the whole set of functions could be in global scope. Use a singleton or use the native export. Try to limit your exports, exporting every function is just a pain an will lead to problems.

  • The function getTileCoordinates is out of place and does not belong here. BTW there is a better way to find the row

function getTileCoordinates(image, index, width, height){
    const tilesAcross = image.width / width | 0; // | 0 floors result
    return {
        x : width * (index % tilesAcross),
        y : (index / tilesAcross | 0) * height
    };
}
  • Don't repeat. The functions downloadSprites and downloadTiles are repeats

  • I could not work out why you are returning the result of the load callbacks. There is no consistency as you do it sometimes and other times not. Also you are passing the image as an argument to the callbacks but there is no indication that you use it. For the onload event you can access the image via this if needed. Note you will have to use a standard function declaration for the callback.

  • There is no error checking. What do you do if the connection or a request is lost?

A rewrite

This is how I would have written your code. It is just a suggestion example and far from ideal. I do not know what you wanted exposed but I only exposed what was not called internally (assuming this is a module)

"use strict";
const images = {
    tiles : {},
};

function loadImage(imageDesc, onLoad) {
    var image = images[imageDesc.name];
    if (image) { onLoad() }
    else{
        image = images[imageDesc.name] = new Image();
        image.src = imageDesc.url;
        image.onload = onLoad;
    }
},

function loadImages(imageArray, onAllLoaded) {
    var imagesLoaded = 0;
    const onLoad = () => {
        if (imageArray.length === ++ imagesLoaded) { onAllLoaded() }
    }
    for (const imageDesc of imageArray) { loadImage(imageDesc, onLoad) }
},

function loadList(type, list, onLoad) {
    loadImages( 
        list.map(imageDesc => {
            const name = `${type}_${imageDesc.id}`;
            return  { name, url : `assets/${type}/${name}.png` };
        }),
        onLoad
    );
},

const assests = {
    images,
    loadTiles(tiles, onLoaded) { 
        loadList("tiles", tiles, onLoaded);
    },
    loadSprites(sprites, onLoaded) { 
        loadList("sprites", sprites, onLoaded);
    },
    loadJSON : (url, name) => fetch(url, { method: 'GET' }).then(response => response.json()),
};


export default assests;
\$\endgroup\$
  • \$\begingroup\$ Where could I put the function getTileCoordinates? \$\endgroup\$ – ken Jan 8 '18 at 17:21
  • \$\begingroup\$ @ken IF you have a rendering module that would be a good place, There is no hard and fast rule. I encode sprite and tile information into the image which I decode by passing the image to the rendering engine which returns a new canvas/s with attached properties for sprite/tile positions that I use rather than the image, which I dump. \$\endgroup\$ – Blindman67 Jan 8 '18 at 17:32
  • \$\begingroup\$ So, if I return an array of images rather than coordinates. The function could have stayed here? \$\endgroup\$ – ken Jan 8 '18 at 20:52
  • \$\begingroup\$ @ken if you are having trouble finding a place for the function then it is fine in this module and it does not have to return images. Ultimately it is your code and a lot of coding is about "does it feel right" and if it feels like this is a good place for it then it is. \$\endgroup\$ – Blindman67 Jan 8 '18 at 21:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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