Adds "completed" signal to GDFunctionState

[neikeq]

Closes #3235

Allows code like this:

func _ready():
    whatever()
    print("_ready finished")

func whatever():
    var ret = yield(do_something(), "completed")
    prints("do_something() returned:", ret)

func do_something():
    print("do_something began")
    yield(get_node("timer"), "timeout")
    print("do_something resumed")
    return 13

# Output
# _ready finished
# do_something began
# do_something resumed
# do_something returned: 13

Rejoice.

Note: In the above example, whatever() would never be resumed if do_something() used a normal yield which returns a GDFunctionState that must be manually resumed. I think that's what #7618 complains about.

[neikeq]

Clearly, using normal yield together with yield on signal in the same function is an horrible idea and should not be done. I'll make a proposal soon to separate the concept of yielding on signal from the yield keyword. Maybe something like await. What the hell, we could even provide an API for users to create their own awaitable tasks.

[spkjp]

Oh, that's very nice. 👍

[ghost]

I think it's just a matter of documentation.
But it's really nice and clever to get this feature with such a small change. Thanks!

[RandomShaper]

The added functionality is nice. But I'm still not sure about the use of a hardcoded signal name.

Not that it's an absolutely bad thing (after all, it's just adding a signal to an object's API), but maybe we can come up with a better syntax that would make this perfect, more like something builtin in the language.

[ghost]

I don't quite understand what you want to point out: in the backed, he added a signal called "completed", and this signal is exposed to GDScript as-is, and the signal name as is hard-coded just like the name of any other signal.

Are you saying that this signal should be hidden from GDScript and put behind some special syntax, or it should be implemented using a totally different mechanism that doesn't involve signals?

I don't think using a signal is any weirder than the fact that yield() returns a function state object which you're supposed to use for dealing with them.

[RandomShaper]

Option 1, I'd say. :) Not sure about the "should" part. I'm just trying to encourage a bit further discussion on this. I find that adding a signal to a class' API is perfectly fine, but in this context, using it to overcome a limitation of the language, I feel we are mixing high and low level elements. If the signal has allowed to implement this in a painless way, that's great and a clever usage of it, but maybe it should be hidden as an implementation detail. That's my point. Hope makes sense. El 16/05/2017 a las 0:23, Ferenc Arn escribió:

…

I don't quite understand what you want to point out: in the backed, he added a signal called "completed", and this signal is exposed to GDScript as-is, and the signal name as is hard-coded just like the name of any other signal. Are you saying that this signal should be hidden from GDScript and put behind some special syntax, or it should be implemented using a totally different mechanism that doesn't involve signals? I don't think using a signal is any weirder than the fact that yield() returns a function state object which you're supposed to use for dealing with them. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#8573 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ALQCtir3BGGU7vS-Zz4k6r6Tp39fcNxhks5r6NBvgaJpZM4NL0cF>.

[ghost]

Yes, we can keep discussing until eternity until we find the most perfect solution, but it wouldn't be productive. Godot's yield mechanism is weird to me, and clean solution is to have true coroutines with channels.

But given the current state of Godot (yield with function state object), this is a very clean cut solution to the problem, within the current status quo.

[ghost]

Also, adding this signal doesn't prevent adding a syntax sugar for it.

[ghost]

And regardless of the original context, even just for the sake of completeness, I think it's clear that function state should have a completed signal.

[RandomShaper]

The potential new syntax would not be an alternative way of dealing with the signal, but the only documented, reliable way of using the enhanced yield. The signal would probably remain visible (like happens with other signals the engine uses internally) but with an underscore-prefixed name, indicating it's an implementation detail, not intended for direct use, subject to change, etc. So this cannot be done as a two step process because the "syntax sugar" would break compatibility. El 16/05/2017 a las 0:41, Ferenc Arn escribió:

…

