New to LINQ: Is this the right place to use LINQ?

Question

First off, I'm new to LINQ, so I don't really know the ins and outs of it. I'm attempting to use it in some code at the minute, and according to my diagnostics it appears to be about as fast as using a for loop in the same way. However, I'm not sure how well this would scale as the lists which I am working with could increase quite dramatically.

I'm using LINQ as part of a collision detection function (which is still in the works) and I'm using it to cull the list to only the ones that are relevant for the checks.

Here is the LINQ version:

partial class Actor {
    public virtual bool checkActorsForCollision(Vector2 toCheck) {
        Vector2 floored=new Vector2((int)toCheck.X, (int)toCheck.Y);

        if(!causingCollision) // skip if this actor doesn't collide
            return false;

        foreach(
            Actor actor in
            from a in GamePlay.actors
            where a.causingCollision==true&&a.isAlive
            select a
            )
            if( // ignore offscreen collisions, we don't care about them
                (actor.location.X>GamePlay.onScreenMinimum.X)
                &&
                (actor.location.Y>GamePlay.onScreenMinimum.Y)
                &&
                (actor.location.X<GamePlay.onScreenMaximum.X)
                &&
                (actor.location.Y<GamePlay.onScreenMaximum.Y)
                )
                if(actor!=this) { // ignore collisions with self
                    Vector2 actorfloor=new Vector2((int)actor.location.X, (int)actor.location.Y);

                    if((floored.X==actorfloor.X)&&(floored.Y==actorfloor.Y))
                        return true;
                }

        return false;
    }
}

This is my previous method:

partial class Actor {
    public virtual bool checkActorsForCollision(Vector2 toCheck) {
        Vector2 floored=new Vector2((int)toCheck.X, (int)toCheck.Y);

        if(!causingCollision) // skip if this actor doesn't collide
            return false;

        for(int i=0; i<GamePlay.actors.Count; i++)
            if( // ignore offscreen collisions, we don't care about them
                (GamePlay.actors[i].location.X>GamePlay.onScreenMinimum.X)
                &&
                (GamePlay.actors[i].location.Y>GamePlay.onScreenMinimum.Y)
                &&
                (GamePlay.actors[i].location.X<GamePlay.onScreenMaximum.X)
                &&
                (GamePlay.actors[i].location.Y<GamePlay.onScreenMaximum.Y)
                )
                if( // ignore collisions with self
                    (GamePlay.actors[i].isAlive)
                    &&
                    (GamePlay.actors[i]!=this)
                    &&
                    (GamePlay.actors[i].causingCollision)
                    ) {
                    Vector2 actorfloor=
                        new Vector2(
                            (int)GamePlay.actors[i].location.X,
                            (int)GamePlay.actors[i].location.Y
                            );

                    if((floored.X==actorfloor.X)&&(floored.Y==actorfloor.Y))
                        return true;
                }

        return false;
    }
}

At the minute, either run in almost no time (but run numerous times a second), but as the project builds and gets more intricate, this will be dealing with far more objects at once and the code to check for collisions will be more detailed.

Answer 1

Your code looks pretty good; I'm not a big fan of changing working code, but if you did want to rewrite it to be easier to read, here's what I would do:

First, abstract away the predicate "is off the screen". Perhaps make it a method of GamePlay. This business of checking every time whether the coordinates are in the bounds is (1) an implementation detail, and (2) making your code hard to read. It is possible that in the future you will have some more sophisticated mechanism for deciding whether an object is on the screen or not.

Second, abstract away the vector flooring operation. Perhaps make it a method of Vector. Note that this method should return a new vector, not mutate the existing vector.

Third, make an equality operator on vectors.

Fourth, name the method better. A predicate should have the form "IsFoo" or "HasFoo". You've phrased it as a command, not as a question.

Fifth, you don't need a loop at all.

Sixth, it is strange to say somebool == true. Just say somebool. The former means "if it is true that this bool is true", which is needlessly complicated.

