XNA player/tile collision not working perfectly

Question

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[1].GetObject(x - 1, y);
            var right = MapEngine.Layers[1].GetObject(x + 1, y);
            var up = MapEngine.Layers[1].GetObject(x, y - 1);
            var down = MapEngine.Layers[1].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;
    }

Answer 1

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.