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.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.
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;
}
}