Advertisement

Error stating Coroutines can only be started from a MonoBehaviour

Started by June 27, 2018 03:30 AM
28 comments, last by ethancodes 6 years, 2 months ago
21 minutes ago, frob said:

Coroutines are great for interrupted behavior of a long-running task due to how they yield midway through processing. But they aren't really necessary if you've got an Update() function. Under the hood all the system is doing is running Update(), then running every registered coroutine function.  The work you've described can just as easily be done with the main update, using a test against elapsed time like Scouting Ninja posted.

At work we talked about this with some folks from Unity.  There are internal optimizations for things like WaitForSeconds that are much more efficient to do that way than with Update-and-check-time.  They explicitly recommended we change every single one of our Update functions that did this to coroutines instead.

What I actually did instead was create a single scheduler class which handled all "do this thing later" operations with a priority queue, since I personally don't like coroutines or their limitations.

The thing that the Unity guys actually don't want people to do is have thousands of Update functions all individually performing a time check.  With one priority queue, I only have to check time until the top of the heap isn't ready, which is usually only one check.

I'm not so sure about "much more efficient".  I know they use pools based on time, but they're still iterating through all the active ones and they have the same work of testing against time. Clearly if you're dealing with an enormous number of items or large time scales there are some things you can do to reduce the work, but that doesn't seem to be the case here.

Take suggestions (both theirs and my code) with a grain of salt: Always season things based on what you actually experience, not what you're told it should be.

Advertisement
1 minute ago, frob said:

Take suggestions (both theirs and my code) with a grain of salt: Always season things based on what you actually experience, not what you're told it should be.

Agreed.  There were several things their performance consultant told us which were silly or just outright wrong.  I verify everything by testing it out myself.

Ok so I'm trying to do what @Nypyren suggested by passing the PickUpManager to the FastBall to call the StartCoroutine from that, but I'm not entirely sure how to do it. I tried several different things but keep going back to my original set up because I feel like I'm just making a mess of my code and don't know what I'm actually doing. Here is my complete PickUpManager code:


private int numOfShots = 4;
	private int bulletsPerShot = 7;
	private BasePickUp pickUp;
	private FastBall speedBall = new FastBall(); //This is used to access methods related to the different speed ball pick ups, I really need to find a different way to manage this though.
	public List<Ball> allBalls = new List<Ball>();

	public bool pickUpActive = false;
	public List<BasePickUp> pickUpQueue = new List<BasePickUp>();

	// Use this for initialization
	void Awake () {
		FindObjectOfType<Ball>().mainBall = true;
		allBalls.Add(FindObjectOfType<Ball>());
	}
	
	// Update is called once per frame
	void Update ()
	{
		if (pickUpQueue.Count != 0 && !pickUpActive) 
		{
			pickUpActive = true;
			pickUpQueue[0].ActivatePickUp();
		}
	}

	public void AddToQueue (BasePickUp pickUp)
	{
		BasePickUp newPickUp = (BasePickUp)pickUp.GetComponent<BasePickUp>().Clone();
		pickUpQueue.Add(newPickUp);
	}

And here is the BasePickUP and FastBall code:


