Finding a gameObject on a screen

Posted on

Problem

I’m working on a warning system for my game. I want to display an arrow on the screen where the object is going to be coming from.

Here is an image of what I have in mind:

enter image description here

The object will spawn from outside the screen and start making its way to the screen. The code that I have created works, but it looks terrible. How can I fix this to make it more readable?

private GameObject warningObject;
private float warningBufferFromScreenEdge = 1f;
public void ShowWarningOnScreen()
{
    if (warning == false)
    {
        if (!NearOffScreenWarning.nearOffScreenWarningSingleton.CheckIfInScreen(gameObject))
        {
            warningObject = ObjectPoolManager.ObjectPoolManagerSingleton.AsteroidWarnings.GetPooledObject();
            if (warningObject != null)
            {
                warningObject.SetActive(true);
                warningObject.transform.position = NearOffScreenWarning.nearOffScreenWarningSingleton.left;
                warning = true;
            }
        }
    }
    else
    {
        if (warningObject.activeInHierarchy == true)
        {
            FindAsteroidWarningSide();
            switch (warningSides)
            {
                case WarningSides.TopLeft:
                    warningObject.transform.position = new Vector2(Boundary.boundarySingleton.XMin + warningBufferFromScreenEdge, Boundary.boundarySingleton.YMax + warningBufferFromScreenEdge);
                    break;
                case WarningSides.Top:
                    warningObject.transform.position = new Vector2(gameObject.transform.position.x, Boundary.boundarySingleton.YMax - warningBufferFromScreenEdge);
                    break;
                case WarningSides.TopRight:
                    warningObject.transform.position = new Vector2(Boundary.boundarySingleton.XMax - warningBufferFromScreenEdge, Boundary.boundarySingleton.YMax - warningBufferFromScreenEdge);
                    break;
                case WarningSides.Right:
                    warningObject.transform.position = new Vector2(Boundary.boundarySingleton.XMin + warningBufferFromScreenEdge, gameObject.transform.position.y);
                    break;
                case WarningSides.BottomRight:
                    warningObject.transform.position = new Vector2(Boundary.boundarySingleton.XMax - warningBufferFromScreenEdge, Boundary.boundarySingleton.YMin + warningBufferFromScreenEdge);
                    break;
                case WarningSides.Bottom:
                    warningObject.transform.position = new Vector2(gameObject.transform.position.x, Boundary.boundarySingleton.YMin + warningBufferFromScreenEdge);
                    break;
                case WarningSides.BottomLeft:
                    warningObject.transform.position = new Vector2(Boundary.boundarySingleton.XMin + warningBufferFromScreenEdge, Boundary.boundarySingleton.YMin + warningBufferFromScreenEdge);
                    break;
                case WarningSides.Left:
                    warningObject.transform.position = new Vector2(Boundary.boundarySingleton.XMin + warningBufferFromScreenEdge, gameObject.transform.position.y);
                    break;
                default:
                    break;
            }
            if (NearOffScreenWarning.nearOffScreenWarningSingleton.CheckIfInScreen(gameObject))
            {
                warningObject.SetActive(false);
                warning = false;
            }
        }
    }
}

enum WarningSides
{
    TopLeft,
    Top,
    TopRight,
    Right,
    BottomRight,
    Bottom,
    BottomLeft,
    Left,

}

private WarningSides warningSides;

private void FindAsteroidWarningSide()
{
    if (transform.position.y > Boundary.boundarySingleton.YMax)
    {
        if (transform.position.x < Boundary.boundarySingleton.XMin)
        {
            warningSides = WarningSides.TopLeft;
        }
        else if (transform.position.x > Boundary.boundarySingleton.XMax)
        {
            warningSides = WarningSides.TopRight;
        }
        else
        {
            warningSides = WarningSides.Top;
        }
    }
    else if (transform.position.x > Boundary.boundarySingleton.XMax)
    {
        if (transform.position.y > Boundary.boundarySingleton.YMin)
        {
            warningSides = WarningSides.Right;
        }
        else if (transform.position.y < Boundary.boundarySingleton.YMin)
        {
            warningSides = WarningSides.BottomRight;
        }
    }
    else if (transform.position.y < Boundary.boundarySingleton.YMin)
    {
        if (transform.position.x > Boundary.boundarySingleton.XMin)
        {
            warningSides = WarningSides.Bottom;
        }
        else if (transform.position.x < Boundary.boundarySingleton.XMin)
        {
            warningSides = WarningSides.BottomLeft;
        }
    }
    else
    {
        warningSides = WarningSides.Left;
    }
}

