Finding a gameObject on a screen

Question

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

Gallant · Accepted Answer · 2015-07-21 15:26:56Z

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

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. — Funlamb, CommentedJul 21, 2015 at 17:15
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)). — Gallant, CommentedJul 21, 2015 at 20:20
Is there a reason you are calculating the RectangleF warningBoundary every call to FindAsteroidWarningSide()? The screen stays the same throughout the whole game. — Funlamb, CommentedJul 22, 2015 at 17:12
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. — Gallant, CommentedJul 22, 2015 at 17:41

Community · Accepted Answer · 2017-04-13 12:40:47Z

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

Stack Exchange Network

Finding a gameObject on a screen

2 Answers 2

Hot Network Questions

Finding a gameObject on a screen

2 Answers 2

Related

Hot Network Questions