Bug Problem with Color changing Code

Hello,

so i'm currently working on a fairly simple game(considering i'm a beginner). I implemented some core mechanics of the game and tested them out to be working fine, however now, i've sort of caught a bug myself.

Overview of the game,
so basically you control a sphere moving it left and right, and you can cycle between its colors by pressing space bar, the objective is to hit the oncoming obstacles of the three colors correctly by switching your player color and controlling the movement. I've added a mechanic to switch between red, green and blue. For now the oncoming obstacles i used were only in red color and after their collision with the player they were being deleted as per intended outcome, and if you switch the player color and then hit the obstacles a game over message displays. I've now noticed that once i cycle between the three colors even once, lets say i'm at red then switch to green then blue and back to red, then if i collide with a red obstacle, game over message displays instead of that object being deleted.

I'm posting my code as well so everyone can have a better idea.

using System.Collections;
using System.Collections.Generic;
using UnityEngine;

public class PlayerController : MonoBehaviour
{
public float speed = 10f;
Rigidbody playerRb;
private float horizontalBound = 23f;
private bool gameOver = false;
bool red = true;
bool blue;
bool green;
public GameObject obstaclePrefab;
public GameObject powerUp;

// Start is called before the first frame update
void Start()
{
playerRb = GetComponent<Rigidbody>(); // initializing the rigid body
}

// Update is called once per frame
void Update()
{

MovePlayer();
ConstrainPlayerMovement();
ChangeColor();

}

//private void RemoveAllForces()
//{
// gameObject.SetActive(false);
// gameObject.SetActive(true);
//}

private void ConstrainPlayerMovement()
{
if (transform.position.x > horizontalBound)
{
transform.position = new Vector3(horizontalBound, transform.position.y, transform.position.z);
if (transform.position.x >= horizontalBound)
{
playerRb.velocity = Vector3.zero;
}

}

if (transform.position.x < -horizontalBound)
{
transform.position = new Vector3(-horizontalBound, transform.position.y, transform.position.z);
if (transform.position.x <= -horizontalBound)
{
playerRb.velocity = Vector3.zero;
}
}
}

// moves the player
void MovePlayer()
{
float horizontalInput = Input.GetAxis("Horizontal");
playerRb.AddForce(Vector3.right * speed * horizontalInput, ForceMode.VelocityChange);
}


// checking for collisions with same colour, if not the same, game is over.
private void OnCollisionEnter(Collision collision)
{
if(collision.gameObject.CompareTag("Obstacle"))
{
if(collision.gameObject.GetComponent<MeshRenderer>().material.color == gameObject.GetComponent<MeshRenderer>().material.color)
{
Destroy(collision.gameObject);
}
else if
(collision.gameObject.GetComponent<MeshRenderer>().material.color != gameObject.GetComponent<MeshRenderer>().material.color)
{
GameOver();
}
}
}

private void GameOver()
{
gameOver = true;
Debug.Log("Game Over you lost");
}

// code to cycle between 3 colors
private void ChangeColor()
{
if (Input.GetKeyDown(KeyCode.Space))
{
if (red)
{
GetComponent<MeshRenderer>().material.color = Color.green;
green = true;
red = false;
blue = false;
}
else if (green)
{
GetComponent<MeshRenderer>().material.color = Color.blue;
blue = true;
green = false;
red = false;
}
else if (blue)
{
GetComponent<MeshRenderer>().material.color = Color.red;
red = true;
blue = false;
green = false;
}
}
}

private void OnTriggerEnter(Collider other)
{
if(other.gameObject.CompareTag("PowerUp"))
{
Color newSpawnColor = other.gameObject.GetComponent<MeshRenderer>().material.color;
Destroy(other.gameObject);
// functionality to spawn obstacles of power up's color to be added.

}
}

}

 

Hi, please and take some time to edit your post and use code tags. We can't really read a soup of letters like we would read code.

 

This is likely because you're comparing the colors directly. Though I'm not sure, I think there are several things combining into a faulty state.

You're rigorously maintaining your three bools: red, green, blue, and then you use that only for the switching logic. Which is weird, why would you maintain 3 separate values, when for a cycle you can maintain just one?

Here's how you should do this, on a conceptual level, you keep track of your internal model. You don't care about the colors per se, it doesn't matter if something is bluish or purple or violet, that's the job for some renderer, not your business logic. So stop comparing colors, and start comparing identities.

Make sure you have a separate file (ColourIdentity.cs) with this somewhere

Code (csharp):

1. using UnityEngine;
2.  
3. public struct ColourIdentity {
4.  
5.   private const int MAX_COLOURS = 3;
6.  
7.   int _index;
8.   public int Index {
9.     get { return _index; }
10.     set {
11.       if(value < 0 || value >= MAX_COLOURS) throw new System.ArgumentOutOfRangeException();
12.       _index = value;
13.     }
14.   }
15.  
16.   public ColourIdentity(int index = 0) : this() {
17.     Index = index;
18.   }
19.  
20.   public void CycleForward() {
21.     if(++_index == MAX_COLOURS) _index = 0;
22.   }
23.  
24.   public Color GetAsColour()
25.     => _index switch() {
26.          0 => Color.red,
27.          1 => Color.green,
28.          2 => Color.blue,
29.          _ => throw new System.ArgumentOutOfRangeException();
30.        }
31.  
32. }

Code (csharp):