Solution

I suspect your TopLeft case has an error in it (adding the buffer to the YMax instead of subtracting it).

All the logic going into creating the Vector2 could use a revisit: why should we have to calculate these X and Y offsets for every warning we want to create? It would be much simpler to have a RectangleF (from System.Drawing) with these offsets already calculated for us:

RectangleF warningBoundary = new RectangleF
(
    Boundary.boundarySingleton.XMin + warningBufferFromScreenEdge,
    Boundary.boundarySingleton.YMax - warningBufferFromScreenEdge, // Invert Y-Axis
    Boundary.boundarySingleton.XMax - Boundary.boundarySingleton.XMin - warningBufferFromScreenEdge,
    Boundary.boundarySingleton.YMin - Boundary.boundarySingleton.YMax + warningBufferFromScreenEdge // Invert Y-Axis
);

...

case WarningSides.BottomRight:
    warningObject.transform.position = new Vector2(warningBoundary.Right, warningBoundary.Bottom);
    break;

Unfortunately the RectangleF class treats (0,0) as the Top-left corner, whereas your boundary appears to treat it as the Bottom-left corner, so the creation of the rectangle requires the Y-axis to be inverted.


I think you fell into a trap by using an enum for your warning sides, however. By pairing the X and Y coordinates as enumerations, you exponentiate the number of states you are required to check (3*3). Ultimately, you’re using FindAsteroidWarningSide to calculate your Vector2, so why not just have that method return the Vector2 directly? By treating the X and Y coordinates separately, the logic also simplifies:

private Vector2 FindAsteroidWarningSide()
{
    float x = gameObject.transform.position.x;
    float y = gameObject.transform.position.y;

    RectangleF warningBoundary = new RectangleF
    (
        Boundary.boundarySingleton.XMin + warningBufferFromScreenEdge,
        Boundary.boundarySingleton.YMax - warningBufferFromScreenEdge, // Invert Y-Axis
        Boundary.boundarySingleton.XMax - Boundary.boundarySingleton.XMin - warningBufferFromScreenEdge,
        Boundary.boundarySingleton.YMin - Boundary.boundarySingleton.YMax + warningBufferFromScreenEdge // Invert Y-Axis
    );

    if (transform.position.x < Boundary.boundarySingleton.XMin)
    {
        x = warningBoundary.Left;
    }
    else if (transform.position.x > Boundary.boundarySingleton.XMax)
    {
        x = warningBoundary.Right;
    }

    if (transform.position.y < Boundary.boundarySingleton.YMin)
    {
        y = warningBoundary.Bottom;
    }
    else if (transform.position.y > Boundary.boundarySingleton.YMax)
    {
        y = warningBoundary.Top;
    }

    return new Vector2(x, y);
}

While I’m not particularly fond of nested ternaries, if you were going for brevity you could shorten the above considerably:

private Vector2 FindAsteroidWarningSide()
{
    RectangleF warningBoundary = new RectangleF
    (
        Boundary.boundarySingleton.XMin + warningBufferFromScreenEdge,
        Boundary.boundarySingleton.YMax - warningBufferFromScreenEdge, // Invert Y-Axis
        Boundary.boundarySingleton.XMax - Boundary.boundarySingleton.XMin - warningBufferFromScreenEdge,
        Boundary.boundarySingleton.YMin - Boundary.boundarySingleton.YMax + warningBufferFromScreenEdge // Invert Y-Axis
    );

    float x = (transform.position.x < Boundary.boundarySingleton.XMin) ? warningBoundary.Left
        : (transform.position.x > Boundary.boundarySingleton.XMax) ? warningBoundary.Right
        : gameObject.transform.position.x;

    float y = (transform.position.y < Boundary.boundarySingleton.YMin) ? warningBoundary.Bottom
        : (transform.position.y > Boundary.boundarySingleton.YMax) ? warningBoundary.Top
        : gameObject.transform.position.y;

    return new Vector2(x, y);
}

