# Why doesn't this weaponpickup script work properly?

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

• "it simply doesn't work" — great, now tell us how! Does it not do anything when you press the buttons? Does it do something different from what you wanted? Does it throw a compiler or runtime error? Does your computer catch fire? The specific symptoms of "not working" are important to be able to diagnose the cause. Also, don't forget to preview your code to make sure it's formatted correctly before posting. – DMGregory Mar 10 at 2:38

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?

• 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;
}

• Please note that in the example above I use the weapon you are holding to tell the game what weapon to spawn before destroying the one you are holding. I am not 100% sure if this will spawn the source prefab or an identical clone of what you have but I'm gambling on the former. It might be a good idea for you to store the weapon you collect in a global reference so you an use that when you drop it (in case this method doesn't work). The other option is for you to NOT instantiate a new weapon and destroy your current one. Simply set the parent of gun to null and position it somewhere. – JansenSensei Mar 10 at 20:01