protected int AmmoRemaining = 35;
	protected int timer = 5;
	protected Ball mainBall;

	void Start ()
	{
		//find the ball and assign it to this variable
		foreach (Ball ball in GameManager.pickUpManager.allBalls) 
		{
			if (ball.mainBall) 
			{
				mainBall = ball;
			}
		}
	}

	void Awake () 
	{
		this.GetComponent<Rigidbody2D> ().velocity = new Vector2 (2f, 10f);
	}

	void OnTriggerEnter2D (Collider2D other)
	{	//if brick is at end of screen, game over
		if (other.name == ("LoseCollider"))
		{
			Destroy(gameObject);
		}
	}

	public void OnCollisionEnter2D (Collision2D collision)
	{	
		Vector2 tweak = new Vector2 (UnityEngine.Random.Range(0f, 0.2f),UnityEngine.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") 
		{
			GameManager.pickUpManager.AddToQueue(this); //Add the pick up to the queue of pick ups.
			Destroy(gameObject); //...and finally destroy power up object
		}
	}

	abstract public void ActivatePickUp();

	//Coroutine used to set ball speed back to normal when pick ups are deactivated
	public IEnumerator PowerUpTimer (int delay)
	{	
		yield return new WaitForSeconds (delay);
		EndSpeedBall();
	}

	public void EndSpeedBall()
	{
		mainBall.Speed = 1;
		mainBall.gameObject.GetComponent<Rigidbody2D>().velocity = mainBall.gameObject.GetComponent<Rigidbody2D>().velocity.normalized;
		mainBall.gameObject.GetComponent<Rigidbody2D>().velocity *= mainBall.Speed*mainBall.MaxSpeed;

			Destroy (GameManager.pickUpManager.pickUpQueue [0]);
			GameManager.pickUpManager.pickUpQueue.RemoveAt (0);
			GameManager.pickUpManager.pickUpActive = false;
	}

	public void ManageAmmo ()
	{
		AmmoRemaining -= 7;

		if (AmmoRemaining <= 0)
		{
			Destroy (GameManager.pickUpManager.pickUpQueue [0]);
			GameManager.pickUpManager.pickUpQueue.RemoveAt (0);
			GameManager.pickUpManager.pickUpActive = false;
		}
	}

	//use this to begin the coroutine
	public void StartCoroutine (int delay)
	{
		StartCoroutine(PowerUpTimer(delay));
	}

	public object Clone ()
	{
		BasePickUp newPickUp = (BasePickUp)this.MemberwiseClone();

		return newPickUp;
	}



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

Can you help me understand what I need to do to pass the PickUpManager into the Fastball (or BasePickUp if that would be better) and what to do with it once it's there.


void Update ()
{
	if (pickUpQueue.Count != 0 && !pickUpActive) 
	{
		pickUpActive = true;
		pickUpQueue[0].ActivatePickUp(this); // MODIFIED
	}
}



abstract public void ActivatePickUp(MonoBehaviour coroutineHost); // MODIFIED




//use this to begin the coroutine
public void StartCoroutine(MonoBehaviour coroutineHost, int delay) // MODIFIED
{
	coroutineHost.StartCoroutine(PowerUpTimer(delay)); // MODIFIED
}




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

As far as I understand what you want to do, this is the least invasive way to get what you need.

This is the non-singleton "coroutine host" approach.

The trick is the "coroutineHost.StartCoroutine" part.  Everything else is just passing the instance of PickUpManager around as a MonoBehaviour just to get there.

The only caveat I know of about this is if the PickUpManager is disabled or destroyed, all coroutines that it's currently running will exit, even if the objects that it's running coroutines on behalf of are still alive somewhere else.  Just make sure PickUpManager is always enabled the whole time you need it to be, your coroutines exit when they should, and you shouldn't have any problems with this approach.

Now, as long as the 'mainBall' reference is still a valid reference, it should be OK.

Of course! This makes perfect sense now! Thank you, I'll try this out tonight when I get home but I think it should work perfectly. And I shouldn't have to worry about the PickUpManager because it should always be there. Thank you so much.

Advertisement

Hmm.  Inheritance in Unity is usually a sign you're heading down a bad path.  Are you sure that shouldn't be a separate component?  I think your clone is wrong too.  You should Instantiate a new prefab and then copy the members, if you're actually trying to create a new object.  Then StartCoroutine would work on it, and you wouldn't need the pickupmanager.  (and it will probably prevent any future errors with trying to use a Monobehavior that isn't hooked up to Unity properly)

13 hours ago, Nypyren said:

At work we talked about this with some folks from Unity.  There are internal optimizations for things like WaitForSeconds that are much more efficient to do that way than with Update-and-check-time.  They explicitly recommended we change every single one of our Update functions that did this to coroutines instead.

What I actually did instead was create a single scheduler class which handled all "do this thing later" operations with a priority queue, since I personally don't like coroutines or their limitations.

The thing that the Unity guys actually don't want people to do is have thousands of Update functions all individually performing a time check.  With one priority queue, I only have to check time until the top of the heap isn't ready, which is usually only one check.

Yeah, I could see update and check being slower, it probably has to load the whole Monobehavior into the cache, then unload it, because it can't tell that it's going to early out in the update method.  So looping over 1000 monobehaviors all doing Update checks like that is painful, compared to a coroutine, which has time intrinsic to the coroutine itself, so it iterates over a much smaller object.

Your priority queue method would also eliminate that loading of multiple monobehaviors.

@Nypyren this worked great! I have one warning though that I can't figure out. I'm still learning how to decipher Unity's warnings and Errors and how exactly to track down what they are talking about. This is the Warning:

You are trying to create a MonoBehaviour using the 'new' keyword.  This is not allowed.  MonoBehaviours can only be added using AddComponent().  Alternatively, your script can inherit from ScriptableObject or no base class at all
UnityEngine.MonoBehaviour:.ctor()
BasePickUp:.ctor() (at Assets/Scripts/BasePickUp.cs:10)
FastBall:.ctor()
PickUpManager:.ctor() (at Assets/Scripts/PickUpManager.cs:11)

 

Can someone help me understand where this is pointing to and maybe what I need to change to fix it? Everything seems to be working ok, but I would still like to fix it if I can. 

Well, I'm not sure exactly why this fixed it, but I realized that i was getting a null reference to my ball if I got a game over, it was still trying to access it after it was destroyed. After adding a null check around it, I fixed that error and also seems to have eliminated this warning. That seems a bit weird since this says I'm instantiating something with Mono, but as long as it's gone I guess i won't complain. lol.

This topic is closed to new replies.

Advertisement