6
\$\begingroup\$

I finally made a "full dungeon crawler map" thanks to this.

The map looks great to me and I'd like to hear some useful advice from you. What I'm trying to do is to simplify my logic about 'detect boundary of rooms and paths'. Now, it's pretty complicated and in my opinion, it has some duplicated logic.

import mario from '../images/mario.png'
import block from '../images/block.png'

class Game extends Component {
  componentDidMount () {

    /*
    * This bit of codes are inspired by https://eskerda.com/bsp-dungeon-generation/
    * so if you want further information, visit here and check out
    */

    // basic canvas constants
    const ctx = document.getElementById('map').getContext('2d')
    const CANVAS_WIDTH = ctx.canvas.width
    const CANVAS_HEIGHT = ctx.canvas.height

    // the number of resursion: it determines how many room will be created
    const ITERATION = 3

    // size of each block consists of room
    const TILE = 40
    const NUM_TILES_W = CANVAS_WIDTH / TILE
    const NUM_TILES_H = CANVAS_HEIGHT / TILE

    // constants for space division
    const WILL_DISCARD = true
    const H_RATIO = 0.45
    const W_RATIO = 0.45

    const random = (min, max) => Math.floor(Math.random() * (max - min + 1) + min) 

    // Tree > Space > Room
    class Tree {
      constructor(leaf, left, right) {
        this.leaf = leaf
        this.left = undefined
        this.right = undefined
      }

      getLeafs () {
        if (this.left === undefined && this.right === undefined) {
          return [this.leaf]
        } else {
          return [].concat(this.left.getLeafs(), this.right.getLeafs())
        }
      }
    }

    class Space {
      constructor(x, y, w, h) {
        this.x = x
        this.y = y
        this.w = w
        this.h = h
        this.center = { 
                        x: Math.floor(x + (w / 2)), 
                        y: Math.floor(y + (h / 2)) 
                      }
      }   

      drawPath (c, space) {
        const img = new Image()
        img.src = block
        img.onload = () => {
          const pattern = c.createPattern(img, 'repeat')
          c.beginPath()
          c.lineWidth = TILE
          c.strokeStyle = pattern
          c.moveTo(this.center.x * TILE, this.center.y * TILE)
          c.lineTo(space.center.x * TILE, space.center.y * TILE)
          c.stroke()
        }
      }
    }

    class Room extends Space {
      constructor (space) {
        super()
        this.x = space.x + random(1, Math.floor(space.w / 3))
        this.y = space.y + random(1, Math.floor(space.h / 3))
        this.w = space.w - (this.x - space.x) 
        this.h = space.h - (this.y - space.y)
        this.w -= random(0, this.w / 4)
        this.h -= random(0, this.h / 4)
      }

      // draw dungeon tiles 
      draw (c) {
        const img = new Image()
        img.src = block
        img.onload = () => {
          const pattern = c.createPattern(img, 'repeat')
          c.fillStyle = pattern
          c.fillRect(
            this.x * TILE,
            this.y * TILE,
            this.w * TILE,
            this.h * TILE
          )
        }
      } 
    }

    const splitSpace = (space, iter) => {
      let root = new Tree(space)

      if(iter) {
        let rs = randomSplit(space)
        root.left = splitSpace(rs[0], iter-1)
        root.right = splitSpace(rs[1], iter-1)
      }
      return root
    }

    const randomSplit = space => {

      let space1
      let space2

      // choose divide direction: vertically or horizontally
      if (random(0, 1)) {
        space1 = new Space(
          space.x,
          space.y,
          random(1, space.w),
          space.h
        )
        space2 = new Space(
          space.x + space1.w,
          space.y,
          space.w - space1.w,
          space.h
        )

        // will discard too small spaces
        if (WILL_DISCARD) {
          let w1_ratio = space1.w / space1.h
          let w2_ratio = space2.w / space2.h
          if (w1_ratio < W_RATIO || w2_ratio < W_RATIO) {
            return randomSplit(space)
          }
        }
      } else {
        space1 = new Space(
          space.x,
          space.y,
          space.w,
          random(1, space.h)
        )
        space2 = new Space(
          space.x,
          space.y + space1.h,
          space.w,
          space.h - space1.h
        )

        if (WILL_DISCARD) {
          let h1_ratio = space1.h / space1.w
          let h2_ratio = space2.h / space2.w
          if (h1_ratio < H_RATIO || h2_ratio < H_RATIO) {
            return randomSplit(space)
          }
        }
      }

      return [space1, space2]
    }

    const ROOT = new Space(0, 0, NUM_TILES_W, NUM_TILES_H)
    const spacesTree = splitSpace(ROOT, ITERATION)

    /* @desc 
    * primary idea of drawPaths is that find two divided area first
    * and then connect them. Thanks to recursion, 
    * it connect center of them until there aren't left or right. 
    */
    const drawPaths = (c, tree) => {
      if (!(tree.left || tree.right)) return
        tree.left.leaf.drawPath(c, tree.right.leaf)
        drawPaths(c, tree.left)
        drawPaths(c, tree.right)
    }

    const leafs = spacesTree.getLeafs()
    let rooms = []

    for (let i = 0; i < leafs.length; i++) {
      const room = new Room(leafs[i])
      rooms.push(room)
      room.draw(ctx)
    }

    drawPaths(ctx, spacesTree)

    // draw user: new layer of canvas
    const user = document.getElementById('user').getContext('2d')

    // detect boundary
    const checkBoundary = (userX, userY, direction) => {

      const withinRoom = roomNum => { 
        const { x, y, w, h } = rooms[roomNum]

        // if user tries to go out of boundary, then return false
        if ((direction === 'left' && userX === x * TILE) ||
        (direction === 'right' && userX === (x + w) * TILE - 20) ||
        (direction === 'up' && userY === y * TILE) ||
        (direction === 'down' && userY === (y + h) * TILE - 20)) {
          return false
        }

        return (userX >= x * TILE && userX <= (x + w) * TILE - 20)
        && (userY >= y * TILE && userY <= (y + h) * TILE - 20)
      }

      let boundaries = []

      const withinPath = (tree = spacesTree) => {
        const { left, right } = tree

        if (!left && !right) return

        // if user tries to go out of boundary, then return false
        if ((direction === 'up' 
        && ctx.getImageData(userX, userY - 20, 20, 20)['data'][0] === 0) ||
        (direction === 'down' 
        && ctx.getImageData(userX, userY + 20, 20, 20)['data'][0] === 0) ||
        (direction === 'right'
        && ctx.getImageData(userX + 20, userY, 20, 20)['data'][0] === 0) ||
        (direction === 'left'
        && ctx.getImageData(userX - 20, userY, 20, 20)['data'][0] === 0)) {
          return false
        }

        const LEFT = left.leaf.center
        const RIGHT = right.leaf.center

        // vertical path or horizontal path
        // horizontal path
        if (LEFT.y === RIGHT.y) {
          boundaries.push(
            (userX >= LEFT.x * TILE && userX <= RIGHT.x * TILE)
            && (userY >= LEFT.y * TILE - 20 && userY <= LEFT.y * TILE)
          )
        } else {        
          boundaries.push(
            (userX >= LEFT.x * TILE - 20 && userX <= LEFT.x * TILE)
            && (userY >= LEFT.y * TILE && userY <= RIGHT.y * TILE)
          )
        }

        withinPath(tree.left)
        withinPath(tree.right)

        return boundaries.reduce((result, boolean) => {
          result = result || boolean
          return result
        })
      }

      return withinPath() || rooms.some((room, roomNum) => withinRoom(roomNum))
    }

    const MARIO = new Image()
    const drawUser = () => {
      if (checkBoundary(this.props.x, this.props.y)) {
        return user.drawImage(MARIO, this.props.x, this.props.y) 
      }

      // if mario is not in proper location, just relocate it
      this.props.respawn()
      drawUser()
    }

    MARIO.src = mario
    MARIO.onload = () => drawUser()

    document.onkeydown = event => {
      user.clearRect(0,0,user.canvas.width, user.canvas.height)
      switch (event.keyCode) {

        // right
        case 39:
          if (this.props.x < user.canvas.width - 20 &&
          checkBoundary(this.props.x, this.props.y, 'right')) {
              this.props.moveTo('right')
          }
          break

        // left
        case 37:
          if (this.props.x > 0 &&
          checkBoundary(this.props.x, this.props.y, 'left')) {
            this.props.moveTo('left')
          }
          break

        // up
        case 38:
          if (this.props.y > 0 &&
          checkBoundary(this.props.x, this.props.y, 'up')) {
            this.props.moveTo('up')
          }
          break

        //down
        case 40:
          if (this.props.y < user.canvas.height - 20 &&
          checkBoundary(this.props.x, this.props.y, 'down')) {
            this.props.moveTo('down')
          }
          break

        default:
          return undefined
      }
      user.drawImage(MARIO, this.props.x, this.props.y)
    }
  }

The code you should pay attention to is the checkBoundary part. It accepts userX, userY and direction as parameter and returns a boolean value that means the user is in the boundary. It also it has inner functions, each named withinRoom and withinPath.

As you can see, they both have very very long if statement which is hard to understand. The basic idea is if a user is at the outermost location of the room for any direction and tries to go outside, it just returns false to get 'undefined' in the onkeydown event.

I think I can improve this, but I'm lacking ideas of how to make it better.

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! I hope you get great answers. \$\endgroup\$ – Phrancis Jun 23 '17 at 3:15
2
\$\begingroup\$

Before I address the checkBoundary function, I have some review points I would like to start with.

General Javascript style points

Semi-colons aren't used to terminate lines. While they are only required after a handful of statements, it could lead to errors if somehow whitespace got removed. It is a good habit to default to terminating lines with them.

The naming of variables (and constants) could be improved. I see MARIO used for a variable that corresponds to an Image object, as well as mario that is used as the path to the image (i.e. '../images/mario.png' which is actually imported and thus would likely be an object bytemap). Typically, all capitals corresponds to constant values (e.g. string literals) so it would be more appropriate to have the path to the image in all capitals - e.g. MARIO_SRC or PATH_TO_MARIO. Then the variable that corresponds to the Image is still technically mutable despite being declared with const so it would be more appropriate to use camel-case characters for it. Perhaps a less confusing name would be marioImg.

const marioImg = new Image();
marioImg.src = MARIO_SRC;

I see this boolean constant declared: const WILL_DISCARD = true. Do you modify that manually during testing? If not, it seems superfluous and the conditionals that use it could be removed.

I see that the function Tree has been modified (from the example in the post on eskerda) to accept parameters left and right but those don't appear to be used. Was there a point to adding those parameters, perhaps to set the member/instance variables if those are not undefined?

Since features like arrow functions are used, the function Tree. getLeafs() could be simplified using the spread syntax.

getLeafs () {
    if (this.left === undefined && this.right === undefined) {
      return [this.leaf]
    } else {
      return [...this.left.getLeafs(), ...this.right.getLeafs()];
    }
  }

There is a function declared for the onload of MARIO:

MARIO.onload = () => drawUser()

This can be simplified to:

MARIO.onload = drawUser;

checkBoundary

I'm not sure what the best approach is to simplifying this function and the nested functions within it. One thought I have is to abstract conditions - for example in withinRoom you could declare a variable like:

const outsideLeftBound = userX < x * TILE;

And use that in places like

if ((direction === 'left' && userX === x * TILE) ||

as in:

if ((direction === 'left' && outsideLeftBound) ||

And then instead of

return (userX >= x * TILE &&

use:

return !outsideLeftBound

Though I realize that would be changing the logic slightly because < is different than ===.

\$\endgroup\$

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.