My first "real" script, and it's driving me insane!!! (pick targets at random in screen)

Unity Engine
1

The goal of this script:

Imagine a delivery game. You have plenty of houses around (100+). You need to go to house A (origin house) to pick a package, then go to house B (target house) to deliver the package.

There is an object in my game scene called “DELIVERY MANAGER” with the following script attached. Player already moves fine in the scene, and so does camera/light. All houses have solid colliders. I want the player to be able to pick or deliver the package if he is close enough to the house.

ALL ADVICE APPRECIATED INCLUDING TO CLEAR MY CODE i know it’s a mess!!

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

public class DeliveryPicker : MonoBehaviour
{
    public Transform playerPosition;
    public TextMeshProUGUI textToPlayer;
    public GameObject gameObject;
    //public Renderer sphereRenderer;
    public GameManager gameManager;
    public GameObject[] prefabDelivery;
    public int randomHouse;
    public int randomHouse2;
    public GameObject targetHouse;
    public GameObject originHouse;
    public Material green;
    public Material red;
    public Material white;
    public int cashReward;

    public bool deliveryActive = false;

    // Start is called before the first frame update
    void Start()
    {
       
    }

    // Update is called once per frame
    void Update()
    {
        if (!gameManager.hasDelivery && !deliveryActive)
        {
            randomHouse = Random.Range(0, prefabDelivery.Length);
            originHouse = prefabDelivery[randomHouse];
            // public Renderer originHouseColor;
            originHouse.GetComponent<MeshRenderer>().material = green;
            gameManager.hasDelivery = true;
        }
        else
        {
            if (playerPosition && !deliveryActive)
            {
                float dist = Vector3.Distance(playerPosition.position, originHouse.transform.position);
                if (dist < 15.0f && Input.GetKeyDown(KeyCode.E) && gameManager.questActive == false)
                {

                    textToPlayer.text = "You got a delivery! Deliver to the house in green!";
                    PickTarget();
                    deliveryActive = true;
                }
            }
        }

        if (deliveryActive)
        {
            if (playerPosition)
            {
                float dist = Vector3.Distance(playerPosition.position, targetHouse.transform.position);
                if (dist < 15.0f && Input.GetKeyDown(KeyCode.E) && gameManager.questActive == false)
                {

                    textToPlayer.text = "Package Delivered! Enjoy the money!";
                    cashReward = Random.Range(95, 250);
                    gameManager.UpdateCash(cashReward);
                    deliveryActive = false;
                    gameManager.hasDelivery = false;
                }
            }
        }
       
    }
    void PickTarget()
    {
        randomHouse2 = Random.Range(0, prefabDelivery.Length);
        while (randomHouse2 == randomHouse) { randomHouse2 = Random.Range(0, prefabDelivery.Length); }
        targetHouse = prefabDelivery[randomHouse2];
        originHouse.GetComponent<MeshRenderer>().material = green;
    }


}

Imaginge a scene with 100+ houses. If no deliveries are active, the script should pick a house, make it green so the player knows where to pick the package, and allow the player to pick up the package by coming close to the House and pressing E. Player also needs not to be on a quest.

Then the house the player picked his package from should go back to its original color (not implemented yet!) while the target house should turn green. Getting near the target house and pressing E should reward the player by a) awarding him cash b) generating another house to pick a package from, and turning that house green.

BUT NOTHING WORKS!!! Either nothing happens at all, or I can’t interact with anything!!! No error in compiler. Spent all night on this code and nothing works. At this point its 1AM, ive been working all day and I cant even think anymore. NOT giving up, but I’d appreciate help. I’ll continue this script tomorrow.

This is my first “real” code (check my old post to see how I progressed lol). All advice/rating/anything appreciated, thank you

2

I am relatively new to Unity as well and no expert by far, but I have had my fair share of frustrations like this, so I’ll try to help out.

It’s hard to know for sure what might be wrong without seeing the rest of your code and what values you have assigned in the Inspector, but just a few questions/comments:

  • Have you tried watching your values in the Inspector while playing, to see if they are what you expect them to be? For example, are you sure that gameManager.hasDelivery and deliveryActive are false at the start, and do they change to true when you expect them to?
  • What do you mean nothing happens at all? Are you not seeing any house change color to green? Instead of changing the material directly, have you tried changing just the color, like this: GetComponent<MeshRenderer>().material.color = color.Green?
  • I noticed you check if (playerPosition) in a few places. What is that meant to be checking, and what happens if you remove that from your if statements and leave everything else intact?
  • I don’t know if it is the best way to debug, but when I encounter issues like this, one “quick” thing I sometimes try is adding some Debug.Log lines in to see if I am even hitting the parts of code I expect. For example, you might try adding something like this after line 37:

Debug.Log("randomHouse = " + randomHouse + " ; originHouse.transform.position = " + originHouse.transform.position);

  • One other note, you may want to do something similar to above to check your distance calculation and make sure the numbers are coming back as what you would expect, in case you are using the wrong units or something like that. Here’s one example of how you might add an else if to your existing code to do that:
float dist = Vector3.Distance(playerPosition.position, originHouse.transform.position);
if (dist < 15.0f && Input.GetKeyDown(KeyCode.E) && gameManager.questActive == false)
{
    textToPlayer.text = "You got a delivery! Deliver to the house in green!";
    PickTarget();
    deliveryActive = true;
}
else if (Input.GetKeyDown(KeyCode.E))
{
    Debug.Log("E key pressed. Current dist to originHouse = " + dist);
}
2 Likes
3

Here are some comments and an untested rewrite of your code (I got carried away a little :smile:).

“At this point its 1AM…”
My first advice would be to go to sleep and come back afterwards. It sounds superficial but once you have gone beyond 5 to 7 hours of pure coding your capacity to think degrades rapidly. You will get nothing done anymore OR (as proven by the industry via “crunch”) you will even generate negative work. This means fixing your mistakes (made in this state) will take longer than writing the code in the first place. I can not tell you how often I have struggled with a problem and forced myself to go to bed. Only to discover that I can solve it within minutes the next day.

public GameObject gameObject;

Remove this line. It’s not used and even if used it would just hide the default gameObject (useless).

Why is “prefabDelivery” named like that. From what I can tell it’s a list of houses to choose from.

Your variables “randomHouse”, “randomHouse2”, “originHouse” and “targetHouse” are redundant. “randomHouse2” should be a local variable since it is not used anywhere outside our PickTarget() method (or you can skip it entirely).

while (randomHouse2 == originHouseIndex) ...

Dont’ write your code in one line like this. It makes your code hard to read and debug.

