Unity (2018.4.13.F1), C#, Coroutine does not follow "waitforSeconds", it's 3 to 5x faster

Unity Engine
1

Hi all,

you people are my last hope. For days I try to get help on an issue, i am unable to solve. I have a coroutine with the command "yield return new WaitForSeconds(0.1f), but is runs at least twice the speed, even more.

I’m an advanced beginner only. I have a script (one class), that has two coroutines, please take a look at the pastebin. Both are created with “yield return new WaitForSeconds(0.1f)”… While the first one “CreatePowerOutput()” works as intended (engine core refilling a capacitor constantly), and is always active, the second one “RechargeShield()” is called specifically when needed (and requires energy from capacitor while doing so), but is executing 3 to 5 times faster than it should, ignoring the given 0.1f seconds. Also important, while the recharge is active, the shield can take further damage, but the way this is setup, the coroutine shouldn’t care, it just recharges until full.

I already tried a few tipps I received on Stackoverflow, like dividing through 10f instead, and also I tried using WaitForSecondsRealtime, but nothing works. I do not alter any time scale in the project(yet), and I didn’t use Time.Deltatime anywhere in this script, since I explicitly order to run in 0.1f intervalls. I just don’t understand why the Coroutine behaves this way.

Important : I checked thoroughly, the Coroutine RechargeShield() is definitly only active ONE at the time (parallel to CreatePowerOutput() ), no second instance of the same coroutine is running, it’s definitly faster and ignoring the wait time, the while loop does not wait 0.1f seconds. The control variable is also working as it should, checked that too. I use the “rechargeModeActive” variable to control the execution of the coroutine.

Is there a problem with using multiple coroutines in a script/class ? Any known Bugs, or is it a problem when you use nested code inside the coroutine ? I don’t know this, though I never heard about that. I would be really happy if someone could maybe point out the problem for me in the Coroutine “RechargeShield()”.

Thanks in advance.

2

I mean waiting 0.1 seconds is a pretty big problem to begin with. Why are you doing that?

The best way to do this is to wait a single frame, and then multiply the recharge rate by the time since the last frame (Time.deltaTime). Your current implementation has two downsides:

  • It’ll be jittery, as it’s only updating some frames.
  • It’ll be framerate dependent! Kinda!

WaitForSeconds(x) doesn’t wait for exactly x seconds. It waits until at least x seconds has passed. If you are running at a rock steady framerate, and that frame rate is divisible by 10, then you’ll hit exactly 0.1 seconds. If you don’t run into float rounding issues, which you will. So you are probably going to overshoot. The lower the framerate, the higher the overshoot. That should cause your coroutine to run slightly longer than what you’re expecting, so it wouldn’t cause the bug, but it would be a problem.

So I can’t quite tell from your scripts why they’re not working as expected. My theories are:

  • They’re working perfectly, and your head maths are wrong
  • You’re running more than 1 instance of the coroutine and you somehow failed to check that correctly
  • There’s something else increasing the charge.

It could be viable to fix it, but honestly, this should not even be a coroutine! It’s making the code super-hard to follow and read, and you’re doing very simple stuff that should be happening every frame. Just do something like:

void Update() {
    if (rechargeModeActive) {
        currentShieldHP = Mathf.MoveTowards(currentShieldHP, maxShieldHP, Time.deltaTime * hpRestored);
        DepleteCharge(Time.deltaTime * rechargePowerReq_Shield);
        if (currentShieldHP == maxShieldHP)
            rechargeModeActive = false;
    }

    capacitorSlider.value = currentPowerCapacitor;
    shieldSlider.value = currentShieldHP;
}

That should achieve exactly the same thing as your coroutine, just better, faster, and easier to read.

Finally, your indentation is so strange that honestly it’s very hard to tell which block belongs to which block. I know that it’s a consistent style, but jeez it took me a while to understand if the yield was inside or outside the while block.

2 Likes
3

Thank you very much for your quick reply. It’s only a part of the code showing here.

First, I must say i have the fixiation (is that a correct word ?) to avoid Update() as hell, which is why I thought about using a coroutine. It will not run each frame, but only every 6th or so, so I thought it might save performance.

The shieldRecharge (as is the capacitorRecharge) are updating a sliders, so I wanted to created a smooth enough visible raising of the slider bar. 0.1 Seconds was honestly smooth enough for me, also the bar will be rather small, and since Recharge is recharging a “portion” of the shield (30 HP per seconds), a litte jittering in the bar would be ok.

