Need help fixing shooting in my first 2D game - Unity 6

Getting Started
1

Hello Unity forums!
This is my first unity project that I’ve undertaken all on my own, and I’m extremely new to Unity, its systems, and C# as a whole. I’ve been researching things I’ve wanted to do and ways to do them, but I’ve found almost nothing helpful that fits what I want to do.

I’m trying to make a 2D dual-stick shooter using Unity 6 and its new Input System Package(which is incredibly complicated to me as a new developer + I hate it). I have movement sorted and working right now, but shooting is where my problem begins. I’ve been struggling with trying to figure out how to continuously spawn bullets after a delay while a key is held. This sounds simple enough to me, but I’ve spent around a week or two testing things and trying to get this to work.

Here’s my code for the player’s shooting:

using UnityEngine;
using UnityEngine.InputSystem;
using System.Collections;

public class PlayerShooting : MonoBehaviour
{
    [SerializeField] private GameObject bulletPrefab;
    [SerializeField] private bool buttonHeld; // for only shooting when button held
    
    private Quaternion bulletRotation; // rotation bullet will face
    private PlayerMovement movementScript; // for playerPos variable from PlayerMovement.cs
    private bool shootBool; // to disable shooting for shot delay or otherwise
    private float shootDelay = 0.5f; // delay in sec between bullets

    void Awake()
    {
        movementScript = GetComponent<PlayerMovement>();
        shootBool = true; // allows player to shoot on game start
    }

    void Update()
    {
        Shoot(bulletRotation); // tries to spawn bullet every frame, only works when buttonHeld true
    }

    public void OnFireUp(InputAction.CallbackContext context)
    {
        if (context.performed && !buttonHeld) // if only up arrow key pressed
        {
            bulletRotation = Quaternion.Euler(0, 0, 0); // bullet faces up
            buttonHeld = true;
        }
        if (context.canceled) // buttonHeld false when key released
        {
            buttonHeld = false;
        }
    }

    public void OnFireRight(InputAction.CallbackContext context)
    {
        if (context.performed && !buttonHeld) // if only right arrow key pressed
        {
            bulletRotation = Quaternion.Euler(0, 0, 270); // bullet faces right
            buttonHeld = true;
        }
        if (context.canceled) // buttonHeld false when key released
        {
            buttonHeld = false;
        }
    }

    public void OnFireDown(InputAction.CallbackContext context)
    {
        if (context.performed && !buttonHeld) // if only down arrow key pressed
        {
            bulletRotation = Quaternion.Euler(0, 0, 180); // bullet faces down
            buttonHeld = true;
        }
        if (context.canceled) // buttonHeld false when key released
        {
            buttonHeld = false;
        }
    }

    public void OnFireLeft(InputAction.CallbackContext context)
    {
        if (context.performed && !buttonHeld) // if only left arrow key pressed
        {
            bulletRotation = Quaternion.Euler(0, 0, 90); // bullet faces left
            buttonHeld = true;
        }
        if (context.canceled) // buttonHeld false when key released
        {
            buttonHeld = false;
        }
    }

    private void Shoot(Quaternion rotation)
    {
        if (shootBool && buttonHeld)
        {
            Instantiate(bulletPrefab, movementScript.playerPos.position, rotation); // spawns bullet with rotation
            StartCoroutine(ShootDelay()); // delays next shot by shootDelay seconds
        }
    }

    private IEnumerator ShootDelay() // for delaying shots
    {
        shootBool = false;
        yield return new WaitForSeconds(shootDelay);
        shootBool = true;
    }
}

I have the Shoot() method in the Update() method and I’m only changing the variables that that method relies on in the action methods. The buttonHeld boolean is what the Shoot() method relies on, as well as the shootBool boolean, but that variable is managed within the ShootDelay() coroutine. The buttonHeld boolean is managed within the action methods(OnFireUp, Down, etc), where it is set to true when the action is performed, and set to false when the action is cancelled. The problem, though, is that this behavior only happens when firing up, and the first bullet spawns after the 0.5 second delay. Firing left, down, or right by pressing the arrow keys doesn’t set buttonHeld to false when releasing the key.

I tried explaining my problem and expectations clearly here, but if any information is needed to help me solve my problem, I will happily provide. My brain is a bit fried from trying to work on this even just to find the problems to report here. I’m trying to just get as much as I can down in order to explain what I need to but I just want to be done for now.
Thank you in advance.

2

