Issue with my pick up queue system

Started by
5 comments, last by Machaira 5 years, 10 months ago

I'm working on a system for my game that will allow the player to stack pick ups in a queue. As one pick up expires, the next automatically activates. I'm having an issue though where if I pick up the first one, it activates fine, but if i pick up a second directly after it, it overrides the first one, activates the second one, and then once it has run it's course, everything goes back to normal gameplay, no first pick up. I'm not sure why this is happening. Hopefully someone can spot what I'm doing wrong in my code.

Here is the code for the pick up manager:


// Update is called once per frame
	void Update ()
	{
		if (pickUpQueue.Count != 0 && !pickUpActive) 
		{
			pickUpActive = true;
			pickUpQueue[0].ActivatePickUp();
		}
		DeactivatePickUp();
	}

	void DeactivatePickUp ()
	{
		if (pickUpQueue.Count != 0 && pickUpActive) 
		{
			Destroy (pickUpQueue [0]);
			pickUpQueue.RemoveAt (0);
			pickUpActive = false;
		}
	}

And here is the PickUp:


	public override void ActivatePickUp ()
	{
		ball.GetComponent<Ball>().Speed = 2.0f; //increase ball speed...
		ball.GetComponent<Ball>().StartCoroutine(timer); //...set time that power up is active
	}

 

There is also a Base Pick Up:


public void OnCollisionEnter2D (Collision2D collision)
	{	
		Vector2 tweak = new Vector2 (Random.Range(0f, 0.2f),Random.Range(0f, 0.2f));

			this.gameObject.GetComponent<Rigidbody2D>().velocity += tweak;

		//if the pickup makes contact with the paddle or ball....
		if (collision.gameObject.tag == "Paddle" || collision.gameObject.tag == "Ball") 
		{
			GameObject.FindObjectOfType<GameManager>().GetComponent<PickUpManager>().pickUpQueue.Add(this);
			 Destroy(gameObject); //...and finally destroy power up object
		}
	}

As a side note, I am trying to find a solution to this that will work for all of my pickups. Some pickups are ammo based, some are timed. 

Advertisement

After a quick look at the code, why are you calling DeactivatePickUp every frame? Does your system require only one pickup active at a time? 

Former Microsoft XNA and Xbox MVP | Check out my blog for random ramblings on game development

10 hours ago, Machaira said:

After a quick look at the code, why are you calling DeactivatePickUp every frame? Does your system require only one pickup active at a time? 

Yes, I want only one active at a time. Checking to see if we should deactivate it each frame made the most sense to me that way if it's done, it can immediately get deactivated and then on the next frame the next one will be activated. I'm certainly open to better options if you've got any ideas.

I am not familiar with unity but to me it seems like you are activating a pickup only to immediatly deactivate it in the same update. I think you need an additional check in the deactivate function to check whether the pickup is ready to be deactivated.

(Edit: depending on the pickup, this check function could return true as soon as a timer expired or the ammo has been used. Also make sure that when destroying a pickup, its effects should be removed)

Your code has a rather confusing design, from what I can see the update code essentially runs every tick and calls deactivate, which does nothing, unless there is a pickup in the queue. In that case it instantly activates and "deactivates" it by removing it from the queue. In reality the ball speed is already changed and it appears its the coroutine that turns it off when the timer expires. I'm not that familiar with Unity but I would assume from the behavior that you describe that the second time you call the coroutine code it essentially overwrites the timer on the first call so the ball only ever gets reset when the latest call to the coroutine finishes.

I would probably change the design around to be more clear if it were me, something like activate the pickup bool and call a function that activates the effect of that pickup, then run a timer or start accumulating delta time, then once update is called and the pickup time has expired then you deactivate the effect of the old one, remove it from the queue and activate the next one(if any).

Or something like that.

16 hours ago, ethancodes said:

Yes, I want only one active at a time. Checking to see if we should deactivate it each frame made the most sense to me that way if it's done, it can immediately get deactivated and then on the next frame the next one will be activated. I'm certainly open to better options if you've got any ideas.

Ideally, you wouldn't check to see if a pickup needs deactivated, it would let you know it's done. Something like this:

 


namespace PickupManager
{
    public class PickupTimerExpireEventHandlerArgs
    {
    }

    public class Pickup
    {
        public bool IsTimed;
        public bool IsActive;
        public bool AutoActivate;
        public float Time;
        public delegate void PickupTimerExpiredEventHandler();
        public event PickupTimerExpiredEventHandler TimerExpired;

        public Pickup()
        {

        }

        public void Update()
        {
            if (IsActive)
            {
                //update timer
                Time -= PickupManager.FrameTime;

                if (Time <= 0.0f)
                    TimerExpired();
            }
        }
    }

    public class PickupManager
    {
        public Queue<Pickup> Pickups;

        public static float FrameTime = .1f;

        public event EventHandler PickupTimeout;

        public PickupManager()
        {
            Pickups = new Queue<Pickup>();
        }

        public void AddPickup(bool isTimed, float time, bool autoActivate)
        {
            Pickup pickup = new Pickup() { IsTimed = isTimed };
            pickup.TimerExpired += PickupTimerExpired;
            pickup.AutoActivate = autoActivate;
            if (Pickups.Count == 0)
                pickup.IsActive = autoActivate;
            Pickups.Enqueue(pickup);

        }

        public void PickupTimerExpired()
        {
            Pickups.Dequeue();
            if (PickupTimeout != null)
                PickupTimeout(this, null);

            if(Pickups.Count > 0 && Pickups.Peek().AutoActivate)
            {
                Pickups.ElementAt(0).IsActive = true;
            }
        }

        public void Update()
        {
            if (Pickups.Count > 0)
                Pickups.Peek().Update();
        }
    }
}

 

Former Microsoft XNA and Xbox MVP | Check out my blog for random ramblings on game development

This topic is closed to new replies.

Advertisement