Also, adding this signal doesn't prevent adding a syntax sugar for it. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#8573 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ALQCtjwg5LdpFbwtILWVIly_wlkdPD8Sks5r6NSfgaJpZM4NL0cF>.

[ghost]

Forget about the original context or "enhanced yield" or any bikeshed about syntax. Tell me why shouldn't GDFunctionState have a signal indicating its completion? Because that's what this PR is.

[ghost]

Just to be clear, you're talking about blocking a non-invasive, working practical solution to a real-life issue based not on it's technical defects but a hypothetical future solution that you don't have.

[RandomShaper]

This discussion is getting unproductive. Let the project maintainers decide when to merge. If in the meantime (or before 2.1.4, from where there will not be going back) I have time, I'll give this some thought and try to propose something concrete. El 16 may. 2017 1:20 a. m., "Ferenc Arn" <notifications@github.com> escribió:

…

Just to be clear, you're talking about blocking a non-invasive, working practical solution to a real-life issue based not on it's technical defects but a hypothetical future solution that you don't have. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#8573 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ALQCtk8tSdMSDThs0UGzUbLX6Mz8eA38ks5r6N21gaJpZM4NL0cF> .

[akien-mga]

Just to be clear, you're talking about blocking a non-invasive, working practical solution to a real-life issue based not on it's technical defects but a hypothetical future solution that you don't have.

@tagcup, I would encourage you to read https://opensource.com/open-organization/17/2/assuming-positive-intent

@RandomShaper is not a random know-it-better whose opinion should be discarded as irrelevant. He's an active contributor and has often expressed very helpful opinions on various topics, especially API changes. If you don't want to participate in a discussion about a "hypothetical future solution" or better syntax, it's totally fine, but then just let his comment drop without trying to expose his alleged "bikeshedding". I very much prefer discussions to happen before a new API is introduced, than after it landed in a stable release and can't be changed without breaking compatibility.

Now, regarding the actual discussion, I have no opinion myself, but I'm glad that some contributors are reviewing changes and giving feedback (something I'm always asking for and trying to encourage). Having to reach a consensus is a healthy stage in the development of core new features.

[RandomShaper]

@neikeq, what do you think about this?:

• renaming the signal to _completed to mark it as an implementation detail (and as such, keep it undocumented) so that the effective experience is that GDScript supports recursive yielding;
• then adding another built-in function to GDScript that does the same as yield(..., "_completed") while hiding the signaling; not sure if await would be suited here (you can tell as you are more versed in .NET or it may conflict with your other idea); other names could be wait, wait_for, hold or something like those.

[ghost]

@akien-mga Except this has been discussed for over a year #3223 #3235, and I already gave two solutions which would be a totally different yield, a clean wait or await, or adding CSP style channels mechanism which would solve even more than that specific issue.

Now, regarding the actual discussion, I have no opinion myself, but I'm glad that some contributors are reviewing changes and giving feedback (something I'm always asking for and trying to encourage). Having to reach a consensus is a healthy stage in the development of core new features.

Now that's funny because I remember you guys favoring practicality over thinking about better solutions when I defended this exact argument in the past.

He's just talking about implementing exactly the same thing, but with a different syntax. Now if you don't wanna call it bikeshedding because you drank a few beers together, that's fine. I'd rather remain objective.

If you want to see discussion, you'd see to get an answer from him for this:

Tell me why shouldn't GDFunctionState have a signal indicating its completion? Because that's what this PR is.

If he can't answer this, his objection has no value. But go ahead, ignore that question, be with your buddy.

renaming the signal to _completed to mark it as an implementation detail (and as such, keep it undocumented) so that the effective experience is that GDScript supports recursive yielding
When I wrote this:

Are you saying that this signal should be hidden from GDScript and put behind some special syntax, or it should be implemented using a totally different mechanism that doesn't involve signals?

I just wrote it to illustrate how silly those would be.

