# XNA player/tile collision not working perfectly

So I've built a map tile engine in XNA, and have been working on the player/tile collision. The method I'm using works well to some extent, but the player will often move through the walls and get stuck and I can't figure out what's going wrong.

Here's my player update loop. You can see I'm creating a motion vector based on player input, creating a new location where the player will move to, then getting the four surrounding tiles and checking each one for an intersect, then cancelling out the motion in that direction if necessary. If a tile is not null, it is assumed impassable.

Can anyone help point out my mistake?

    public override void Update(GameTime gameTime)
{
Motion = new Vector2();
var newPosition = Position;

//set movement speed
Speed = 1;
if (Controls.IsKeyDown(Keys.LeftShift))
{
Speed = 2;
}

//set movement
if (Controls.IsKeyDown(Keys.S))
{
newPosition.Y += Speed;
Motion.Y = 1;
}
if (Controls.IsKeyDown(Keys.W))
{
newPosition.Y -= Speed;
Motion.Y = -1;
}
if (Controls.IsKeyDown(Keys.A))
{
newPosition.X -= Speed;
Motion.X = -1;
}
if (Controls.IsKeyDown(Keys.D))
{
newPosition.X += Speed;
Motion.X = 1;
}

//normalize speed vector to stop faster diagonal movement
if (Motion != Vector2.Zero)
Motion.Normalize();

//calculate new position
newPosition += Motion * Speed;
var newBounds = new Rectangle((int)newPosition.X, (int)newPosition.Y, Width, Height);

//check for block collision
if (Motion != Vector2.Zero)
{
//get x,y tile index of player position
var x = GetPlayerTileX();
var y = GetPlayerTileY();
//get surrounding 4 blocks
var left = MapEngine.Layers.GetObject(x - 1, y);
var right = MapEngine.Layers.GetObject(x + 1, y);
var up = MapEngine.Layers.GetObject(x, y - 1);
var down = MapEngine.Layers.GetObject(x, y + 1);
//check for collision in all directions, and cancel motion accordingly
if (left != null && left.Bounds.Intersects(newBounds))
Motion.X = 0;
if (right != null && right.Bounds.Intersects(newBounds))
Motion.X = 0;
if (up != null && up.Bounds.Intersects(newBounds))
Motion.Y = 0;
if (down != null && down.Bounds.Intersects(newBounds))
Motion.Y = 0;
}

//move the player
Position += Motion * Speed;

//stops player leaving the map edge
LockPlayer();

//update bounds
Bounds = new Rectangle((int)Position.X, (int)Position.Y, Width, Height);

//camera follows player
Camera.Position = Position;
}


I think your issue is that you are incrementing newPosition twice. First you modify the corresponding components when you check for key presses from the player, and then you modify it again later with newPosition += Motion * Speed;. One or the other is unnecessary.
I personally would remove incrementing from the key checking, and simply set the Motion value.