Collision Script in javascript help needed

Question

Alright so I just tried to cut down on lines of code by changing manually writing out everything into an array.. My problem is that he now teleports and gravity doesnt work... first of all I have a grid of cell objects which basically are a 32x32 grid "640X480". These objects are all passed onto an array like so-

var gridcellarray = [750];
gridcellarray[0] = cell0;
gridcellarray[1] = cell1;
gridcellarray[2] = cell2;
and so on for 750 32x32 cells...

Now as for the collision script I have this...

function collisioncheck(obj) {
    obj = obj;
    for(var i = 0; i < gridcellarray.length; i++){
    //really long if statement// sorry...
    if ((gridcellarray[i].solid == true) && ((obj.PosY >= gridcellarray[i].y - obj.maskImg.height) && !(obj.PosY >= gridcellarray[i].y ) && !((obj.PosX > gridcellarray[i].x + solidOriginX + solidImg.width/2-5) || (obj.PosX < gridcellarray[i].x - solidOriginX - solidImg.width/2)))){
         if(obj.onground == 0){
             obj.PosY = gridcellarray[i].y - obj.maskImg.height;
             obj.VelY = 0;
             obj.onground = 1;
             obj.jump = 0;
         }
      }
     else if (obj.PosY >= canvas.height - obj.maskImg.height){
        if(obj.onground == 0){
            obj.VelY = 0;
            obj.onground = 1;
            obj.jump = 0;
            obj.PosY = canvas.height - obj.maskImg.height;
        }
    }
    else {
        obj.VelY += 1;
        obj.onground = 0;
    }
  }
 }

now this code worked just fine before If I had manually copied it 750 times for each cell and the problem is that Now that I have one iteration of it cycling through them as an array it gets confused and teleports me to the bottom and If I try to jump even when not below or on a block it teleports me back to the bottom.

//edit
I think all the variables should explain their purpose by name but if you have any questions please ask.

//edit
All Variables apply the object your checking collision for such as collisioncheck(player) here is a link to a running demo of the code... Zack Bloom's code is in it and it works great applied to the unposted horizontal collision scripts but vertically, it wont reset and acknowledge your standing on a block ie ongroud = true; Demo link

//edit
Ok now that Zack pointed out resetting the y to and x amount it helped alot but he is still not able to jump, as the onground variable doesnt want to reset when the collision is detected... oh and the teleporting is due to my upwards script -

  //Upwards Collision//     
  if ((cell.solid == true) && ((obj.PosY >= cell.y - 32) && !(obj.PosY > cell.y+32) && !((obj.PosX > cell.x + solidOriginX + solidImg.width/2-5) || (obj.PosX < cell.x - solidOriginX - solidImg.width/2)))){
         if (obj.onground == 0){
             obj.VelY += 1;
             obj.onground = 0;
             obj.PosY = cell.y + obj.maskImg.height-13; 
         }
     }

Any Ideas on how to fix THIS mess above? to stop him from teleporting? It is only meant to check if the top of the collision mask(red rectangle) is touching the block as if trying to jump through it, but it is meant to stop that from happening so you hit your head and fall back down. Thanks in Advance!

Answer 1

The else if / else really don't belong in the loop at all, they don't evaluate the cell being looped over, but will be triggered many times each time collisioncheck is called.

function collisioncheck(obj) {
    for(var i = 0; i < gridcellarray.length; i++){
        var cell = gridcellarray[i];

        if (cell.solid && ((obj.PosY >= cell.y - obj.maskImg.height) && !(obj.PosY >= cell.y ) && !((obj.PosX > cell.x + solidOriginX + solidImg.width/2-5) || (obj.PosX < cell.x - solidOriginX - solidImg.width/2)))){
             if(!obj.onground){
                 obj.PosY = cell.x - obj.maskImg.height;
                 obj.VelY = 0;
                 obj.onground = 1;
                 obj.jump = 0;

                 break;
             }
         }
     }
     if (obj.PosY >= canvas.height - obj.maskImg.height){
        if(!obj.onground){
            obj.VelY = 0;
            obj.onground = 1;
            obj.jump = 0;
            obj.PosY = canvas.height - obj.maskImg.height;
        }
     } else {
        obj.VelY += 1;
        obj.onground = 0;
     }
}

