Skip to content

Add WaitUntil / WaitWhile support to EditorCoroutine #2

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Aug 7, 2018

Conversation

hxdnshx
Copy link
Contributor

@hxdnshx hxdnshx commented Jan 15, 2018

No description provided.

@hxdnshx
Copy link
Contributor Author

hxdnshx commented Jan 15, 2018

In addition,it would be better to making the minimum version requirements of the Editor lower.
These codes work well in Unity 5.6.0.

@marijnz
Copy link
Owner

marijnz commented Jul 14, 2018

Hey, sorry for the late response! I realised I don't get notifications for PR's and am now going through all of my repos.. If you're still up for it, there's two things:

  1. WaitUntil and WaitWhile both implement CustomYieldInstruction! To support those and other ones, it's good enough to just check for that type and use "keepWaiting" instead. Also prevents having to use reflection.
  2. The project is using tabs, not spaces for indentation. As you have spaces here and there, it looks weird on Github, so it's worth fixing.

Good idea to support these classes, I like them and they're definitely an addition.

@hxdnshx
Copy link
Contributor Author

hxdnshx commented Jul 14, 2018

Thanks for your reply. It is a great suggestion that implementing a handler for CustomYieldInstruction.
I have modified my code, and fixed tab/space problem.
@marijnz <- I hope Github works well about mention notification.

Copy link
Owner

@marijnz marijnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good changes, but couple things still good to look at.

.gitignore Outdated
@@ -0,0 +1,36 @@
[Ll]ibrary/
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is already a .gitignore, any reason to add another one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

git reads .gitignore rather than Unity.gitignore. I'll rename Unity.gitignore to the correct name.
Ref: https://git-scm.com/docs/gitignore

/*
* To keep coroutine suspended return true. To let coroutine proceed with execution return false.
* From: https://docs.unity3d.com/ScriptReference/CustomYieldInstruction.html
*/
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two small things here:

  1. The indentation is off, as it's spaces instead of tab. So it looks odd on GitHub. Better to fix that.
  2. I think the comment here is not needed, the code is quite explicit in what it does. The comment only distracts for me.

{
coroutine.currentYield = new YieldCustomYieldInstruction()
{
customYield = current as CustomYieldInstruction
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect! 👍

}
}

private bool _status;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this still be renamed to status? Last thing 👍

@marijnz marijnz merged commit 1ca65b7 into marijnz:master Aug 7, 2018
@marijnz
Copy link
Owner

marijnz commented Aug 7, 2018

Thanks! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants