7
\$\begingroup\$

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;
    }
}
\$\endgroup\$

2 Answers 2

6
\$\begingroup\$

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);
}
\$\endgroup\$
4
  • \$\begingroup\$ Thanks for the response. I'm still wrapping my head around it. I fixed the bug you mentioned and found another in the process. While testing the functionality I noticed that I need to implement one more thing. When an asteroid is say moving from the top of the screen to the bottom it will begin warning at the top. Then it will move along the screen with no warning. Then start warning again at the bottom of the screen when the asteroid wont be an issue til it wraps around again to the top portion of the screen. What's a good way to implement a velocity check? Maybe I'll resubmit a new review. \$\endgroup\$
    – Funlamb
    Commented Jul 21, 2015 at 17:15
  • \$\begingroup\$ Assuming the edge of your game is (0,0), I would check if the signage (+/-) of the asteroid's velocity matches the signage of its position to determine if it's moving toward or away from the game (e.g., if velocity.X is negative and position.X is negative, you wouldn't want to show the warning). A trick to check if the signage matches for both positive/negative would be if (Math.Abs(velocity.X + position.X) == Math.Abs(velocity.X) + Math.Abs(position.X)). \$\endgroup\$
    – Gallant
    Commented Jul 21, 2015 at 20:20
  • \$\begingroup\$ Is there a reason you are calculating the RectangleF warningBoundary every call to FindAsteroidWarningSide()? The screen stays the same throughout the whole game. \$\endgroup\$
    – Funlamb
    Commented Jul 22, 2015 at 17:12
  • \$\begingroup\$ Ideally it shouldn't be calculated every call; I just didn't have a reasonable place to put it. You should definitely instantiate it only once. \$\endgroup\$
    – Gallant
    Commented Jul 22, 2015 at 17:41
5
\$\begingroup\$
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;
    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.