As for the Coroutine, it’s not in the pastebin, but I used a lot of debug lines to ensure it’s working. StartCoroutine and StopCoroutine is called only once, but in the same time the capacitor is updating 50 times, the problematic coroutine does it 120 times or more.

But if you say waitForSeconds is not 100% reliable, that’s important info, I might reconsider using it in the future for such precise tasks.

Also, thank you very much for the code you offered, I will try this out. Sorry about the intendation . I try to keep the brackets aligned, that’s all. But it’s often a pain for me too, not to mention the time i fight with the auto-intendation from VS.

I have one more question, it’s very vital to me, and a problem in my game creation.

I’m always afraid of using update(), since it’s code run every frame. That’s why I thought Coroutine would be better suited, since it wont run each frame.

I’m building a space game for PC (so enough cpu power available actualy). My ships have a lot of hardpoints and turrets, each performing the usual actions, like working, rotating, firing, calculating, you name it. Also there will be lots of projectiles (spawned from bullet pools and queues, no instantiation directly in runtime). I limit myself to 20 ships per battle, plus some additional objects. That’s why I’m afraid to use Update(), but maybe this is completly unnecessary because I underestimate the speed of the code.

The thing is, I got into modding and game design about 15 to 20 years back (yeah, I’m rather old ), while in the projects I was involved we constantly hit bottlenecks, like performance, texture size, polygon count etc… I have a hard time leaving that behind of me ,to be honest. :slight_smile:

Do you think, with this magnitude i will get in trouble with Update() ? I wanted to preserve Update() for the projectile and collision calculations, because these can’t be any jitterish, this must be as smooth as possible.

Since it’s my first big project, I do not really have a good feeling about performance in larger projects, hence I’m too concerned about Update.

Anyway, thanks a lot for helping me, i will use your variant at least for the constant update items, like engine core and shieldgenerator.

Cheers
Michael

4

You’re right that something is far more performant if it happens every 1/10th second instead of every 1/60th. It should only spend a sixth of the time!

… in theory. In practice, there’s a (slight) overhead to all the extra junk a coroutine does. If you’re running into performance issues (that you have profiled and your profiling shows that it’s coming from Update() work), then having the work happen in coroutines that wait for longer timeframes could help, but I wouldn’t do it like that.

Instead, the approach I would take would be:

  1. Remove the Update() calls from the Things, and instead have a ThingManager that holds a reference to all of the Things and update them. There’s quite a bit of overhead from the engine going down to every individual thing to Update it, so managers saves quite a bit.

  2. time slice by updating only a certain number of Things each frame. You’re kinda doing this through coroutines, but since you don’t have control over when exactly the WaitForSeconds start and end, you will have some frames with a lot of the Things updating, and other with few of them. The manager from step 1) can update say 100 of them each frame. Note that in that case you have to write down when the last update was and use the difference between then and now as delta time.

Both of those things may introduce code complexity and potentials for bugs! In that case don’t do them until you have an actual performance problem. 1) might be a better code pattern for readability overall in some instances, so that might just be a win/win, but that depends on the problem and your personal code style.

You can go in and adjust VS’ auto-indentation settings to fit your own code style. That’ll make it be a friend instead of a foe! It’s been a while since I’ve used VS (Rider’s better), but it’s at least capable of that, I believe.

2 Likes
5

Thanks again :slight_smile: A device manager might be a good idea. I probably let the turret Update() as it is, since this code is not done by me, but works fine for +1000 turrets (which I never reach, max 200), but since the ship already has a script that is more or less the manager already, i call the updates from there. Again, technically my scenes shouldn’t become an object overkill, not everytime all 20 ships are present, and not everything will be a capitalship (which has the most turrets and hardpoints), I’m just overproctective.

Not sure yet how to time slice, but I will read into it. If it should become necessary, I research it. But for now, I go with the manager idea, this seems to have a really large impact, and should benefit me a lot.

You have been very helpful, and now I can continue working. Luckily, i’m still at the beginning of the prototype phase of ships and items, so it’s not a pushback changing that little piece of code.

Thank you for your time ! :slight_smile:
B/R
Michael

6

That’s exactly the issue ^^. He used StopCoroutine incorrectly. The StopCoroutine call does nothing so the coroutine would continue to run. So each time you try to stop and start a new coroutine you would pile up coroutines which would run at the same time. So each time you are recharged to max and loose some charge again you start a new coroutine and the old one(s) continue to run. To stop a coroutine you have to store the Coroutine object that StartCoroutine returns and use that coroutine reference in StopCoroutine. Currently you create a new IEnumerator object and pass that to StopCoroutine which does nothing as this new object does not represent and currently running coroutine.

