added signal declaration

[geekrelief]

I think I've fixed all the issues mentioned in the prior PR #76. I fixed the when compiles(godotTypeInfo issue and removed the call to ord. Also moved the check for the signal command outside of parseSignal in case we want to add additional commands for other things in the future then we can switch on the value of statement in parseType. So that's why I pushed a new PR.

Saving the references for the parameters for GodotSignalArgument and deinitializing them afterwards seems a little messy. If you have suggestions there, I can change it. Maybe switching to a macro and using quote do: might be nicer? But I was trying to match the rest of the code's style.

[endragor]

There is no need to create separate PRs. You may simply force push to your branch to update an existing PR.

[geekrelief]

Thanks for the tip!

[geekrelief]

#80 I updated the PR to also add async handling of signals.

[endragor]

I'd rather have these features separate. Async handling requires discussion and better async support from Nim. Using a Future creates a cyclic GC-reference (because callback closure has to capture the Future). This makes it necessary to invoke mark-and-sweep procedure of Nim GC. Its complexity depends on the number of alive (rather than dead) objects. So if you have 1 GB of objects allocated, GC will walk through all of them. This will create a very noticeable hiccup. Therefore you mustn't create reference cycles in Nim game code.

Let's deal with signals in general first, and async support later. Besides, async can be implemented on user side for those who are ready to deal with those problems (for example, we use fork of Nim where async code doesn't create reference cycles). Something like this should work:

gdobj AsyncSignalHelper of Reference:
  var callback: proc (val: Variant) {.closure, gcsafe.}
  proc onSignal(val: Variant) {.gdExport.} =
    callback(val)

proc signal*[T](obj: Object, signal: string): Future[T] =
  result = newFuture[T]("waitForSignal")
  let fut = result
  proc callback(val: Variant) =
    fut.complete(fromVariant[T](val))

  let helper = gdnew[AsyncSignalHelper]()  
  helper.callback = callback
  obj.connect(signal, helper, "on_signal", flags = CONNECT_ONESHOT)

# now you can use ``await signal(obj, "some_signal")``

This requires adding extra helper implementations for different number of parameters, but it's not a big deal.

[geekrelief]

Thanks for taking a look!

Correct me if I'm wrong, but I'm not defining a closure, so would GC still be an issue? I parse the async method looking for on_signal, so I can define the Future, callback, and on_signal proc as class members. The resulting code would have a form like this:

import godot
import godotapi / [timer]
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 on_signal(self, "timeout") becomes
    await self.on_signal_fire_timer_self_timeout(self, "timeout")
    print "timeout 1"
 
  # Future, callback, on_signal are generated as class members
  var f_fire_timer_self_timeout:Future[void]

  proc cb_fire_timer_self_timeout() {.gdExport.} =
    self.f_fire_timer_self_timeout.complete

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

I'm generating an id to append to the identifier of the Future, callback, and onSignal proc here: geekrelief@a723bae#diff-7fce0e158180be6b3cc107e90b800160e3f996d86489ddcce84e7140a1c67132R302-R310

Also, I'm checking the Future return type for a tuple so we can receive multiple values

    var vals = await on_signal(self, "bsclick", tuple[button_idx:int, shape_idx:int])
    print &"bsclick {vals.button_idx = } {vals.shape_idx = }"

You're way more experienced than me on nim, so I'll defer to your wisdom there. Do you want me to repush the PR minus the async handling? I separated the PRs.

[geekrelief]

(for example, we use fork of Nim where async code doesn't create reference cycles).

Have you made this fork available anywhere? I'd like to improve my understanding of the compiler, and the issue better.

[endragor]

Correct me if I'm wrong, but I'm not defining a closure, so would GC still be an issue?

await defines a closure internally. Since the closure doesn't accept Future as a parameter, it has to capture it. And the Future stores the callback list. That's the cycle. You can see the code here: https://github.com/nim-lang/Nim/blob/e70ac0f34c9f195f6e423b3ac7e54b912e62c71d/lib/pure/asyncfutures.nim#L23

So until that is fixed, I wouldn't add anything async-related to godot-nim. As I mentioned, it's possible to add that on user side, without changing the library. But again, it would be advised against, because of the current async implementation.

Have you made this fork available anywhere? I'd like to improve my understanding of the compiler, and the issue better.

It's not publicly available. But for this particular problem we just provide Future as a parameter when the callback is invoked. {.async.} macro implementation also has to be changed to always use the callback version with the parameter, rather than capture the Future.

[geekrelief]

Thanks for the explanation. I think I get it despite not knowing all the details. I'll make an effort to study nim's asyncfutures.nim and asyncmacro.nim.

[samdze]

Is there something that keeps this PR from being merged?

[DarthvaderBn]

Why this PR is still not merged?

[ghost]

Only thing holding me back from using these nim bindings with godot is the signal support along with the lack of yield/await for signals. Any idea when this could be merged?

[Zireael07]

Seconding the above questions.