2
\$\begingroup\$

I recently started making a game in JavaScript, but I've only really built the renderer. This is the first 'real' game I started working on and I don't know if I'm doing any major things wrong and if there's things to improve.

renderer.js

function Renderer(atlas){
    this.canvas = document.querySelector("canvas");
    this.ctx = this.canvas.getContext("2d");
    

    this.width = this.canvas.width = screen.width;
    this.height = this.canvas.height = screen.height;

    this.loaded = 0;
    this.tileSize = 64;
    if(atlas){
        this.tileAtlas = new Image(this.tileSize, this.tileSize);
        this.loaded = 1;
        this.tileAtlas.src = atlas;
    }

    this.showImg = function(arg, x, y){
        let offset = arg * 16 - 16;

        if(arg){
            this.ctx.drawImage(this.tileAtlas, offset, 0, 16, 16, x, y, this.tileSize + 1, this.tileSize + 1);
        }
    }

    this.renderMap = function(map, xd, yd){
        this.ctx.imageSmoothingEnabled = false;
        for(let y = 0; y < map.length; y++){
            for(let x = 0; x < map[y].length; x++){
                if(x*this.tileSize+xd<-this.tileSize){continue}
                if(x*this.tileSize+xd>this.canvas.width){break}

                this.showImg(map[y][x], x*this.tileSize+xd, y*this.tileSize+yd);
            }
        }
    }

    this.clear = function(){
        this.ctx.clearRect(0, 0, this.canvas.width, this.canvas.height);
    }
}

index.html

<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="UTF-8">
    <meta http-equiv="X-UA-Compatible" content="IE=edge">
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
    <title>Gayme test</title>
    <style>
        body{
            overflow: hidden;
            background-color: white;
        }
        canvas{
            position: absolute;
            background-color: rgb(0, 140, 255);
            top: 0px;
            left: 0px;
            z-index: -1;
        }
    </style>
</head>
<body>
    <canvas></canvas>
    <p></p>
    <script src="terrain.js"></script>
    <script src="renderer.js"></script>
    <script src="main.js"></script>
</body>
</html>
\$\endgroup\$
1
  • \$\begingroup\$ Just an idea: you could scale using the height of the atlas if you have an atlas consisting of subsequent images. Generally, if you still have literals such as 16 in your code, you should either use them to validate the size or you should remove them in favor of the input. \$\endgroup\$ Aug 11 '21 at 14:24
0
\$\begingroup\$
this.loaded = 0;

I don't see the point of this variable; not much longer it will be set to 1, but it is never exposed anywhere.


this.width = this.canvas.width = screen.width;
this.height = this.canvas.height = screen.height;

I think this should be a parameter to the renderer with a default value. Why do you need this.width if you're only going to use canvas.width? Please make sure that all values are represented only once, or you'll certainly get into an illegal state later on.


this.showImg = function(arg, x, y){

arg = "Arg!" - variables should be named according to their function; this one isn't. Apparently it is a 1 based index, but at least then call it index or indexInAtlas. That makes the whole idea of using an atlas clear in one go. Personally, I always use zero based indexing - and with good reason, but if you use one based then use: (indexInAtlas - 1) * 16 to increase understanding.


if(arg){

If you don't want to draw anything, then don't call a method that starts with show; skip calling the function in renderMap.


this.renderMap = function(map, xd, yd){

There is no indication whatsoever what xd and yd is and it is even more confusing with the x and y parameters in the function right above it. It seems some kind of margin, but in that case it should be marginX and marginY. However, I don't expect margins to change at every call: this should be a property of the canvas, it should not be a parameter.


    for(let y = 0; y < map.length; y++){
            if(x*this.tileSize+xd<-this.tileSize){continue}

Here you show the readability of a line with whitespace and the one without. Always use whitespace:

            if(x * this.tileSize + xd < -this.tileSize) {
                continue
             }

See what a difference it makes?


Where is the testing of y and yd? Do you assume infinite space there? In that case, at least create a note / comment for it.


            this.showImg(map[y][x], x*this.tileSize+xd, y*this.tileSize+yd);

The drawing of the map should not yet perform these calculations. It is much better to have a drawTile function that uses x and y for the map, and then retrieves the margins from the properties.


It is highly recommended to separate calculations on the map, the calculations to retrieve images from the atlas and the calculations to perform the actual drawing as much as possible. This will save you an awful lot of trouble trying to understand the code later on, when the game starts to become more and more complex. Divide and conquer is not just a strategy when playing the game; it is also the best strategy during development.

\$\endgroup\$
4
  • \$\begingroup\$ Ok, thanks so much! About the last answer, if I were to add a drawTile() function, would I pass in x and y as inputs to calculate in said function? I don't really get what you mean by 'retrieve margins from the properties', sorry. \$\endgroup\$
    – Coenicorn
    Aug 11 '21 at 14:29
  • \$\begingroup\$ Yes, so you do the checking if you have to draw a tile in renderMap, and you calculate the actual place where to put the image in the drawTile functions. Maybe I misunderstood the dx and dy values - if the starting point of the map can change for every call to renderMap then they are maybe not margins and should indeed be parameters. Often you can avoid these kind of confusions by creating e.g. a CanvasPosition(x,y) class and then create a CanvasPosition mapPosition variable or similar. That way x and y are just x and y but the function of the coordinates is clear. \$\endgroup\$ Aug 11 '21 at 14:58
  • \$\begingroup\$ Ok, I get it now. Thank you good sir! \$\endgroup\$
    – Coenicorn
    Aug 11 '21 at 15:02
  • \$\begingroup\$ In that case you'd get e.g. drawTile(tileIndex, positionOnMap, mapPositionOnCanvas). \$\endgroup\$ Aug 11 '21 at 15:02

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.