1. // checking for collisions with same colour, if not the same, game is over.
2. void OnCollisionEnter(Collision collision) {
3.   GameObject go = collision.gameObject;
4.  
5.   bool isObstacle = go.CompareTag("Obstacle");
6.   bool isPowerUp = go.CompareTag("PowerUp");
7.  
8.   ColourObject colourObj = go.GetComponent<ColourObject>();
9.   if(colourObj == null) return; // this isn't an object we're supposed to react to
10.  
11.   bool sameColor = colourObj.Colour.Index == selectedColour.Index;
12.  
13.   if(isPowerUp) {
14.     if(sameColor) {
15.       // ActivatePowerUp(go, colourObj.Colour);
16.     } else {
17.       Destroy(go);
18.     }
19.     return;
20.   }
21.  
22.   if(isObstacle) {
23.     if(sameColor) {
24.       Destroy(go);
25.       return;
26.     }
27.   }
28.  
29.   GameOver();
30. }

Code (csharp):

1. private ColourIdentity selectedColour = new ColourIdentity();
2.  
3. // code to cycle between 3 colors
4. void ChangeColor() {
5.   if(Input.GetKeyDown(KeyCode.Space)) { // this test doesn't belong here, but I'm keeping it the same
6.     selectedColour.CycleForward();
7.     GetComponent<MeshRenderer>().sharedMaterial.color = selectedColour.GetAsColour();
8.   }
9. }

Add this to your obstacles and power ups when you instantiate/spawn them, you may use AddComponent for that.

Code (csharp):

1. using UnityEngine;
2.  
3. public class ColourObject : MonoBehaviour {
4.  
5.   private ColourIdentity _colour;
6.   public ColourIdentity Colour {
7.     get => _colour;
8.     set {
9.       _colour = value;
10.       GetComponent<MeshRenderer>().material.color = _colour.GetAsColour();
11.     }
12.   }
13.  
14. }

Make sure to set the colour after adding

Code (csharp):

1. var colourObj = go.AddComponent<ColourObject>();
2. colourObj.Colour = new ColourIdentity(0); // or 1 or 2

Edit: IndexOutOfRangeException should be ArgumentOutOfRangeException, I always mix the two

 

I would personally write this even more robustly, but I don't want to flood you with concepts and syntax you perhaps didn't learn yet. So on my own scale of code quality, this is a strong, useful 6/10. Yours was a weak 2/10, which is okay, don't fret about it, I am just giving you a point of reference.

I.e. I'd also make sure that ColourIdentity also implemented IEquatable<ColourIdentity>, name some things differently, and I'd develop external utilities to actually produce colors and assign those to Mesh Renderers. As you can see, internally you only really care about these identities, whether something is the same or not the same. Whether it's rendered as orange or as teal in the end, is a completely separate concern.

 

So sorry for that, it was my first ever post, but being a regular reader here i did wonder how to get my code to look like ,well "code". Thanks for pointing this out, now i know. Always learning and improving haha

 

Wow, Thank you so much for such a detailed insight, although i would admit some things here are out of my intellectual capacity at this stage haha. But definitely trying my best to get my head round this. A lot of it does make sense to me already. With the switching logic, initially i was actually just managing one logic, but after countless hours of trying to figure this bug out, and basically trying every little thing in the hopes of fixing it, i decided how about i control all three values as well lmao.
But once again, thanks alot. this does provide a lot of insight and i'm hoping to implement it your way now.

 

If it's easier for you to think about it, the whole idea is that you use a unique integer to represent a different color.

You don't necessarily need

ColourIdentity

above, however you still need some code to reinterpret that integer as a color, and you want to put your cycling logic somewhere.

If you make a

struct

out of it, a new value type, you can have everything there, which is the proper way to do this. But also sets everything up for future expansion.

For example, instead of having GetAsColour, you can make ApplyTo.

Code (csharp):

1. private Color getAsColour() { ... } // what public GetAsColour used to be
2.  
3. public void ApplyTo(Material material) {
4.   material.color = getAsColour();
5. }

And now you just

Code (csharp):

1. selectedColour.ApplyTo(GetComponent<MeshRenderer>().sharedMaterial);

Etc.
Btw the difference between sharedMaterial and material is huge, this is why I keep it on this side of the line. Just 'material' will make a new instance of it. Which is not always what you want. I would avoid that in the second use case as well (if the obstacles tend to change color over time), but I had to keep this at a minimum for you to understand more easily.

Likewise you can also implement

Code (csharp):

1. public bool Equals(ColourIdentity other)
2.   => Index == other.Index;

but also

Code (csharp):

1. public bool IsSimilarTo(ColourIdentity other) {
2.   switch(_index) {
3.     case 0: // let's say this is red
4.       if(other.Index == 7) return true; // let's say this is orange
5.       break;
6.   }
7.   return false;
8. }

At this point you can see that it would be useful to have words to describe colors internally. You can do this with an enum type. But enums are a huge problem if you want to serialize this. And how do you serialize this? Well you can do

Code (csharp):

1. using System;
2. using UnityEngine;
3.  
4. [Serializable]
5. public struct ColourIdentity {
6. ...
7. [SerializeField] int _index;
8. ...
9. }

Though this is a bit of a wide topic, so tell me if you're interested, because you can also make a drawer to show this piece of information as a color (+ value) in the inspector.

--
Just stick your code where it belongs. If you can't tell where it belongs, that's a clear tell tale sign that your overall design is wrong somehow as things will only get messier as your project gets larger. So when that happens spend some time analyzing your code. This early on, every piece of code needs to have a place to call home. This is how you architecture the project and how you make clear separations of concern.

If you have any questions about the example, still need to learn some syntax, feel free to ask.