How I handled the challenge, and preventing spawning defenders on top of each other

Unity Courses Talk
,
#1

Any feedback is appreciated.

 public Vector2 mousePosition;
private Defender[] takenPositions;

private void OnMouseDown()
{
    mousePosition = Camera.main.ScreenToWorldPoint(Input.mousePosition);
    mousePosition.x = Mathf.Round(mousePosition.x);
    mousePosition.y = Mathf.Round(mousePosition.y);
    
    if (IsPositionOpen(mousePosition) && Button.selectedDefender)
    {
        Instantiate(Button.selectedDefender, mousePosition, Quaternion.identity);
    }
}

private bool IsPositionOpen(Vector2 mousePosition)
{
    takenPositions = GameObject.FindObjectsOfType<Defender>();

    foreach (Defender component in takenPositions)
    { Vector2 convert2dPosition = new Vector2(component.transform.position.x, component.transform.position.y);
        if (convert2dPosition == mousePosition) {
            return false;
    }
    }
    return true;
}
#2

Hi Gunaveh,

Good job on developing a solution. Looking for all Defenders in the scene to determine if the field is free makes sense. :slight_smile:

Unfortunately, the Find methods are fairly slow, so it’s recommended to avoid them whenever possible. Since we parent our spawned Defenders, we could simply iterate over the transform of the parent and check the position of the child game objects.

Here is a suggestion:

public Transform parent; // your Defender parent

private bool IsPositionOpen(Vector2 mousePosition) {
    Vector3 childPos;

    for (int i = 0; i < this.parent.childCount; i++) {
        childPos = this.parent.GetChild(i).position;
        
        if (childPos.x == mousePosition.x && childPos.y == mousePosition.y)) {
            return false;
        }
    }

    return true;
}

And a tiny detail: if (Button.selectedDefender && IsPositionOpen(mousePosition))
I would do it this way round because if no button is selected, you don’t want to iterate over the Defenders because if the position was free, you would not be able to place a Defender anyway.

What do you think?

1 Like
#3

I took your advice on iterating thru the parent and swapping the order on the if statement.

I used a foreach loop, if anything to get more practice with them. What do you think?

  public Vector2 mousePosition;
public GameObject parent;

private void OnMouseDown()
{
    mousePosition = Camera.main.ScreenToWorldPoint(Input.mousePosition);
    mousePosition.x = Mathf.Round(mousePosition.x);
    mousePosition.y = Mathf.Round(mousePosition.y);
    
    if (Button.selectedDefender && IsPositionOpen(mousePosition))
    {
        Instantiate(Button.selectedDefender, mousePosition, Quaternion.identity);
    }
}

private bool IsPositionOpen(Vector2 mousePosition)
{
   foreach (Transform component in parent.transform)
    {
        Vector2 convert2dPosition = new Vector2(component.transform.position.x, component.transform.position.y);
        if (convert2dPosition == mousePosition) return false;
    }
    return true;
}
1 Like
#4

Sure, that’s also fine.

In my approach, I tried to use as few variables as possible. However, since a Vector2 is a struct, it does not matter much in a little project like this. I personally prefer the a more “solid” way because in loops with more iteration steps you would unnecessarily put pressure on the garbage collector when economising on resources is crucial. The earlier you learn to pay attention to those details, the better.

Therefore, I would put the declaration of the Vector2 variable outside the foreach loop. Furthermore, the component already is of Type transform. No need to access the transform anew.

private bool IsPositionOpen(Vector2 mousePosition)
{
   Vector2 convert2dPosition = new Vector2 (0f, 0f);

   foreach (Transform component in parent.transform)
    {
        convert2dPosition.x = component.position.x
        convert2dPosition.y = component.position.y;

        if (convert2dPosition == mousePosition) return false;
    }
    return true;
}

Foreach loops take up more resources than for loops. See here and here. However, in this particular example, I don’t know if the foreach loop might be better than the for loop. It highly depends on the speed of GetChild(i) which I call in each iteration step.

By the way, if you do not need a GameObject parent, you could declare it as a Transform. Unity automatically grabs the Transform component when you drag in the game object from your Hierarchy. See here.

#5

Thank you! you saved me from a ton of unnecessary steps. I didn’t even realize how poorly that was written but it makes sense now.

#6

Don’t worry too much about it. Those are just details which actually do not matter in this project. Nobody who is playing your game will ever notice a difference between the original version and the improved one. Your solution is perfectly fine as it is.

I just mentioned them because you asked for feedback, and I thought this might be helpful for future projects.

Happy coding, and, if you celebrate Christmas, happy holidays! :slight_smile:

Privacy & Terms