Let's see how this shakes out:

public virtual bool HasCollisionWithAnyActor(Vector2 toCheck)
{
    // "Easy out": if this actor does not cause collisions then
    // we know that it is not colliding with any actor.
    if (!causingCollision)
      return false;

    Vector2 floored = toCheck.Floor();

    var collidingActors = 
      from actor in GamePlay.actors
      where actor != this
      where actor.causingCollision
      where actor.isAlive
      where GamePlay.IsOnScreen(actor.location)
      where actor.location.Floor() == floored
      select actor;

    return collidingActors.Any();
}

Look at how much easier that reads than your version of the method! None of this messing around with X and Y coordinates. Make helper methods do all that scutwork. The code now clearly expresses the semantics: tell me whether there are any collisions with other living, collision-causing actors on the screen.

Answer 2

Here LINQ version is faster than the previous one because you have forgotten to create a local variable to store the GamePlay.actors[i]. Then, access to the actors array if done a lot of times in the for loop.

public virtual bool checkActorsForCollision(Vector2 toCheck)
{
    Vector2 floored = new Vector2((int) toCheck.X, (int) toCheck.Y);

    if (!causingCollision) // skip if this actor doesn't collide
    return false;

    for (int i = 0; i < GamePlay.actors.Count; i++)
    {
        Actor actor = GamePlay.actors[i];
        // ignore offscreen collisions, we don't care about them
        if ((actor.location.X > GamePlay.onScreenMinimum.X) &&
            (actor.location.Y > GamePlay.onScreenMinimum.Y) &&
            (actor.location.X < GamePlay.onScreenMaximum.X) &&
            (actor.location.Y < GamePlay.onScreenMaximum.Y))
        {

            if ((actor.isAlive) && (actor != this) &&
                (actor.causingCollision)) // ignore collisions with self
            {
                Vector2 actorfloor =
                  new Vector2((int) actor.location.X,
                              (int) actor.location.Y);
                if ((floored.X == actorfloor.X) &&
                    (floored.Y == actorfloor.Y))
                  return true;
            }
        }
    }
    return false;
}

Now, LINQ as very good performancies in general. But there are some cases where it's better to use the classical for version. You have to find the right balance between the lisibility of your code and the performances (by comparing LINQ version and for version). For my part, I use and abuse of LINQ because I really like its syntax and its lisibility.

Here is a complete LINQ version:

return (from actor in GamePlay.actors
        where actor.causingCollision && a.isAlive
        where actor != this
        where (actor.location.X > GamePlay.onScreenMinimum.X) &&
              (actor.location.Y > GamePlay.onScreenMinimum.Y) &&
              (actor.location.X < GamePlay.onScreenMaximum.X) &&
              (actor.location.Y < GamePlay.onScreenMaximum.Y)
        select new Vector2((int)actor.location.X,
                           (int)actor.location.Y)).Any(actorfloor => (floored.X == actorfloor.X) &&
                                                                     (floored.Y == actorfloor.Y));

Answer 3

I don't see any problem with using LINQ here. If you want to really know if you are increasing the performance you should do some meassuring with Diagnostics.StopWatch

http://msdn.microsoft.com/en-us/library/system.diagnostics.stopwatch.elapsedticks.aspx

Also, you can use even more LINQ like this to make the function a bit more compact.

return (from actor in (from a in GamePlay.actors
                                   where a.CausingCollision == true && a.IsAlive
                                   select a)
                    where (actor.location.X > GamePlay.onScreenMinimum.X) && (actor.location.Y > GamePlay.onScreenMinimum.Y) && (actor.location.X < GamePlay.onScreenMaximum.X) && (actor.location.Y < GamePlay.onScreenMaximum.Y)
                    where actor != this
                    select new Vector2((int) actor.location.X, (int) actor.location.Y)).Any(actorfloor => (floored.X == actorfloor.X) && (floored.Y == actorfloor.Y));