1
\$\begingroup\$

I am working on a background project that is a little interactive. Now I am using some collision detection to draw some lines and stuff.

The code I have works great but it is very bulky and hard to read.

if (background.allPixels[i].location.x - drawLineBetweenPixelRange < background.allPixels[j].location.x + background.allPixels[j].width &&
    background.allPixels[i].location.x + background.allPixels[i].width + drawLineBetweenPixelRange > background.allPixels[j].location.x &&
    background.allPixels[i].location.y - drawLineBetweenPixelRange < background.allPixels[j].location.y + background.allPixels[j].height &&
    background.allPixels[i].location.y + background.allPixels[i].height + drawLineBetweenPixelRange > background.allPixels[j].location.y)

It comes down to running this for the x and y coordinates:

var result = (x - a < y + b) && (x + b + a > y)

Can this be simplified?

I was hoping if the check itself could be shortened by taking stuff out that might counter eachother.

\$\endgroup\$
3
\$\begingroup\$

Bug #1

Your code does not follow pattern (x - a < y + b) && (x + b + a > y) but (x - a < y + b1) || (x + b2 + a > y) instead. So our refactored formula is moot! obsolete


Bug #2

A collision is detected when any of the guards trigger, not when all.


Proposed Solution

Solving both bugs, we could refactor Nikko Khresna's method:

function detectCollision(source, target, line) {
  return 
       source.location.x < target.location.x + target.width + line
    || source.location.x + source.width + line > target.location.x
    || source.location.y < target.location.y + target.height + line
    || source.location.y + source.height + line > target.location.y
  ;
}

and you can call it as follows

const collision = detectCollision(
  background.allPixels[i], background.allPixels[j], drawLineBetweenPixelRange);

Obsolete section because OP made a mistake in the formula: I'll keep it for historical reasons :)

Formula (Obsolete)

The way I see it, we could rewrite your expression:

var result = (x - a < y + b) && (x + b + a > y)

as

var result = (x - a - b < y) && (x + b + a > y)

and further as

var result = (x - (a + b) < y) && (x + (a + b) > y)

now we can substitute (a + b) with offset

var result = (x - offset < y) && (x + offset > y)

which means x and y have to be within offset distance of each other (|x-y| = abs(x-y))

var result = Math.abs(x - y) < offset

Now you can substitute x, y and offset with your variables, and - as suggested in the other answer - use a method that calculates this collision for you.

\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks. This was what i was looking for and it looks very promising i will try it out as soon as i got time. I will let you know. \$\endgroup\$ – JanWillem Huising Aug 27 '19 at 10:23
  • \$\begingroup\$ I understand how my formula wasn't correct cause target width and source width can be different from each other. I do not understand your proposed solution tho. You are using the OR operators, the way i see it this will always translate to true, because if source.location.x < target.location.x + target.width + line is false, source.location.x + source.width + line > target.location.x will always be true. Am i missing something or did you mean to use the AND operator ? \$\endgroup\$ – JanWillem Huising Oct 11 '19 at 13:48
1
\$\begingroup\$

first, accessing those properties all over again is really confusing

let bgI = background.allPixels[i]
let locI = bgI.location
let line = drawLineBetweenPixelRange
let bgJ = background.allPixels[j]
let locJ = bgJ.location


if (locI.x - line < locJ.x + bgJ.width &&
    locI.x + bgI.width + line > locJ.x &&
    locI.y - line < locJ.y + bgJ.height &&
    so on and so on...

second, redefining all those variables all over again really hurts efficiency

function detectCollision(background, line) {
    let bgI = background.allPixels[i]
    let bgJ = background.allPixels[j]
    let locI = bgI.location
    let locJ = bgJ.location

    if (locI.x - line < locJ.x + bgJ.width &&
        locI.x + bgI.width + line > locJ.x &&
        locI.y - line < locJ.y + bgJ.height &&
        so on and so on....
}

detectCollision(background, drawLineBetweenPixelRange)

hope it helps

\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review. Please go into more detail how it "hurts efficiency". Also note that your function uses global variables i and j and therefore will be a a nightmare to debug. \$\endgroup\$ – Zeta Nov 10 '18 at 8:21
  • \$\begingroup\$ Inefficient because: t gives space for human error, is not DRY, responsibilty of methods are not well separated. i and j could be anythig, global var, index of iteration, etc. So I'll leave the real implementation of i and j to OP. \$\endgroup\$ – Nikko Khresna Nov 10 '18 at 11:20
  • 1
    \$\begingroup\$ Thanks for the feedback. Since this code does not run again on any other place i hadn't thought about putting it in its own function. I will think about applying this but is not the answer i am looking for. I whas hoping if the check itself could be shortened by taking stuff out that might counter eachother. \$\endgroup\$ – JanWillem Huising Nov 10 '18 at 14:29
  • 1
    \$\begingroup\$ @NikkoKhresna Additional information should be put into your post. Please edit accordingly. Comments may get deleted when they aren't needed anymore and don't have a history, whereas posts do. \$\endgroup\$ – Zeta Nov 11 '18 at 12:24

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