I will not state step by step how to do this as you are new, its an amazing chance to research, I will say this though:
Look into Coroutines, Do while loops, Delays and InvokeRepeating
Through a combination of these your problem will be fixed (a do while loop is basically a loop that runs if a condition is true or false, make sure to have a break out through conditions like if ballsFired = 10; break;

3

Try if else instead of multiple if statements
//If pressed true; else false;

5

It seems what you are aiming to do is start firing bullets once conditions are met and keep firing them until conditions are no longer valid. Having said so you may want to abandon doing stuff in the Update () function, as you have other methods already set up. Namely interpreting input by using the Input System. As for firing working with one button, but not the others, one could suspect erroneous/unclear binding. Anyhow in your code you may want to try calling Shoot () from one of the OnFire methods. The following might not be exactly what you need, but should give you an idea of a slightly different approach. You may want to rename a few members for clarity, too. A static coroutine assures bullets are shot only one way at any given time. In the while loop, you could also add a counter, e.g. 10 bullets max or so. It often proves good practice to limit such loops with either time-to-run- or repetition-limiters. But that’s just a bit on the side.


static Coroutine shootDelay = null;

public void OnFireLeft(InputAction.CallbackContext context)
{
    if (context.performed) // if only left arrow key pressed
    {
        Shoot(Quaternion.Euler(0, 0, 90)); // bullet faces left
    }
    if (context.canceled) // buttonHeld false when key released
    {
        buttonHeld = false;
    }
}

private void Shoot(Quaternion rotation)
{
    if ( shootDelay  != null ) { StopCoroutine(shootDelay); }
    shootDelay = StartCoroutine(ShootDelay());
}

private IEnumerator ShootDelay() // for delaying shots
{
    int counter = 0;
    int counterMax = 10;
    buttonHeld = true;
    while(buttonHeld && counter < counterMax)
    {
        counter++;
        Instantiate(bulletPrefab, movementScript.playerPos.position, rotation); // spawns bullet with
        yield return new WaitForSeconds(shootDelay); 
    }
    shootDelay = null;
    yield break;
}
6

Thanks for your reply! I’ve tried to put the Shoot() method in the OnFire() methods, and I understand why you would do that, but the problem then is that only one bullet is spawned every time I press and hold an arrow key.

As for your suggestion about the bindings being incorrect, I’ve double- and triple-checked each entry in the list, and every firing binding is exactly the same in the Input System window.

I have a plan, also, for limiting the amount of shots that can be made before reloading, and in order to better optimize the game(specifically using Object Pooling).

I really appreciate the help, and I’m sorry my own reply has been so late!

7

By the way, this line of your code confused me a little, and I’m not sure what would be the benefit of doing this. Why would you set a variable equal to a coroutine start command?

8

Thanks for the suggestion, but I tried this, and it didn’t change the result.

9

It’s an endeavour to write game code and akin to art no two artists make same brush strokes. There are many ways how this challenge can be completed. Nevertheless, irrespectively of what one tries to achieve taking a more simple path often proves beneficial. To me it is illogical to have several functions with the exact same body just because there are as many as x firing buttons. OK, there’s z-rotation, but that can be passed as a parameter. It’s also true fancy code tends to disappoint. At any rate the mechanics of logic should be clear before any code is written.

If we can imagine a parallel, such would be opening a door for the wind to blow through. Once I open the door (e.g. button down), the wind starts and continues blowing through (e.g. shots fired within a coroutine) until I close the door (e.g. button up). And that to me looks simple.

Controlling shooting within a coroutine has several benefits compared to that within the Update() method, the first that comes to mind is that you are pretty much in charge of the execution speed of a coroutine (unless you put a strain on the resources), whereas updating occurs quite irregularly. One can easily inspect the FPS graph to verify.

Does the code work as expected with a single button? I.e., whilst the button is held down, the bullets keep flying out at the desired interval? Does the code work with any single one button out of the four? Start with simple actions, verify compliance, enhance a bit, verify compliance etc.

If several buttons can affect the same state (flag/bool), it means there’s an override happening after the button supposed to do something is pressed. E.g. the ‘button up’ sets a bool value to false, but another action sets it to true immediately afterwards. So that needs be checked.

When the button is pressed, is it registered only in the first frame it’s been pressed or is it registered as pressed in every consecutive frame? Like Input.GetKeyDown() and Input.GetKey() are same-same but different.

There’s also the unfavourable bit with WaitForSeconds() cause it’s affected by time-scaling, meaning it can inadvertently result in ‘execution delayed until forever’ and it’s often preferable to use WaitForSecondsRealtime()which is immune to time-scaling.

The answer to why shootDelay = StartCoroutine(ShootDelay()); is simple, cause the least I can do from anywhere in the code is if(shootDelay != null) { StopCoroutine(shootDelay); } and said coroutine will stop. No stoppers within the coroutine are required. E.g. each button when pressed first stops the coroutine and then starts it. There’s also the StopAllCoroutines(); method that stops all coroutines running on a behaviour.

Also to change the state of a bool variable one can use alternatives like

bool boolVariable = false;
async void ChangeBoolWithDelay()
{
     await System.Threading.Tasks.Task.Delay(100);
     boolVariable = !boolVariable;
}

Last, but not least, there may be many reasons why code wouldn’t work. Sometimes we find the errors and at times we don’t. It’s a matter of personal preference but quite often after a reasonable time of ‘debugging’ it makes sense to delete everything and start afresh. A line written in a different manner may be all it takes to get the code running.

Good luck.

10

This results in you storing a reference to the instance of the coroutine you’re starting.

Each time you use StartCoroutine, you create a new instance of one.

By storing that reference in a variable, you’re then able to perform a “null check” to determine whether that specific coroutine instance is still running. If it is, you can stop it before starting a new coroutine (in order to prevent two different instances of the same coroutine from running at the same time). This is exactly what is being done on the line above StartCoroutine in the code you quoted.