Do you want to be a super (voxel) hero?

Question

I have implemented an upgrades system in my Unity3d game, and I am pretty sure that I am not doing things in an optimal way. Any advice regarding best practices would be much appreciated.

The idea is that when the player collects a powerup, their run speed and jump speed increases. Collect enough powerups, and you can jump around the level like the incredible hulk.

Play the game here: Super Voxel

The powerup is a basic Sphere object with its Sphere Collider Is Trigger checkbox activated. It has this script attached:

SphereTriggerScript.cs

public class SphereTriggerScript : MonoBehaviour {
 public GameObject sphereObject;
 private GameObject playerObject;
 void Start() {
 playerObject = GameObject.Find("Player1");
 }
 void OnTriggerEnter (Collider other) {
 Destroy(sphereObject);
 PlayerScript script = playerObject.GetComponent<PlayerScript>();
 script.jumpForce += 1;
 script.runSpeed += 1;
 }
}

In the scene I have a basic Player1 prefab with this script attached:

PlayerScript.cs

public class PlayerScript : MonoBehaviour {
 public int runSpeed;
 public int jumpForce;
 void Start () {
 runSpeed = 20;
 jumpForce = 10;
 }
}

The powerup references this script with GameObject.Find. I do understand that this is suboptimal, however the powerups are created programmatically at run time, so it is not possible to simply drag and drop the Player1 game object onto the powerup prefab.

I have modified the default FPSController prefab (specifically the FirstPersonController script) to add a public GameObject field that I populate with the Player1 prefab. Then when I need to get the jump or run speed, I access that script like so:

PlayerScript script = playerObject.GetComponent<PlayerScript>();
m_MoveDir.y = script.jumpForce;

Finally, I am doing something a bit funny to position the powerup orbs properly. When creating them, I use a RayCast to position them at the "top" of the building:

int randomNumber = random.Next(0, 100);
if (randomNumber > 80) {
 int maxPossibleHeight = 250;
 RaycastHit hit;
 Ray ray = new Ray(new Vector3(x * tileSize, maxPossibleHeight, y * tileSize), Vector3.down);
 if (tileObject.GetComponentInChildren<MeshCollider>().Raycast(ray, out hit, 2.0f * maxPossibleHeight)) {
 int randomHeightOffset = random.Next(2, 12);
 float height = hit.point.y;
 GameObject powerup = (GameObject)GameObject.Instantiate (Resources.Load (powerupPrefabName));
 powerup.transform.position = new Vector3(x * tileSize, height + randomHeightOffset, y * tileSize);
 }
}

• Ethan Bierlein: Unrelated tip: You can use Screen.lockCursor = true to lock the cursor. It's a little easier to play when the cursor is locked.
• bazola: It seems that for Unity 5, the command is Cursor.lockState = CursorLockMode.Locked; Cursor.visible = false;

Answer 1

In your PlayerScript class, you have two variables, runSpeed, and jumpForce, and you initialize them in Start, like this:

void Start () {
 runSpeed = 20;
 jumpForce = 10;
}

This isn't necessary. Rather, you can do something like this:

public class PlayerScript : MonoBehaviour {
 public int runSpeed = 20;
 public int jumpForce = 10;
}

This will also mean that the variables runSpeed and jumpForce will have the default values of 20 and 10 in the Unity Editor as well.

In addition, in your SphereTriggerScript class, you also don't need to initialize playerObject in Start as well.

---

In your SphereTriggerScript class, in the method OnTriggerEnter, you never do a check to make sure that the collider is actually the Player GameObject. In order to do this, you'll have to wrap the contents of SphereTriggerScript.OnTriggerEnter in this if statement:

if(other.gameObject.name == playerObject.name)
{
 ...
}

This ensures that in case any other GameObject happens to collide with a power-up, that it won't be destroyed.

---

The UnityEngine namespace comes with a static class Random, which I prefer over System.Random while using Unity3D. Rather than having to instantiate System.Random, I can just call the methods directly from UnityEngine.Random.

Compare:

System.Random random = new System.Random();
int randomNumber = random.Next(0, 100);

To:

int randomNumber = (int)Random.Range(0, 100);

---

Other than the above, I just have a few small nitpicks:

• The general style for storing an instantiated GameObject is the below example, not containing the Gameobject. prefix, and using as for a cast instead.

  GameObject gameObject = Instantiate( ... ) as GameObject;
  

• In addition, C# braces should be placed on the next line, not like Java braces, as seen in the below example.

  if( ... )
  {
   ...
  }
  

• Finally, you have a few magic numbers scattered across your code, so here's a small list of places containing magic numbers:

  • script.jumpForce += 1;

  • script.runSpeed += 1;

  • int randomNumber = random.Next(0, 100);

  • int randomHeightOffset = random.Next(2, 12);

Other than that, your code looks pretty nice! I'm excited to see where this game goes!

Answer 2

This is way after the fact, but I wanted to add something:

You do not need to pay the cost of Find(Tag). Instead, check the tag when collided with:

void OnTriggerEnter(Collider other)
{
 if (other.CompareTag("Player"))
 {
 var player = other.gameObject;
 // do powerup things
 }
}

Other, minor notes:

• A public int which is set in Start is misleading: you can set it in the editor, but the value is then discarded at runtime. Instead, offer a [SerializeField] private int and set the value inline to provide a default in-editor, and only make it public if other scripts need to modify it. I'd prefer offering a property over making your state public, however.
• You could do the pick up logic in an OnTriggerEnter on the player script; it depends on how you want to think of the behavior. Is the player collecting powerups? The player script should handle the collection code. Is the powerup rewarding the player? Then the powerup script should handle the behavior. For performance, choose the script which will have the event called less often.
• You should probably provide a level geometry raycast layer, set your level geometry to be on that layer, and cast against that. That's more idiomatic of raycasting code that I've seen than finding the mesh collider you care about directly. Oh, and unless some of your buildings have a top below 0, you don't need to multiply your max height by two for your ray length; just adding one would be enough to guarantee that you hit the ground. In any case, maxPossibleHeight should be const (or exposed as a serialized script property) and at a higher level; perhaps in a class dedicated to storing constants related to world generation!
• And I agree with everything else Ethan said.