HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Finding a gameObject on a screen

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
unity3dcodereviewcsharpstackoverflowc#
screengameobjectfinding

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:

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.

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.XMax)
    {
        x = warningBoundary.Right;
    }

    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.XMax) ? warningBoundary.Right
        : gameObject.transform.position.x;

    float y = (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.

Code Snippets

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;
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 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);
}
if (warningObject.activeInHierarchy == true)
{
    warningObject.transform.position = FindAsteroidWarningSide();

    if (NearOffScreenWarning.nearOffScreenWarningSingleton.CheckIfInScreen(gameObject))
    {
        warningObject.SetActive(false);
        warning = false;
    }
}
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);
}

Context

StackExchange Code Review Q#97542, answer score: 6

Revisions (0)

No revisions yet.