then adding another built-in function to GDScript that does the same as yield(..., "_completed") while hiding the signaling; not sure if await would be suited here (you can tell as you are more versed in .NET or it may conflict with your other idea); other names could be wait, wait_for, hold or something like those.

This is nothing more that implementing what I said #3235, using this PR as the backend. You're just reiterating what has already been discussed.

[ghost]

And BTW, he is the one talking about adding a new API. Which is not the same thing as adding a new and totally sensible signal isn't.

And again, there is no real reason you can have this signal and add an await function later on if you want (if you ignore "there should be one and only one true way to do a thing" kind like philosophical arguments, which is already not true for GDScript anyway).

[RandomShaper]

I'm starting to think you are not interested in the actual topic, but rather in getting some fun (there are many kinds of people with weird ways of getting fun). If I told you that you're right, would you stop attacking me? Would that satisfy your ego? You are talking as a tyrannical boss, and I'm not your employee. If you can't be respectful online, please stop participating until you learn. Would you be talking to me so harshly if you were in front of me? Your opinion about my proposal is welcome, after all. Our interest is making Godot as good as possible. But forget for how long your issue has been opened and limit yourself to the present. El 16 may. 2017 5:10 p. m., "Ferenc Arn" <notifications@github.com> escribió:

…

And BTW, *he* is the one talking about adding a new API. Which is not the same thing as adding a new and totally sensible signal isn't. And again, there is no real reason you can have this signal and add an await function later on if you want (if you ignore "there should be one and only one true way to do a thing" kind like philosophical arguments, which is already not true for GDScript anyway). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8573 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ALQCtssPsM7Medq3NZnqErQmrHwtrfvmks5r6bxvgaJpZM4NL0cF> .

[akien-mga]

*sigh*

Locking that conversation as it's going nowhere. People from the GH org (esp. @neikeq as PR author and @reduz who should review it) are still free to discuss the actual technical questions here.

[neikeq]

TL;DR: I hate spring.

---

The idea of this PR is to have something working without touching GDScript. We can discuss better solutions that would invole changing GDScript, but I can tell you that any solution would most likely have to use the "completed" signal under the hood to know when a function actually completed.

The following is a solution I had in mind and the problems I found when implementing it:

When I mentioned await I didn't really mean (or not necessarily) implementing the Awaitable pattern like in C#. Just a different syntax, with the purpose of separating it from yield, since yield() without arguments must not be used together with yield on signal.
Yield on signal should also return a different type than GDFunctionState (although it would use GDFunctionState under the hood), to prevent users from resuming it manually.

A problem I found when implementing this is the behavior when a function never yields, e.g.:

func hello_there():
    var ret = await(oh_its_you())
    print(ret)

func oh_its_you():
    if whatever:
        await(timer, "timeout")
    return 10

In the above example, it's possible that the called method never yields, therefore await would error because it expects a value of the awaitable type that is returned by await in the called method. There are 3 possible solutions:

• Make await continue without suspending if the returned type is not our awaitable type.
  • Cons: Allows syntax like var ret = await(10) which would be the same as var ret = 10, while the user could understand that as waiting 10 seconds.
• Force the user to manually wrap the return type, e.g.: return AwaitResult(10).
  • Cons: Well, it forces you to wrap it.
• Make the GDScript compiler or parser detect when a method can yield with await and automatically wrap the return value in such cases, so return 10 would be automatically compiled to return AwaitResult(10).
  • Cons: You tell me, I can't think of anything right now.

[hubbyist]

Make await continue without suspending if the returned type is not our awaitable type.
Cons: Allows syntax like var ret = await(10) which would be the same as var ret = 10, while the user could understand that as waiting 10 seconds.

Is'nt "being not a function" enough criteria to label something as "this will not be awaited"? This would be more simple to detect when parsing IMO. And for "await(10)" parser may give a syntax error I think?

[neikeq]

No, you may want to do something like this:

var awaiter = async_method()
do_something_else()
await(awaiter)

