GetValidActionGridPositionList - Different Approach

Unity Courses Show
,
#1

I went with smth a bit different, because nested for loops make my eyes bleed :slight_smile:

            public List<GridPosition> GetValidActionGridPositionList() {
                GridPosition unitGridPosition = _targetUnit.GetGridPosition();

                bool IsValidPosition(GridPosition position) {
                    LevelGrid levelGrid = LevelGrid.Instance;

                    return levelGrid.IsValidGridPosition(position) &&
                        !levelGrid.HasAnyUnitOnGridPosition(position) &&
                        unitGridPosition != position;
                }

                int startIndex = -_maxMoveDistance;
                int countPositionsToGenerate = _maxMoveDistance * 2 + 1;

                IEnumerable<int> valueRange = Enumerable.Range(startIndex, countPositionsToGenerate);

                return valueRange
                    .SelectMany(x => valueRange.Select(
                            y => new GridPosition(x, y) + unitGridPosition
                        )
                    ).ToList()
                     .FindAll(IsValidPosition);
            }
2 Likes
#2

Good code! Have you tested your code?

#3

Nested for loops make your eyes bleed, but a SQL statement disguised as a Linq expression comes naturally. I love it!

Only caveat I can see is that this will include the _targetUnit.GetGridPosition in the results (of course, that position will already be in the closed list, so no harm, no foul. You could include a .Where() to filter it out.

1 Like
#4

Maybe it’s the way it is formatted or something, but I’m not a fan. I love Linq and I also dislike nested for loops, but this is just too hard to read. If you’re the only person that has to live with this, then you’re good. It took a little effort to see that the Select was inside the SelectMany so at first it made no sense to me how that would work. Kudos for doing it, though.

1 Like
#5

SQL does come very naturally for me :slight_smile:

The FindAll(IsValidPosition) will be filtering out all invalid positions, including the units own position

#6

Yeah live tested in Unity, works well for me. No Integration tests tho (although I wanna cover that separately if not covered by the main lessons)

Have you found a bug perhaps?

#7

Yeah its the most readable I could make it, It still come easier me to read than nested for loops

Ideally this would look cleaner though but aside from writting a custom module to do that, this is the best I can do atm

#8

I’m not seeing any bugs, just the potential that the source location is included (which, as I said, would be filtered out anyways). It did take me a few passes to work through the query, so my only other concern would be maintaining it down the road. As long as you can read it and see the execution flow, it should be fine, but if you were in a larger project, it might get some odd looks from non SQL programmers on the team. :slight_smile:

1 Like
#9

It’s fine. I would maybe have formatted it a little different - perhaps some indents to show scope.

I just work with a lot of interns and junior developers and have learned the value of readability. A few years ago I would’ve been writing very similar code and smiled, but now I would tolerate a little eye-blood if it meant my juniors could easily read it.

2 Likes
#10

These are useful comments! Def must keep in mind that even if it reads well to me, others may feel differently!

#11

To be clear, I’m not knocking the use of Linq in general, it’s a powerful tool (just don’t use it every frame for complex querie). I use Linq all the time.

One thing I’ve learned over 40 years of coding is the importance of readability, and self-documenting code. For example, this may be both a more performant and more readable GetNeighbors method… This is from my GridSystem.cs:

       private GridPosition[] neighbors = new[]
                                       {
                                           new GridPosition(-1, 0),
                                           new GridPosition(-1, -1),
                                           new GridPosition(-1, 1),
                                           new GridPosition(1, 0),
                                           new GridPosition(1, -1),
                                           new GridPosition(1, 1),
                                           new GridPosition(0, 1),
                                           new GridPosition(0, -1);
                                       };

    public IEnumerable<GridPosition> GetValidNeighbors(GridPosition position)
    {
        return neighbors.Select(neighbor => position + neighbor).Where(neighbor => IsValidGridPosition(neighbor));
    }

It does lack some elegance, in that I hardcoded the values, but even a junior coder should be able to get a quick grasp of what’s going on in the method. (It’s also less expensive, with two copies of neighbors being placed on the heap in need of garbage collection vs four (not counting the ToList() and FindAll).

2 Likes
#12

yeah Ive been reading about Linq and it seems the general consensus is that its performance is not as great as using simple for loops.

One way I can think of improving performance would be to memoize the results of this function?

#13

It actually got a lot better. The problem is not so much the performance of the actual query. It’s the garbage that it creates and garbage collection is heavy. That’s why we don’t want to do complex Linq on every frame (like @Brian_Trotter mentioned). At some point the GC will start freezing frames because it just has too much garbage to collect

#15

Any way we can manage that with Unity? Or Linq is just a big NoNo?

#16

You’re ok with Linq as long as it isn’t in Updates, i.e. in an event driven manner.

When I first started using Linq expressions in Unity, I tried to be clever, using Linq in an AI detection scheme… the NPCs all had Factions, and some factions were friendly to other factions, and some were hostile… and the Player was just in the faction Player… so my query was

public IEnumerable<CombatTarget> GetEnemiesWithinRange(FindObjectsOfType<CombatTarget>().
                               Where(d=>Vector3.Distance(d.transform.position, transform.position)<range).
                               Where(f=>f.faction.IsHostile(faction)).
                               Where(h=>h.GetComponent<Health>().IsAlive()).
                               OrderBy(d=>Vector3.Distance(d.transform.position, transform.position);

This actually worked just fine in testing… 4 enemies all started bashing each others brains in as soon as they encountered each other. Then I put it in a procedurally generated world with 100 AIs of varying factions, and the world ended, frame rate 1fps, thank you for playing. :slight_smile:

3 Likes
#17

How often (or from where) were you calling that GetEnemiesWithinRange()?
Each Update()? :skull:
That FindObjectsOfType<CombatTarget>() alone would tank the performance, I suppose…

#18

I was doing it in Update(). :skull: And I should have known better.

1 Like
#19

Just want to say these comments on each video are just as interesting in the video itself, thanks folks for the random brain food to digest :+1:

Privacy & Terms