Now we can get rid of that switch statement and enum altogether and just call the method directly:

if (warningObject.activeInHierarchy == true)
{
    warningObject.transform.position = FindAsteroidWarningSide();

    if (NearOffScreenWarning.nearOffScreenWarningSingleton.CheckIfInScreen(gameObject))
    {
        warningObject.SetActive(false);
        warning = false;
    }
}

The final code:

private GameObject warningObject;
private float warningBufferFromScreenEdge = 1f;
public void ShowWarningOnScreen()
{
    if (warning == false)
    {
        if (!NearOffScreenWarning.nearOffScreenWarningSingleton.CheckIfInScreen(gameObject))
        {
            warningObject = ObjectPoolManager.ObjectPoolManagerSingleton.AsteroidWarnings.GetPooledObject();
            if (warningObject != null)
            {
                warningObject.SetActive(true);
                warningObject.transform.position = NearOffScreenWarning.nearOffScreenWarningSingleton.left;
                warning = true;
            }
        }
    }
    else
    {
        if (warningObject.activeInHierarchy == true)
        {
            warningObject.transform.position = FindAsteroidWarningSide();

            if (NearOffScreenWarning.nearOffScreenWarningSingleton.CheckIfInScreen(gameObject))
            {
                warningObject.SetActive(false);
                warning = false;
            }
        }
    }
}

private Vector2 FindAsteroidWarningSide()
{
    float x = gameObject.transform.position.x;
    float y = gameObject.transform.position.y;

    RectangleF warningBoundary = new RectangleF
    (
        Boundary.boundarySingleton.XMin + warningBufferFromScreenEdge,
        Boundary.boundarySingleton.YMax - warningBufferFromScreenEdge, // Invert Y-Axis
        Boundary.boundarySingleton.XMax - Boundary.boundarySingleton.XMin - warningBufferFromScreenEdge,
        Boundary.boundarySingleton.YMin - Boundary.boundarySingleton.YMax + warningBufferFromScreenEdge // Invert Y-Axis
    );

    if (transform.position.x < Boundary.boundarySingleton.XMin)
    {
        x = warningBoundary.Left;
    }
    else if (transform.position.x > Boundary.boundarySingleton.XMax)
    {
        x = warningBoundary.Right;
    }

    if (transform.position.y < Boundary.boundarySingleton.YMin)
    {
        y = warningBoundary.Bottom;
    }
    else if (transform.position.y > Boundary.boundarySingleton.YMax)
    {
        y = warningBoundary.Top;
    }

    return new Vector2(x, y);
}

private float warningBufferFromScreenEdge = 1f;  

this should be readonly because you are only reading the value and never changing it.


On top of @Gallant‘s answer:

if (warning == false)

as warning is a bool you should either use the ! opertator or just check if(warning) and reverse the programm flow. In addition this

 if (warningObject.activeInHierarchy == true)  

would be better expressed like so

if (warningObject.activeInHierarchy)  

By returning early you can reduce the horizontal spacing which adds readability to your code.

By storing the result of NearOffScreenWarning.nearOffScreenWarningSingleton.CheckIfInScreen() in a variable you will reduce some code duplication.

Implementing the mentioned points above would lead to

public void ShowWarningOnScreen()
{
    bool isInScreen = NearOffScreenWarning.nearOffScreenWarningSingleton.CheckIfInScreen(gameObject);
    if (warning && warningObject.activeInHierarchy)
    {
        warningObject.transform.position = FindAsteroidWarningSide();

        if (isInScreen)
        {
            warningObject.SetActive(false);
            warning = false;
        }
        return;
    }

    if (!isInScreen) { return; }

    warningObject = ObjectPoolManager.ObjectPoolManagerSingleton.AsteroidWarnings.GetPooledObject();
    if (warningObject != null)
    {
        warningObject.SetActive(true);
        warningObject.transform.position = NearOffScreenWarning.nearOffScreenWarningSingleton.left;
        warning = true;
    }
}

Leave a Reply

Your email address will not be published. Required fields are marked *