Why doesn't this weaponpickup script work properly?

Question

I'm making an FPS game. My guns are disabled at the start.

When I use this script to pick up and hold one weapon at a time, requiring that you click J to drop it, it simply doesnt work!

There are two guns using the same pickup script with different prefabs, and all the prefabs but "MyGun" and "playertransform" are the same.

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

public class weaponpickup : MonoBehaviour {

    public GameObject MyGun;
    public GameObject WeaponOnGround;
    public GameObject DroppedWeapon;
    public Transform playertransform;

    // Use this for initialization
    void Start () {
        MyGun.SetActive(false);
    }

    // Update is called once per frame
    void Update () {
        if(Input.GetKeyDown(KeyCode.J))
        {
            Instantiate(DroppedWeapon, playertransform.position, playertransform.rotation);
            MyGun.SetActive(false);

        }
    }
    public void OnTriggerEnter(Collider other)
    {
        if(other.gameObject.tag == "Player" && MyGun.activeInHierarchy == false) 
        {
            MyGun.SetActive(true);
            WeaponOnGround.SetActive(false);
        }
    }
}

Answer 1

I can actually see a number of issues with that code at first glance. First comes the obvious question: Are all the weapons already on your character when the game starts or do you collect them from the world?

Why I ask is because:

• you instantiate DroppedWeapon exactly at the player position and then you have a trigger setup to let the player pickup whatever it collides with (i.e. as soon as you instantiate the dropped weapon it will get picked up the very next frame) however, you don't actually add any logic to get rid of the gun you are holding or collecting the new weapon, you just disable MyGun while still holding on to it (unless that is handled elsewhere)
• as soon as you collide with a weapon you immediately enable MyGun, not the actual weapon you just collided with
• there doesn't seem to be any actual swapping of dynamic weapons, rather it looks like you are just spawning/enabled/disabling stuff you have already placed on the component

Unless there is more code that does stuff you are not showing us, here is what I see when looking at the code:

1. Start and disable MyGun (no testing if you HAVE anything set there so I assume you are sure MyGun exists. a parent node maybe?)
2. If you press J then you Instantiate DroppedWeapon (again, not seeing you verifying that there is actually anything TO spawn, just assuming it is there. I am assuming you assigned it in the inspector then?). Right now you are Instantiating DroppedWeapon no matter what you are holding (if anything at all)
3. Place DroppedWeapon exactly at the player's position, thus triggering the player to pick up DroppedWeapon immediately
4. The pickup function simply enables the previous MyGun and disables the DroppedWeapon without actually picking up DroppedWeapon. Thus dropping the weapon simply makes your gun active again while creating and leaving a new game object disabled in the world
5. If you collide with an active weapon in the world it will do nothing with it. All you are doing is setting MyGun to active if it's disabled and then disabling the one in the world. If MyGun is already active when you collide with this weapon you simply don't do anything and continue using MyGun

As you can see, there is quite a few things that need to be fixed (unless it is being handled elsewhere) but the main problem here is that you are spawning a new pickup inside your player's collider and then have a trigger setup to simply enable whatever gun you previously had while hiding the one you just collided with.

A good start would be this (assuming MyGun is a parent object):

float RandOff() {
    float result = Random.Range(0.5f,2f);
    if (Random.Range(0f, 1f) > 0.5f) result *= -1;
    return result;
}

void DropGun() {
    if (MyGun.transform.ChildCount == 0) return;
    Vector3 newLocation = new Vector3(RandOff(), 0, Randoff());
    GameObject gun = MyGun.transform.GetChild(0).gameObject;
    Instantiate(gun, playerTransform.position.TransformPoint( newLocation, Quaternion.Identiity);
    Destroy(gun);
}

void OnTriggerEnter(Collider other) {
    if (MyGun.transform.ChildCount > 0) return;
    //do whatever other tests to see if this is a weapon to pickup then...
    other.gameObject.SetParent(MyGun);
    other.transform.localPosition = Vector3.zero;
    other.transform.localRotation = Quaternion.identity;
}