[hubbyist]

If everything is awaitable await() may autocast the return values to AwaitResult before further processing some how? Since this seems as a mandatory part of the await processing anyway I think.

[neikeq]

That's the last point:

• Make the GDScript compiler or parser detect when a method can yield with await and automatically wrap the return value in such cases, so return 10 would be automatically compiled to return AwaitResult(10).

It seems to be the best option, and shouldn't be too hard (I've got a bit more familiar with GDScript source code lately).

[hubbyist]

There must be a difference between, making parser guess the yielding part of code and, making an autocast on return values internally while processing results inside await() function(block), I think. Am I missing something?

[RandomShaper]

Yes, that one looks as the best option. Does "when a method can yield" mean 'when a method contains at least one call to yield()'' or the check would need to be more complex? El 17/05/2017 a las 14:54, Ignacio Etcheverry escribió:

…

That's the last point: * Make the GDScript compiler or parser detect when a method can yield with |await| and automatically wrap the return value in such cases, so |return 10| would be automatically compiled to |return AwaitResult(10)|. It seems to be the best option, and shouldn't be too hard (I've got a bit more familiar with GDScript source code lately). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8573 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ALQCtnRPGWUhjwh9-Gmvn9xB0OFceeSYks5r6u39gaJpZM4NL0cF>.

[RandomShaper]

Ah, no. I'm getting it now.

Do you mean that the thing to be detected is that the method is eventually await()ed so it doesn't return until that point, don't you?

[RandomShaper]

If that's the case, the await() should be in the same scope as the initial call to the async method.

You wouldn't be able to add the awaitables to an array for later processing (potentially during a future frame), for instance.

Perhaps then we should start considering marking awaitable methods with some keyword. That would be added work in the user side, but simpler than wrapping the returns.

That way, one will never forget to wrap any return point, but at the cost of less flexibility:

• every return from an async-tagged method would be intended for being await()ed;
• and, as a consequence, an async method cannot be used synchronously without calling await() on it immediately, but that is no big issue, IMO.

[neikeq]

What I meant by a function that can yield with await is a like the example above:

func oh_its_you():
    if condition:
        await(timer, "timeout")
    return 10

This function can yield with await (although it only does so if the condition is met).

[neikeq]

Here you can see an incomplete implementation of what I was talking about: neikeq@e85fde43

Don't pay too much attention to GDFuncAwaiter and GDAwaitableResult, they are just placeholders that wrap GDFunctionState. I haven't thought of an API for a serious implementation yet.

You can test it with the following script:

extends Node

onready var timer = get_node("Timer")

const YIELD_PLZ = true

func _ready():
    timer.start()
    print("_ready() begin")
    var ret = await(do_something())
    prints("do_something() returned:", ret)
    print("_ready() end")

func do_something():
    print("do_something() begin")
    var ret = await(timer_awaiter())
    prints("timer_awaiter() returned:", ret)
    print("do_something() end")
    return ret


func timer_awaiter():
    print("timer_awaiter() begin")
    if YIELD_PLZ:
        await(timer, "timeout")
        print("timer_awaiter() between two awaits")
        await(timer, "timeout")
    print("timer_awaiter() end")
    return 13

[RandomShaper]

Apologies for the late feedback. Very busy these days.

I'll play a bit with your code, but in the meantime I can tell you this looks nice. Clean and readable.

[karroffel]

@neikeq bump, there are build errors 😛 But you have reduz' consent 😄

[neikeq]

Rebased.
What about the discussion above though? The example I shared could be a better alternative. I'm not available to do it for 3.0 though.

[akien-mga]

What about the discussion above though? The example I shared could be a better alternative. I'm not available to do it for 3.0 though.

I can't say, but since the current code is relatively simple and non intrusive, I guess that it could be merged and either reverted and replaced by something "better" before 3.0, or kept if everyone is happy with it.