Starting and stopping coroutines all the time is also bad for performance reasons. Coroutines are objects which need to be allocated and garbage collected once they are done. It’s generally better to have a single coroutine that continues running and just enters a “sleep” state.

private IEnumerator RechargeShield()
{
    while (true)
    {
        while (!shieldGeneratorToggle.isOn || currentShieldHP >= maxShieldHP)
            yield return null;
        float shieldRechargeValue = hpRestored/10;
        float shieldRechargePowerValue = rechargePowerReq_Shield/10;
        currentShieldHP += shieldRechargeValue;
        DepleteCapacitor(shieldRechargePowerValue);
        Debug.Log("Shield recharged with " +shieldRechargeValue);
        if (currentShieldHP > maxShieldHP)
        {
            currentShieldHP = maxShieldHP;
        }
        yield return new WaitForSeconds(0.1f);
    }
}

Here the coroutine would simply wait in the inner while loop and checks each frame that either the generator toggle is off or that we are fully charged. If any of those conditions is met, it will be stuck in that inner loop. As soon as both, the generator toggle is on and the we are not fully charged, the inner while loop is finished and the rest of the coroutine code runs the recharging logic. Each iteration of the outer while loop (every 0.1 seconds) the inner while loop is checked again but as long as both conditions are true (so the while condition is false) the inner while loop is skipped and the recharging continues. As soon as the currentShieldHp reaches max, the coroutine would again wait in the inner while loop until it needs recharging again. Of course if the generator toggle is off, you would still be stuck inside the inner while loop.

If you have issues understanding the boolean logic here, you can also write it like this which may be easier to read:

while (!(shieldGeneratorToggle.isOn && currentShieldHP < maxShieldHP))

So the condition inside the inner brackets essentially expresses the condition when we do want to recharge. However the while loop should run when we don’t want to recharge, so we negate the result.

This is the same condition as above, just the “not” pulled out of the brackets. If you have trouble understanding this conversion, I recommend practising some boolean algebra. Generally a && b is the same as !(!a || !b). That’s also the reason why you can construct any logic gate out of a NOR or a NAND gate since you can convert one into the other by negating the arguments and the result.

edit
ps: Pastebin has the wonderful feature of syntax highlighting and supports countless of languages. Since you didn’t want to post the code here in code tags, would you please at least use the language feature of pastebin ^^. Since you created those pastes with an account, you should be able to edit the pastes and give them the proper syntax highlighting. Also giving them a reasonable title would also not hurt, especially for your own sanity and organisation ^^.

second edit

I totally forget to mention that the above coroutine should only be started once in Start(). It would continue to run in the background and does all the handling on its own. It represents a statemachine and essentially sits in two states: recharging or waiting for recharge.

1 Like
7

Right, I totally missed that!

Why does StopCoroutine even accept an IEnumerator? StartCoroutine returns a Coroutine object you can call stop on! And why doesn’t it complain if you’re stopping a coroutine it’s not running?

Like, who does this, which is the “correct” way to use StopCoroutine(IEnumerator):

IEnumerator coroutine = Foo();
StartCoroutine(coroutine);
...
StopCoroutine(coroutine);

???

@Starman001 , to expand on what’s going on that @Bunny83 spotted, this is how you’re supposed to use StopCoroutine:

private Coroutine runningRoutine;

void Foo() {
    ...
    runningRoutine = StartCoroutine(...)
    ...
}

void Bar() {
    ...
    StopCoroutine(runningRoutine);
    ...
}

When you invoke an IEnumerator method with yields into it (like you do in StartCoroutine(RechargeShield()) ), what happens is that the code inside RechargeShield runs until the first yield return (or yield break). After that, an IEnumerator object is returned and passed to StartCoroutine. Then the engine holds on to that IEnumeratorObject, and checks what it’s yielded, and uses that to decide when to continue.

When you call StopCoroutine, it checks to see if what you pass it matches some running coroutine, and in that case stops that coroutine. Since you were calling it with a new RechargeShield instance, you both ran the first part of RechargeShield again, and never stopped the existing coroutine.

I’d still use Update instead of coroutines here, but the way Bunny suggests would work. There are some issues there (you should really have a more complex timing mechanism to not be framerate dependent for something like gameplay elements), but it’s a workable solution.

2 Likes
8

@Bunny83 : Thanks for the explanation and your time involved. Guess I learned something today :slight_smile: Have to read it few times in a calm moment this evening , but it sounds logic, and it would explain the behaviour which does increase in speed. But I’m now more worried about my other scripts, i do have two or three more coroutines there, and I used StopCoroutine on those as well somewhere in my UI system that I created. But I thought i have picked that up from the unity scripting documentation. In the current version, VS did not gave me any error messages at all.

