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

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

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
wantyousuperherovoxel

Problem

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

Solution

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!

Code Snippets

void Start () {
    runSpeed = 20;
    jumpForce = 10;
}
public class PlayerScript : MonoBehaviour {
    public int runSpeed = 20;
    public int jumpForce = 10;
}
if(other.gameObject.name == playerObject.name)
{
    ...
}
System.Random random = new System.Random();
int randomNumber = random.Next(0, 100);
int randomNumber = (int)Random.Range(0, 100);

Context

StackExchange Code Review Q#101324, answer score: 6

Revisions (0)

No revisions yet.