Discussion OverlapSphere results return, VS suggestion not making sense

So I made a function, to use OverlapSphere, and get all nearby objects(classes) within a set radius:

Code (CSharp):

1. public List<Hex> RadiusSearchForHex(Hex hex, float radius)
2.     {
3.         List<Hex> list = new List<Hex>();
4.         foreach (var collider in Physics.OverlapSphere(hex.worldPos, radius))
5.         {
6.             if (collider.transform.parent.TryGetComponent<Hex>(out Hex nearby))
7.             {
8.                 if (nearby != hex && nearby.myType == Hex.TypeOfHex.Land)list.Add(nearby);
9.             }
10.         }
11.         return list;
12.     }

And I was happy with it, yet Visual Studio did it's common "this can be improved" suggestion, so for the giggles of it I let it try to simplify it:

Code (CSharp):

1. public List<Hex> RadiusSearchForHex2(Hex hex, float radius)
2.     {
3.         return (Physics.OverlapSphere(hex.worldPos, radius).
4.             Where(collider => collider.transform.parent.TryGetComponent<Hex>(out Hex nearby)).
5.             Where(collider => nearby != hex &&
6.             nearby.myType == Hex.TypeOfHex.Land).Select(collider => nearby)).ToList();
7.     }

It gives errors on each "nearby" on second "Where", and I tried playing around with fixing it, but it seems the "Where" methods only wish to use the "collider" reference, and cannot see that "nearby" was declared. Which to my own conclusion, I would have to GetComponent from the collider several times, in order to keep the suggested code.

I will admit, the lamda(=>) operator still throws me for a loop sometimes, but I think it's more the "Where" in this situation since it seems purely focused on the returned colliders, and doesn't register "nearby".

I've pretty much given up on this, and plan to stick to my original method. But for the giggles of it, is anyone able to say for sure that the suggested code would never work the way I intend it to?(if there's no real performance gain, especially)

 

It wasn‘t clear: does the refactored code work or does it not?
Because as far as I can tell by looking at it, it should work exactly the same.
But whether this is an „optimization“ or Microsoft just wishing to educate devs on the use of LINQ … I tend to favor the latter argument.

The optimization is more likely to be detrimental to runtime performance due to the use of LINQ, although sometimes it can be surprisingly the fastest solution. Only the profiler can tell.

Personally, I don‘t like this particular LINQ optimization because I find that code to be denser but more convoluted, and it‘s terrible for debugging because of it being a single return statement so if it ever goes wrong, it‘s more difficult to find out where exactly and for what reason it fails. At least I prefer to step through a loop, and be able to add some Debug.Log wherever I please.

I‘m also suprised why VS did not implement the obvious return expression syntax:

Code (CSharp):

1. public List<Hex> RadiusSearchForHex2(Hex hex, float radius) =>
2.     (Physics.OverlapSphere(hex.worldPos, radius).
3.             Where(collider => collider.transform.parent.TryGetComponent<Hex>(out Hex nearby)).
4.             Where(collider => nearby != hex &&
5.             nearby.myType == Hex.TypeOfHex.Land).Select(collider => nearby)).ToList();

 

Ohh sorry!

I meant the "nearby" declaration isn't seen by the "Where" method. "Where" only checks the IEnumerable of colliders within the array of OverlapSphere. And I tried multiple things, declaring

Hex nearby;

before the return, tried changing the "Where(nearby"(which is still a collider), and still same error.

So like I assume, if it only references the collider, I would have to GetComponent on each of those "collider"

 

EDIT: it is using Linq, the suggestion must have put it in it

 

That is my thought as well, if for whatever reason later on I wish to add more checks, then that return would be even longer than the first way of doing it, lol..

I was just checking if there was some super simple cool way of doing it, but overall it makes no sense to go that far. As I always say, Rule#1 is understanding your own code!

 

Yeah, the suggestions are not always the best (in VSCode in my case but I think they're from the same analyzers) and VS also doesn't know how the code might evolve. That's why they're "suggestions" and not warnings or errors, because they might not be always the appropriate solution.

The TryGetComponent creates a local variable, which is local to the block it's contained in. In the Linq-variant that's the implicit block of the lambda, so the variable can only be accessed inside of that one lambda, not in any of the others.

The straightforward fix would be to declare the variable in the method block, so it can be captured in all three lambdas:

Code (CSharp):

1. public List<Hex> RadiusSearchForHex2(Hex hex, float radius)
2. {
3.     Hex nearby = null;
4.     return (Physics.OverlapSphere(hex.worldPos, radius).
5.         Where(collider => collider.transform.parent.TryGetComponent<Hex>(out nearby)).
6.         Where(collider => nearby != hex &&
7.         nearby.myType == Hex.TypeOfHex.Land).Select(collider => nearby)).ToList();
8. }

A nicer use of Linq would be to first use Select with GetComponent and then pass the Hex reference through the query. You also want to return IEnumerable instead of creating a new List each time:

Code (CSharp):

1. public IEnumerable<Hex> RadiusSearchForHex2(Hex hex, float radius)
2. {
3.     return Physics.OverlapSphere(hex.worldPos, radius).
4.         Select(collider => collider.transform.parent.GetComponent<Hex>()).
5.         Where(nearby => nearby != null && nearby != hex && nearby.myType == Hex.TypeOfHex.Land);
6. }

 

This has several potential issues. Since the OverlapSphere may return all sorts of colliders, what if there's a collider object on a root object in the scene? In that case this line would through a null reference exception since parent would be null. So I would probably use GetComponentInParent on the collider instead. Just like GetComponentInChildren, GetComponentInParent also starts searching at the current level. So if there's a Hex component on the object with the collider it would return that first and only goes up the hierarchy if none was found.

 

Yeah I noticed that was the case, and the "Where" was only seeing colliders.

And I did try this, except for saying

= null;

on nearby;

Code (CSharp):

1. public List<Hex> RadiusSearchForHex2(Hex hex, float radius)
2. {
3.     Hex nearby = null;
4.     return (Physics.OverlapSphere(hex.worldPos, radius).
5.         Where(collider => collider.transform.parent.TryGetComponent<Hex>(out nearby)).
6.         Where(collider => nearby != hex &&
7.         nearby.myType == Hex.TypeOfHex.Land).Select(collider => nearby)).ToList();
8. }

And it still didn't work, the "nearby"s still had error marks on them(from not declaring it null... lol). As far as that other second snippet, I'll test that.

It's good, the only colliders in the game are a child of the main that purely are just a properly scaled "Hex" collider. Trying to put the collider on the parent(root) object, was very tiny since my Blender.fbx scale translates to 100 in Unity, and a parent at 1 makes that practically invisible, lol.. So I just made a separate child, just for raycast collisions, and scaled it to correct size.

 

oof! apparently I jumped the gun, and didn't notice the error, it was the "not setting something to null/0" error...
::face palm::

wow, I really gotta pay more attention at times.