For debugging add some logs (as SausageMania suggested) or take a look into the Debugger (Unity - Manual: Debug C# code in Unity).

Your PickTarget() code and your Update() “!GameManager.hasDelivery” code is the same. There is a nice priciple for programming called DRY (Don’t Repeat Yourself). Don’t repeat your pickTarget code. Break it up into little repeatable steps. It helps a lot and makes reasoning about your code easier too.

You have two lines which calculate the same distance to the player. Again, think of DRY and make it a method.

That “GameManager.hasDelivery” stuff is not very nice. You are duplicating information here. That may come back to bite you in the butt later. Maybe add a HasDelivery() method to GameManager which the asks the DeliveryPicker if it has a delivery.

Here is my take on your code. Not sure if it works (untested) but you will see that the layout makes reasoning about what happens much easier. I also added some comments for you.

You can use [Header(“Text”)] Attributes to structure your references in the inspector. Use them if you have a lot of refs. It makes finding stuff much easier.

using UnityEngine;
using TMPro;

public class GameManager
{
    public bool hasDelivery = true;
    public bool questActive = true;
    public void UpdateCash(int cashReward) {}
}

public class DeliveryPicker : MonoBehaviour
{
    /// <summary>
    /// The GameManager is the connection to the game logic.
    /// </summary>
    [Header("Dependencies")]
    public GameManager GameManager;

    /// <summary>
    /// The players position (transform).

    /// Needed to caclulate the distance to houses.
    /// </summary>
    public Transform PlayerPosition;

    [Header("Materials")]
    public Material Green;
    public Material Red;
    public Material White;

    [Header("Configs")] // you may want to load these from somewehere else (config files, scriptable object, ...)
    public float MaxDistanceToHouse = 15f;
    [Tooltip("x = min reward, y = max reward (exclusive)")]
    public Vector2Int CashRewardRange = new Vector2Int(95, 250);
    public KeyCode ActionKey = KeyCode.E;

    [Header("References")]
    public GameObject[] Houses;
    public TextMeshProUGUI TextToPlayer;

    // Temporary variables used to store temporary data (don't make these public or mark them as "[System.NonSerialized]").
    [System.NonSerialized]
    public int CashReward;
    protected GameObject originHouse;
    protected GameObject targetHouse;

    /// <summary>
    /// Is there a delivery waiting for pickup or is the player carrying a delivery?

    /// This will be sycned to GameManager.hasDelivery for global access.
    /// </summary>
    protected bool hasDelivery;

    // Consider having a HasDelivery() method in GameManager which then asks DeliverPicker if it has a delivery.
    public bool HasDelivery
    {
        get => hasDelivery;
    }

    /// <summary>
    /// Is the player currently carrying a package (true = yes).

    /// </summary>
    protected bool deliveryActive = false;

    // Update is called once per frame
    void Update()
    {
        // Stop if we  have no player postition or game mamanger (we can't do anything without them)
        if (PlayerPosition == null || GameManager == null)
            return;

        bool keyPressed = Input.GetKeyDown(ActionKey)

        if (deliveryActive)
        {
            // player has a package, now we wait for delivery
            if (keyPressed && GameManager.questActive == false && isPlayerNearHouse(PlayerPosition, targetHouse, MaxDistanceToHouse))
            {
                finishRoute();
                deliveryActive = false;
                GameManager.UpdateCash(CashReward);
                TextToPlayer.text = "Package Delivered! Enjoy the money!";

                pickNewRoute();
            }
        }
        else
        {
            // player has no package
            if (hasDelivery == false)
            {
                pickNewRoute();
            }
            else
            {
                if (keyPressed && GameManager.questActive == false && isPlayerNearHouse(PlayerPosition, originHouse, MaxDistanceToHouse))
                {
                    deliveryActive = true;
                    TextToPlayer.text = "You got a delivery! Deliver to the house in green!";
                }
            }
        }
    }

    protected void finishRoute()
    {
        if (targetHouse != null)
        {
            // reset targetHouse house color
            targetHouse.GetComponent<MeshRenderer>().sharedMaterial = White; // use sharedMaterial instead of material (material instantiates a new material every time it is used)
        }

        originHouse = null;
        targetHouse = null;

        hasDelivery = false;
        GameManager.hasDelivery = hasDelivery;
    }

    /// <summary>
    /// Picks two new houses as origin and target and calculates a new CashReward for said route.
    /// </summary>
    protected void pickNewRoute()
    {
        // var oldTarget = targetHouse; // incase you want to do some thing with the old target (ignore it for the new route origin for example).
        finishRoute();

        // new route with new origin and target (target must not be the same as origin)
        originHouse = pickRandomHouse(null); // or oldTarget maybe?
        targetHouse = pickRandomHouse(originHouse);

        // update the cash reward (maybe based on the house distance)
        CashReward = Random.Range(CashRewardRange.x, CashRewardRange.y);

        // set new target house color
        targetHouse.GetComponent<MeshRenderer>().sharedMaterial = Green;

        hasDelivery = true;
        GameManager.hasDelivery = hasDelivery;
    }

    protected bool isPlayerNearHouse(Transform player, GameObject house, float maxDistance)
    {
        return Vector3.SqrMagnitude(house.transform.position - player.position) < (maxDistance * maxDistance); // SqrMagnitude is more efficient
    }

    /// <summary>
    /// Picks a random house from the "Houses" list.

    /// Ensures the new house is not null.
    /// Throws and exception if no house could be found.
    /// </summary>
    /// <param name="ignoredHouse">The new house must not be this house.</param>
    /// <param name="maxTries">The limit of tries to avoid infinite loops. Throws an exception if the limit is reached.</param>
    /// <returns></returns>
    protected GameObject pickRandomHouse(GameObject ignoredHouse = null, int maxTries = 100)
    {
        GameObject newHouse;
        do
        {
            newHouse = Houses[Random.Range(0, Houses.Length)];
        }
        while ( maxTries-- > 0 && (newHouse == ignoredHouse || newHouse == null) );

        if (maxTries <= 0)
        {
            throw new System.Exception("Could not pick a house.");
        }

        return newHouse;
    }
}
2 Likes
4

I just wanted to take the time to thank you both for taking the time to review my code and sharing your help. I appreciate greatly!

5

When this happens, it is almost ALWAYS one of the following “usual suspects:”

  • Part (or all) of your code is not running (or did not run)
  • Your code is running far less often than you think
  • Your code is running far MORE often than you think

Regardless of what is going on, 100% of code problems will yield to the following:

To help gain more insight into your problem, I recommend liberally sprinkling Debug.Log() statements through your code to display information in realtime.

Doing this should help you answer these types of questions:

  • is this code even running? which parts are running? how often does it run? what order does it run in?
  • what are the values of the variables involved? Are they initialized? Are the values reasonable?
  • are you meeting ALL the requirements to receive callbacks such as triggers / colliders (review the documentation)

Knowing this information will help you reason about the behavior you are seeing.

You can also put in Debug.Break() to pause the Editor when certain interesting pieces of code run, and then study the scene

You could also just display various important quantities in UI Text elements to watch them change as you play the game.

If you are running a mobile device you can also view the console output. Google for how on your particular mobile target.

Here’s an example of putting in a laser-focused Debug.Log() and how that can save you a TON of time wallowing around speculating what might be going wrong:

https://discussions.unity.com/t/839300/3