Unity: Factory design pattern issues when respawning

Question

I'm having an issue with the powerup system I've implemented in a 2D platformer I've been working on. The powerup system as it is currently implemented works for each of the three types of powerups on their own, but as soon as I include multiple powerups within a scene, I start to get issues. Also, if it helps, I'm pretty sure the design pattern I'm using here is an abstract factory. I'm not entirely sure, though. It's been a while since my last design patterns course in grad school.

The powerup system works as follows: There are three types of powerups that change some sort of player attribute (jump height, speed, and wall grip) when they're picked up. Upon colliding with the powerup, a countdown timer appears in the corner of the screen that indicates how long the effect will be active for. After the timer runs out, the normal player attributes are restored. The powerups will also respawn when the player dies.

I've ran a couple different test cases to determine what actions cause particular issues and here's what I've come up with so far:

• Everything works the way it should when only a single powerup is collected and the player dies. Powerup respawns properly.
• If I collect a powerup, and then collect another one, the first powerup does not respawn when the player dies. This happens regardless of whether or not the timer expires before collecting the second powerup.
• When a second powerup is collected, the timer does not reset properly and continues counting down from the current timer number from the previous powerup.

Here's the abstract powerup class:

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

[RequireComponent(typeof(Collider2D))]
public abstract class Powerup : MonoBehaviour
{
    [Range(1, 10)]
    public int duration = 10;
    public AudioManagerScript audioManager;
    public GameObject sceneAudioManager;

    public Image movementBoostImg;
    public Image jumpBoostImg;
    public Image wallGripImg;

    protected PlayerController player;

    private Timer timer;


    protected virtual void Start()
    {
        timer = Timer.Instance;
        sceneAudioManager = GameObject.FindWithTag("SceneAudio");
        audioManager = sceneAudioManager.GetComponent<AudioManagerScript>();

        movementBoostImg.rectTransform.sizeDelta = new Vector2(Screen.width, Screen.height);
        jumpBoostImg.rectTransform.sizeDelta = new Vector2(Screen.width, Screen.height);
        wallGripImg.rectTransform.sizeDelta = new Vector2(Screen.width, Screen.height);
    }

    protected virtual void OnTriggerEnter2D(Collider2D collision)
    {
        if (collision.CompareTag("Player"))
        {
            audioManager.PlaySound(0);
            player = collision.GetComponent<PlayerController>();
            PerformCountDown(duration);
            timer.powerup = gameObject;
            StartPowerup();
            gameObject.SetActive(false);
        }
    }

    protected virtual void PerformCountDown(int duration) => timer.DoCountdown(duration, EndPowerup);

    protected abstract void StartPowerup();

    protected abstract void EndPowerup();
}

And here's an example of one of the more specific powerups:

using System.Collections;
using System.Collections.Generic;
using UnityEngine;
using DG.Tweening;

public class MovementPowerup : Powerup
{

    public float fastMovementSpeed = 15.0f;
    public float normalMovementSpeed = 9.0f;

    protected override void StartPowerup() {
        player.movementSpeed = fastMovementSpeed;
        audioManager.PlaySound(6);
        movementBoostImg.DOFade(1, 1).OnComplete(() => movementBoostImg.DOFade(0, 1));
    } 
    protected override void EndPowerup() => player.movementSpeed = normalMovementSpeed;
}

Additionally, the timer singleton is as follows:

using System;
using System.Collections;
using UnityEngine;
using UnityEngine.UI;

public class Timer : Singleton<Timer>
{
    public GameObject holder;
    public Image image;
    public Sprite[] seconds;

    [HideInInspector] public GameObject powerup;
    public Action OnDone;

    public void DoCountdown(int start, Action OnDone)
    {
        this.OnDone = OnDone;
        holder.SetActive(true);
        StartCoroutine(CountDown(start));
    }

    private IEnumerator CountDown(int duration)
    {
        image.sprite = seconds[duration - 1];

        yield return new WaitForSeconds(1);

        if (duration >= 2) {
            StartCoroutine(CountDown(duration - 1));
        } else {
            OnDone?.Invoke();
            holder.SetActive(false);
        }
    }

    public void ResetTimer()
    {
        if (powerup == null)
            return;

        StopAllCoroutines();
        powerup.SetActive(true);
        holder.SetActive(false);
    }
}

I've had a few ideas as to how to approach this particular problem, but I'm not entirely sure what that best approach might be. I have a suspicion that the answer lies in the timer class. My first thought would be to implement logic that would check to see if a powerup is already active in the timer class and just call a function to restart the timer when a new powerup is collected, but I'm not sure if that's the ideal solution. Any thoughts would be much appreciated.

Also, as a side note, I've made a few posts on here over the past few years and I've been absolutely astounded by the unprofessionalism in some of the responses I've received. I will often include everything that I can immediately think of related to my issues, but I am not perfect and will sometimes inadvertently omit information that might prove useful in finding a solution. Don't be a jerk. Politely ask for whatever it is you think my original post should have included and I'd be more than happy to provide it under most circumstances. The elitist and outright rude attitudes, especially of some of the higher-ranking users, is absolutely disgusting and toxic. Please choose to be patient and kind to those of us with less experience in certain areas.

Answer 1

I think thank problem lies on your timer class being a singleton. Meaning you only have one timer, but each power up should probably have its own timer.

I think this speaks to a general problem with your architecture.

Your power ups should probably be responsible for timing themselves. And also adding themselves to a list or something when expired so that they can be respawned.

I suggest having each power up, when collided with, call a method ApplyPowerup. That method also starts the timer. And also Calls AddSelfToRespawnList.

When the timer expires, it calls RemovePowerup.

Then when the player dies it fires an event that the RespawnSystem is listening for. And then it iterates through all power ups that added themselves to the RespawnList. and respawns them.

In general, you want to group behaviour that’s related into the same class. And keep unrelated behaviour separate. So it’s the powerup that times itself and then expires. It’s not the timer itself that cares about power ups. So you could have a Timer class (Not a singleton) and then each powerup uses an instance of that class to actually do the timing. But the timer should know nothing about the existence of power ups.

So in conclusion, I think it makes sense to have a Timer class that just times something maybe with a duration and a countdown. And then the Powerup class USES that timer to keep its time.