However, after what you Baste posted how much performance can be saved with a manager, i rather go this road instead, might even be good for use with the projectiles. It’s definitly a performance win for my battle system. Especially since you guys told me now, Coroutines are not so good at all for what I took them (easy save on performance), not to mention the garbage collection.

BTW, i check pastebin :wink: I rarely use it, i actually just started because some other website once demanded I use pastebin, can’t remember which one, it’s been a while.

@anon_67522533 : One question regarding your code-example, not fully getting that to be honest.

runningRoutine = StartCoroutine(…)

What belongs inside the brackets here, the name of the IEnumerator I created ? And when I understand it correctly, I would then call Foo() to start the process, and Bar() to end the process. Correct ?

9

Yeah. Here’s an example with the different behaviours. Try copying it into your project.
Coroutine1 is what you used to do, Coroutine 2 is how to start/stop properly, and Coroutine 3 is for when you want to make sure that only a single instance of the coroutine is running at the same time.

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

public class TestScript : MonoBehaviour {
    private Coroutine cr2;
    private Coroutine cr3;

    private void Update() {
        if (Keyboard.current.aKey.wasPressedThisFrame) {
            if (!Keyboard.current.shiftKey.isPressed) {
                Debug.Log("Starting Coroutine 1");
                StartCoroutine(Coroutine1());
            }
            else {
                Debug.Log("\"Stopping\" Coroutine 1 (wrong way to do it!)");
                StopCoroutine(Coroutine1());
            }
        }

        if (Keyboard.current.dKey.wasPressedThisFrame) {
            if (!Keyboard.current.shiftKey.isPressed) {
                Debug.Log("Starting Coroutine 2");
                cr2 = StartCoroutine(Coroutine2());
            }
            else {
                Debug.Log("Stopping Coroutine 2 (note breakage if it's not already started)");
                StopCoroutine(cr2);
            }
        }

        if (Keyboard.current.gKey.wasPressedThisFrame) {
            if (!Keyboard.current.shiftKey.isPressed) {
                if (cr3 == null) {
                    Debug.Log("Starting Coroutine 3");
                    cr3 = StartCoroutine(Coroutine3());
                }
                else {
                    Debug.Log("Coroutine 3 already started!");
                }
            }
            else {
                if (cr3 != null) {
                    Debug.Log("Stopping Coroutine 3");
                    StopCoroutine(cr3);
                    cr3 = null; // Have to do this manually!
                }
                else {
                    Debug.Log("No Coroutine 3 to stop!");
                }
            }
        }
    }

    private IEnumerator Coroutine1() {
        Debug.Log("Coroutine 1 start");
        int cntr = 0;
        while (true) {
            yield return new WaitForSeconds(1f);
            Debug.Log($"Coroutine 1 {cntr++}");
        }
    }

    private IEnumerator Coroutine2() {
        Debug.Log("Coroutine 2 start");
        int cntr = 0;
        while (true) {
            yield return new WaitForSeconds(1f);
            Debug.Log($"Coroutine 2 {cntr++}");
        }
    }

    private IEnumerator Coroutine3() {
        Debug.Log("Coroutine 3 start");
        int cntr = 0;
        while (true) {
            yield return new WaitForSeconds(1f);
            Debug.Log($"Coroutine 3 {cntr++}");
        }
    }
}

You can use that to test the behaviour. A few things to note:

  • I’m on the new input system, so if you’re not using or used to that, just swap Keyboard.current.aKey.wasPressedThisFrame with Input.GetKeyDown(KeyCode.a). Should be fairly easy to understand!
  • Try cancelling coroutine 2 before it’s started, and note that it errors on you, due to insufficient null-checks
  • I was slightly wrong in my earlier post, the IEnumerator doesn’t run at all when you create it, just when you invoke it (so StopCoroutine(Coroutine1()) doesn’t print “Coroutine 1 start”, it just does nothing.
  • You can start many instances of Coroutine 1 and 2, since the code doesn’t check for that!
  • The code above is a bit bad for example reasons. IEnumerators are just methods, and you could rewrite the three of them to just take an int;
private IEnumerator Coroutine(int number) {
    Debug.Log($"Coroutine {number} start");
    int cntr = 0;
    while (true) {
        yield return new WaitForSeconds(1f);
        Debug.Log($"Coroutine {number} {cntr++}");
    }
}
1 Like
10

Thank you, i appreciacte the help very much!