Yield on signal implementation?

[arunvickram]

So what's your strategy for doing this? To be honest I looked at this a while ago, and didn't understand nim's async and godot-nim well enough at the time to move forward.

Here are my thoughts on this so far. I can see how we'd used asynchdispatch's poll in the method process to process Futures. We'd explicitly have to add something like:

import asynchdispatch
gdobj ...
  method process() =
    if hasPendingOperations():
      poll(0)
    ...

So Futures will complete.

Then we'd tag a function with {.async.}, and since that function returns a Future when invoked. Inside that function we would then use await on some function, call it onSignal, that returns a Future used in a callback for connect.

  method ready() =
    # created self.timer
    asyncCheck self.onTimer()

  proc onTimer() {.async.} = 
    await onSignal(self.timer, "timeout") #onSignal returns a Future[void], await on it to block
    print "timer timeout"

What I'm trying to figure out is how to create/reference the callback to be passed to connect when it only accepts a string for the callback? I don't think we can automatically generate the callback inside of onSignal without passing the signal parameter types since a signal api is not part of gdnative. Take for example, Area2D has a area_shape_entered ( int area_id, Area2D area, int area_shape, int self_shape ) signal, we'd need to pass the signal parameters explicitly, or modify the gdnative api to include signals.

  proc somefunc() {.async.} =
    await onSignal(self.area2D, "area_shape_entered", proc(area_id:int, area:Area2D, area_shape:int, self_shape:int))
    print &"{area_shape=} entered"
    ...

So assuming we have all the info we need to generate a callback what are the options for passing it to connect? Currently callbacks are class member procs that need {.gdExport.}, which uses nativeScriptRegisterMethod to register it with godot. But we can't use {.gdExport.} on a callback inside of a proc. So onSignal needs a new macro to process it, call it genSignalCallback, that will genSym a name for the callback, register it with godot, then pass the callback name to connect. But the problem with that is genSignalCallback needs to write to the AST generated by godotmacros gdobj. I don't know if that's even possible "out-of-band", so we'd have to parse procs when gdobj is generating the AST to add the generated callback as another method to be registered. Since only {.async.} functions can contain await onSignal we'd only have to parse those functions.

Hmm so this seems doable. Thoughts?

Originally posted by @geekrelief in #74 (comment)

[arunvickram]

Looking at the way ECMAScript is doing it, they modified the godot.Object.connect function to allow you to pass in anonymous functions instead of strings that reference the object's instance methods. So what I was thinking was let's make another connect where, instead of passing a string that references the instance method to be called, you pass a proc that handles the signal. That may require macros or templates, but the benefit is we can do something like this instead:

method ready() =
  self.connect("signal", self) do (data: string) -> void:
    # do stuff here

If we figure this out, then what we can do is create Future[T]s using asyncdispatch/asyncfutures in a callback function and attach that the signal just in ECMAScript. What do you think?

[arunvickram]

@geekrelief I opened the thread. We can discuss things here.

So onSignal needs a new macro to process it, call it genSignalCallback, that will genSym a name for the callback, register it with godot, then pass the callback name to connect. But the problem with that is genSignalCallback needs to write to the AST generated by godotmacros gdobj. I don't know if that's even possible "out-of-band", so we'd have to parse procs when gdobj is generating the AST to add the generated callback as another method to be registered. Since only {.async.} functions can contain await onSignal we'd only have to parse those functions.

I'm gonna revisit the signal macro code, although I think your proposal is fairly sound. On top of that, I think adding an anonymous proc is potentially doable because iirc all it requires is for us to register anonymous procs. The reason I mentioned the anonymous procs for connect is because the ECMAScript implementation leverages that ability to do their signal await, although we can do it through macros as well.

[geekrelief]

Yes, one way or another we need to register the callback proc with godot. Actually, I have some proof of concept code. But I need to implement things on the macro side.

import godot
import godotapi / [timer]
import hot
import asyncdispatch

gdobj TimerComp of Timer:

  method ready() =
    registerFrameCallback(
      proc() =
        if hasPendingOperations():
          poll(0)
    )
    asyncCheck self.fireTimer()

  proc fire_timer() {.async.} =
    self.one_shot = true
    self.start(1)
    await self.onSignal(self, "timeout")
    print "timeout 1"
    self.one_shot = true
    self.start(1)
    await self.onSignal(self, "timeout")
    print "timeout 2"

  var f:Future[void]

  proc callback() {.gdExport.} =
    self.f.complete

  proc onSignal(target:Object, signalName:string):Future[void]  =
    self.f = newFuture[void]("onSignal")
    discard target.connect(signalName, self, "callback", flags = CONNECT_ONESHOT)
    self.f

Somewhere in the gdobj macro we'll check for a proc with {.async.}, then look for onSignal and generate a future, callback, and onSignal specific to the target, signal and its parameters. Maybe the macro can also call registerFrameCallback to pump the async dispatcher too.

I'm aiming for something like this.

 proc fire_timer() {.async.} =
    self.one_shot = true
    self.start(1)
    onSignal(self.timer, "timeout")
    print "timeout"
    onSignal(self.area2d, "area_entered", (area:Area2D, shape_idx:int, ...))
    print &"{area=}"
    onSignal(self.area2d, "area_entered", (area2:Area2D, shape_idx2:int, ...))
    print &"{area2=}

So you can call onSignal(target:Object, signalName:string,[(parameters)]). So, what happens if you have a multiple onSignals with the same Future signature? Like Future[void] in different {.async.} procs. You're going to need a future for each of them. And if those futures return values like for signal Area2D.area_entered those parameter names should be unique within the async proc. So the macro needs to track each onSignal(target, signalName, parameters) for each async proc.

I need to work on some other stuff at the moment, but hopefully I can start on this later tonight or tomorrow.

[arunvickram]

No problem, I've been splitting my time among different projects and so that's why I haven't been as attentive to this as I want to be at the moment.

[geekrelief]

I got this working on gdnim https://github.com/geekrelief/gdnim/blob/master/components/sprite_comp.nim.

[geekrelief]

Updated my PR for signals with async handling. #79

[arunvickram]

Nice, I've been setting up my Linux (NixOS) environment on my desktop, and I'm still trying to get Godot to compile.

[geekrelief]

@endragor left a comment on my PR, so there might be some changes coming up with regards to implementation or api. It's pretty much my first draft, but you can try it out to see if its "ergonomical" or anything obviously broken.

[geekrelief]

Separated the PR for async handling. #83

[geekrelief]

Based on my conversation with endragor. I don't think this will ever make it into godot-nim unless there's an async/await rewrite to remove closure cycles. My undertanding is still a bit murky on this, but because of the way async generates a callback closing over Future, it'll force the GC to run which he doesn't want. So it's up to the end users to decide if they want to implement / incorporate this.

This article on ARC / ORC mentions it in the ORC section, so this might never get fixed.
https://nim-lang.org/blog/2020/10/15/introduction-to-arc-orc-in-nim.html
On the other hand, I really wonder how bad this would be in practice.

[geekrelief]

Here's more info https://news.ycombinator.com/item?id=24800161

leorize
I believe the phrasing might not be clear to everyone, so here's a reduced version:

• Nim's current async implementation creates a lot of cycles in the graph, so ARC can't collect them.
• ORC is then developed as ARC + cycle collector to solve this issue, and it has been a success.

[geekrelief]

I fixed the issue I was having with ORC and gdnim. So I've made it the default gc so we can collect those async cycles.

[arunvickram]

@geekrelief So if I'm understanding this correctly, the solution works because ORC is designed to handle async?