So i am making a level of unity: In this level you have to sort the garbage. The player can move with the arrow keys, and can pick up trash with the E key. Then take it to the right trash can. I made a script that should make the character pickup the item and then he can go to the right trashbin and if he hits the trashbin the item will be destroyed, but it does not work and I have no idea what is wrong.
using System.Collections;
using System.Collections.Generic;
using UnityEngine;
using TMPro;
public class PickupItem : MonoBehaviour
{
public float pickupRange = 2f;
public LayerMask pickupLayer;
public AudioClip pickupSound;
public string[] pickupTags;
public AudioClip wrongBinSound;
public string[] trashBinTags;
public TextMeshProUGUI itemNameText;
private AudioSource audioSource;
private GameObject currentObject;
private bool holdingItem = false;
private void Start()
{
audioSource = GetComponent<AudioSource>();
}
private void Update()
{
RaycastHit hit;
if (Physics.Raycast(transform.position, transform.forward, out hit, pickupRange, pickupLayer))
{
foreach (string tag in pickupTags)
{
if (hit.collider.tag == tag && !holdingItem)
{
currentObject = hit.collider.gameObject;
if (Input.GetKeyDown(KeyCode.E))
{
StartCoroutine(Pickup());
}
break;
}
}
}
else
{
currentObject = null;
}
}
private void OnCollisionEnter(Collision collision)
{
foreach (string trashBinTag in trashBinTags)
{
if (collision.gameObject.tag == trashBinTag)
{
switch (currentObject.tag)
{
case "paper":
if (trashBinTag == "TrashbinPa")
{
Debug.Log("paper in vuilbak");
audioSource.PlayOneShot(pickupSound);
itemNameText.text = "";
holdingItem = false;
}
break;
case "glass":
if (trashBinTag == "TrashbinG")
{
Debug.Log("glass in vuilbak");
audioSource.PlayOneShot(pickupSound);
Destroy(currentObject);
itemNameText.text = "";
holdingItem = false;
}
break;
case "metal":
if (trashBinTag == "TrashbinM")
{
Debug.Log("metal in vuilbak");
audioSource.PlayOneShot(pickupSound);
Destroy(currentObject);
itemNameText.text = "";
holdingItem = false;
}
break;
case "plastic":
if (trashBinTag == "TrashbinP")
{
Debug.Log("plastic in vuilbak");
audioSource.PlayOneShot(pickupSound);
Destroy(currentObject);
itemNameText.text = "";
holdingItem = false;
}
break;
default:
audioSource.PlayOneShot(wrongBinSound);
break;
}
break;
}
}
}
IEnumerator Pickup()
{
if (currentObject != null)
{
Debug.Log("Object picked up");
yield return new WaitForSeconds(1);
audioSource.PlayOneShot(pickupSound);
currentObject.SetActive(false);
itemNameText.text = "Inventory: " currentObject.name;
holdingItem = true;
}
}
}
My character settings: enter image description here
One of mine Trashbin(Glass): enter image description here
One of mine Trash items(Wine bottle): enter image description here
I don't know what i'm doing wrong can someone help me?
I tried to debug but no outcome. I can pickup the item but i cannot hit the trashbin so it can delete the item. I use tags for the right trashbin which also use tags.
CodePudding user response：
I'm not entirely sure what your switch statement is doing, since currentObject gets disabled when it gets picked up (currentObject.SetActive(false);). By disabling it, the raycast in your Update() method will not find anything, and currentObject will be set to null every frame.
This will prevent any of your destruction code from running inside the OnCollisionEnter() method.
I would suggest either not disabling the currentObject, or pausing the raycast in Update() when an object is being held. This will allow the switch statement to run inside the collision method, and should destroy your object.
CodePudding user response：
Alright, so let's take it one step at a the time. First of all, that code needs a lot of optimization as it's a FPS-drop nightmare.
private void Update()
{
RaycastHit hit;
if (Physics.Raycast(transform.position, transform.forward, out hit, pickupRange, pickupLayer))
{
foreach (string tag in pickupTags)
{
if (hit.collider.tag == tag && !holdingItem)
{
currentObject = hit.collider.gameObject;
if (Input.GetKeyDown(KeyCode.E))
{
StartCoroutine(Pickup());
}
break;
}
}
}
else
{
currentObject = null;
}
}
Here you are raycasting every frame regardless of the player wanting to pick up the trash or not (i.e. hitting the E key). This doesn't make much sense. As I said in my comment, this check if (Input.GetKeyDown(KeyCode.E)) should be a lot earlier.
Also, if the player has holdingItem == true, raycasting to search for additional trash doesn't really make sense if the player can only carry one trash at a time. So you can optimize that code out this way. I would even argue that holdingItem will cause conflicts since the same effect can be achieved by using currentObject != null, therefore, I'll recycle holdingItem.
Next, iterating through lists each frame, also adds overhead. If you see the break, you actually call it only if this condition if (hit.collider.tag == tag && !holdingItem) is fulfilled. That means that regardless of holding an item or pressing E, as long as the player stares at some trash, you will Raycast each frame, iterate through the list each frame and compare the tag.
I mentioned tag compare. Comparing tags as string also adds some overhead. Please see GameObject.CompareTag() as that is the proper way to efficiently compare tags. Also learn how conditions work, both for or as well as for and. In your case you chose the most inefficient way to compare:
if (hit.collider.tag == tag && !holdingItem)
Even if you would choose NOT to use CompareTag() (although you should use it), this will always be more efficient:
if (!holdingItem && hit.collider.tag == tag)
This is because if the player holds an item, the next condition is skipped. When using or and and clauses in conditions always use the least expensive and most common one first.
The else is also dodgy there. That means that if the player holds an object, that object will be set to null if a Raycast doesn't hit anything in range. This might be the reason why your trash destruction fails.
This should be better, I think:
private void Update()
{
if (currentObject == null && Input.GetKeyDown(KeyCode.E))
{
RaycastHit hit;
if (Physics.Raycast(transform.position, transform.forward, out hit, pickupRange, pickupLayer))
{
holdingItem = false; // you have a 1 second delay in Pickup()
// no delay here
foreach (string tag in pickupTags)
{
if (!holdingItem && hit.collider.gameObject.CompareTag(tag))
{
holdingItem = true; // stops overlaps
StartCoroutine(Pickup(hit.collider.gameObject));
break;
}
}
}
}
}
IEnumerator Pickup(GameObject trash)
{
if (holdingItem && trash != null && currentObject == null)
{
#if UNITY_EDITOR
// don't include this outside tests
Debug.Log(trash.tag " picked up");
#endif
yield return new WaitForSeconds(1);
audioSource.PlayOneShot(pickupSound);
currentObject = trash;
currentObject.SetActive(false);
itemNameText.text = "Inventory: " currentObject.name;
}
}
Next, let's look at the trash bin.
There is a ton of repeated code that can just be simplified away. Easier for maintenance too, not to mention bug fixes.
This is how I'd write OnCollisionEnter():
private void ThrowTrashAway()
{
#if UNITY_EDITOR
// don't include this outside tests
Debug.Log(currentObject.tag " in vuilbak");
#endif
audioSource.PlayOneShot(pickupSound);
Destroy(currentObject);
currentObject = null;
itemNameText.text = "";
holdingItem = false;
}
private void OnCollisionEnter(Collision collision)
{
if (currentObject == null)
return; // don't execute anything
if ((collision.gameObject.CompareTag("TrashbinPa") && currentObject.CompareTag("paper"))
|| (collision.gameObject.CompareTag("TrashbinG") && currentObject.CompareTag("glass"))
|| (collision.gameObject.CompareTag("TrashbinM") && currentObject.CompareTag("metal"))
|| (collision.gameObject.CompareTag("TrashbinP") && currentObject.CompareTag("plastic")))
ThrowTrashAway();
else
audioSource.PlayOneShot(wrongBinSound);
}
Also keep in mind you don't destroy paper trash in your original code, like you do with other types of trash. So if you tested with paper, it wouldn't have worked anyway.
At first glance I'd say it's because of how you originally wrote the code in Update(). Of course this is also blind guessing since you haven't provided any debugging info regarding what steps the code executed in each branch on the stack nor did you tell us how each log call was or wasn't called. Regardless of whether you apply the suggestions, at least keep the concept in mind for further projects. Just because checking something in Update() is easier doesn't mean you have to do it there.
Still, as I said in my comment, the best way to really know why your code did not execute is a debugging session (not Debug.Log() calls, although they can help as well but they should never replace real debugging).
Let us know how it turned out.