But an even better way of doing it would be to store each gridcell based on where it was, so you just have to look up the gridcells near the ship.

// You only have to do this once to build the structure, don't do it every time you
// need to check a collision.
var gridcells = {};
for (var i=0; i < gridcellarray.length; i++){
    var cell = gridcellarray[i];

    var row_num = Math.floor(cell.PosX / 32);
    var col_num = Math.floor(cell.PosY / 32);

    if (!gridcells[row_num])
        gridcells[row_num] = {};

    gridcells[row_num][col_num] = cell;
}

// Then to check a collision:
function collisioncheck(obj){
    // I'm not sure exactly what your variables mean, so confirm that this will equal
    // the width of the object:
    var obj_width = solidImg.width;
    var obj_height = obj.maskImg.height;

    var collided = false;

    var left_col = Math.floor(obj.PosX / 32),
        right_col = Math.floor((obj.PosX + obj_width) / 32),
        top_row = Math.floor(obj.PosY / 32),
        bottom_row = Math.floor((obj.PosY + obj_height) / 32);

    for (var row=top_row; row <= bottom_row; row++){ 
        for (var col=left_col; col <= right_col; col++){
            var cell = gridcells[row][col];

            if (cell.solid){
                collided = true;

                if (row == top_row){
                    // We collided into something above us

                    obj.VelY = 0;
                    obj.PosY = cell.PosY + 32;
                } else if (row == bottom_row && !obj.onground){
                    // We collided into the ground

                    obj.PosY = cell.x - obj_height;
                    obj.VelY = 0;
                    obj.onground = 1;
                    obj.jump = 0;
                }

                if (col == left_col){
                    // We collided left

                    obj.VelX = 0;
                    obj.PosX = cell.PosX + 32;
                } else if (col == right_col){
                    // We collided right

                    obj.VelX = 0;
                    obj.PosX = cell.PosX - obj_width;
                }
            }
        }
    }

    if (obj.PosY >= canvas.height - obj_height){
        if (!obj.onground){
            obj.VelY = 0;
            obj.onground = 1;
            obj.jump = 0;
            obj.PosY = canvas.height - obj_height;
        }
    }

    if (!collided){
        obj.VelY += 1;
        obj.onground = 0;
    }
}

Rather than looping through 720 cells each frame, we are only looking at the cells we know we are overlapping. This is much more efficient and easier to read than all the position calculations.

Answer 2

Some comments on your code:

var gridcellarray = [750];

That creates an array with a single member that has a value of 750. I think you are assuming that it is equivalent to:

var gridcellarray = new Array(750);

which creates an array with a length of 750. There is no need to set the size of the array, just initialise it as an empty array and assign values.

var gridcellarray = [];
gridcellarray[0] = cell0;

This replaces the value of the first member with whatever the value of cell0 is.

function collisioncheck(obj) {
    obj = obj;

There is no point in the second line, it just assigns the value of obj to obj (so it's redundant).

  for(var i = 0; i < gridcellarray.length; i++){

It is much more efficient in most browsers to save the value of gridcellarray.length, otherwise it must be looked up every time (the compiler may not be able to work whether it can cache it), so:

  for (var i = 0, iLen = gridcellarray.length; i < iLen; i++) {

It is more efficient to store a reference to gridcellarray[i] rather than look it up every time. Also, you can use a short name since it's only used within the loop so its purpose is easily found:

    var c = gridcellarray[i];

so not only will the code run faster, but the if statement has fewer characters (and you might prefer to format it differently):

if ((c.solid == true) &&
    ((obj.PosY >= c.y - obj.maskImg.height) && !(obj.PosY >= c.y ) &&
    !((obj.PosX > c.x + solidOriginX + solidImg.width/2-5) ||
      (obj.PosX < c.x - solidOriginX - solidImg.width/2)))) {

Wow, that really is some if statement. Can you break it down into simpler, logical steps?

Ah, Zack has posted an answer too so I'll leave it here.