AdamsLair / duality

a 2D Game Development Framework
https://adamslair.github.io/duality
MIT License
1.4k stars 289 forks source link

Improved coroutines implementation #755

Closed SirePi closed 4 years ago

SirePi commented 4 years ago

This is a reworked implementation of the first, much more complex implementation (see PR #674) and takes into account all the comments to the previous version. In addition to PR #706, the capability to Reset a WaitCondition has been added, and useless backup files (unreferenced anyway) have been removed from the repository.

SirePi commented 4 years ago

Yeah, the code went through a bit of iterations.. might have some comments to improve 😅 Thanks for the pointers.. regarding the is/as bits, would you go with as/if or as/elvis?

deanljohnson commented 4 years ago

As/if

Since you are removing by value, it’s O(n) to find the item. Then to actually remove the item from a list, O(n) items get moved. If you wanted to be more precise it’s really O(n/2) in both cases (amortized) but I dropped the 1/2 term. If you remove by index, you don’t have to do the find step as you have the index already.

On Sun, Oct 13, 2019 at 9:19 AM SirePi notifications@github.com wrote:

@SirePi commented on this pull request.

In Source/Core/Duality/Utility/Coroutines/CoroutineManager.cs https://github.com/AdamsLair/duality/pull/755#discussion_r334284639:

  • foreach (Coroutine c in this.coroutines)
  • {

  • c.Update();

  • if (c.Status == CoroutineStatus.Error)

  • this.lastFrameErrors.Add(c, c.LastException);

  • if (c.Status != CoroutineStatus.Running && c.Status != CoroutineStatus.Paused)

  • this.trashcan.Add(c);

  • }

  • foreach (Coroutine c in this.trashcan)

  • {

  • this.coroutines.Remove(c);

  • pool.ReturnOne(c);

  • }

Why do you say that removing is O(n) + O(n)? the trashcan contains the same Coroutine that has to be removed from the list, no search is being done 🤔

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/AdamsLair/duality/pull/755?email_source=notifications&email_token=ADCY4AXZILOMEBW2Q47OWKLQONC7NA5CNFSM4I4NP2Q2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCHY3YZY#discussion_r334284639, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADCY4AV3QAOTZXDUEBBT5ZDQONC7NANCNFSM4I4NP2QQ .

Barsonax commented 4 years ago

@SirePi can you update the status on this?

SirePi commented 4 years ago

Closing this PR. Replaced by #801