Registering custom signals with multiple arguments

[arunvickram]

Hi everyone, I've been looking through the information trying to see how to create signals with multiple arguments, and I was wondering how to go about creating signals with arguments in Nim?

I know that something like

gdobj CharacterStats of Node:
  method init*() =
    self.addUserSignal("health_changed")

is somewhat the equivalent of the following GDScript:

extends Node

signal health_changed

I'm still unsure how to write the equivalent GDScript:

extends Node
signal health_changed(amount)

Thanks for all the help so far!

[zetashift]

I haven't done this before but according to : https://pragmagic.github.io/godot-nim/v0.5.4/godotapi/objects.html#addUserSignal,Object,string,Array proc addUserSignal*(self: Object; signal: string; arguments: Array) {..}

you should be able to do or something like this atleast:

gdobj CharacterStats of Node:
  method init*() =
    let amount = 5
    self.addUserSignal("health_changed", newArray(amount.toVariant))

[arunvickram]

That's what I figured and hoping wouldn't be the case. I was hoping that at least you could do something like

self.addUserSignal("health_changed", newArray(VariantType.Int))

I'm still unsure whether or not the newArray passed to addUserSignal would contain elements such as 0.toVariant or whether the number passed into the newArray would really just be the numerical representation of the VariantType enum, like VariantType.Int = 1

[zetashift]

I actually don't know much about this either and googling this didnt get me far. But: https://docs.godotengine.org/en/stable/tutorials/plugins/gdnative/gdnative-cpp-example.html#signals says:

Here we see a nice improvement in the latest version of godot-cpp 
where our register_signal method can be a single call first taking the signals name, 
then having pairs of values specifying the parameter name and type of each parameter 
we'll send along with this signal.

However I cannot read C++ haha.

[arunvickram]

It looks like this would require a change in the genType function to use Nativescript 1.1 and register signals along with registering methods https://github.com/pragmagic/godot-nim/blob/f25c3c7a5dbe2b6ac8e4a3407e5c03751658e401/godot/nim/godotmacros.nim#L440

[arunvickram]

It would also likely require a change to parseType to parse custom signal declarations like this:

signal healthChanged(amount: int)

[endragor]

This is how you can do it:

self.addUserSignal("health_changed", newArray([{"amount": TYPE_INT}.toTable()]))

TYPE_xxx constants can be found in global_constants.nim. A small improvement could be to implement a toVariant/fromVariant conversion for a seq of pairs. Then the .toTable() part won't be necessary.

[geekrelief]

self.addUserSignal("health_changed", newArray([{"amount": TYPE_INT}.toTable()]))

hmm, I tried using this, and I get a type mismatch error for newArray since there isn't a proc that accepts Table.

This is how I've added a signal in the past. But it's pretty verbose:

  import godotapi / [global_constants]
  var arg0 = newDictionary()
  arg0["name".toVariant] = "amount".toVariant
  arg0["type".toVariant] = TYPE_INT.toVariant
  var args = newArray(arg0.toVariant) # each dictionary adds an argument
  self.addUserSignal("health_changed", args)

[endragor]

Trying changing array to sequence (add @). This should work:

self.addUserSignal("health_changed", newArray(@[{"amount": TYPE_INT}.toTable()]))

[arunvickram]

@endragor Does the register_signals method in the C++ version register them so that the signals are viewable in the Godot editor? If so, that's what I was alluding to and was wondering whether that would be a better way to go about it?

[geekrelief]

Trying changing array to sequence (add @). This should work:

self.addUserSignal("health_changed", newArray(@[{"amount": TYPE_INT}.toTable()]))

Yeah, that's not working either. Here's the truncated error.

Error: type mismatch: got <seq[Table[system.string, system.int64]]>
but expected one of:
proc newArray(arr: GodotArray): Array
  first type mismatch at position: 1
  required type for arr: GodotArray
  but expression '@[toTable([("amount", 2'i64)])]' is of type: seq[Table[system.string, system.int64]]
...

Looking at the code in arrays.nim, I don't see how newArray accepts a seq or a Table. The only proc that would make sense is for openarray[Variant], but the arguments would need to have toVariant called on it right?

In anycase, I just followed the docs for https://docs.godotengine.org/en/stable/classes/class_object.html#class-object-method-add-user-signal and that's how I arrived at the code that uses a Dictionary per argument with "name" and "type" keys. It would be nice to have a cleaner have a cleaner way to set this up, otherwise I'll write a macro.

[arunvickram]

@geekrelief this is the same error that I'm running into atm, which is partially why I suggested looking into calling register_signal in the same genType type call that registers the methods and the variables to be exported. I could be wrong, but it seems like that might be more idiomatic Godot thing to do.

[geekrelief]

@arundilipan Yeah, I hear. I've considered modifying stuff in godotmacros.nim for other things like allowing let variables for instance or other macros. But I've resorted to creating my own macros to call with methods/procs. We would need to modify parseType and genType to allow things other than var, proc, and methods.

I'll try playing around with it tomorrow.

[zetashift]

@geekrelief any code you could share I think it could come in handy for future people looking into these stuff.

EDIT: nvm I see the gdnim repo.

[geekrelief]

@zetashift Feel free to create issues on the gdnim repo if you have questions or ping me on discord. Been looking to share, but I'm not sure what's the best/appropriate way to reach out to our tiny godot-nim community.

[geekrelief]

Anyone have a suggestion on how to get nim to recognize this syntax?

signal my_signal(a:bool = true)

Nim is erroring at the '=' thinking my_signal is using an Object construction syntax when what we want is a proc syntax for default arguments. And it would be nice to have that for default arguments for signals to match gdscript.

signal my_signal(a:bool)

works fine gut no default arguments.

signal my_signal(a:bool):
  a = true

Works too, but not as pretty.

Edit:

signal my_signal(a = true, b:int)

Is another option, it produces an AST I can work with.

[arunvickram]

@geekrelief how did you get Nim to recognize that syntax?

[geekrelief]

@arundilipan Not sure which syntax are you talking about. But if you do a

dumpAstGen:
  signal my_signal(a=true, b:int)

You get:

nnkStmtList.newTree(
  nnkCommand.newTree(
    newIdentNode("signal"),
    nnkCall.newTree(
      newIdentNode("my_signal"),
      nnkExprEqExpr.newTree(
        newIdentNode("a"),
        newIdentNode("true")
      ),
      nnkExprColonExpr.newTree(
        newIdentNode("b"),
        newIdentNode("int")
      )
    )
  )
)

[arunvickram]

@geekrelief how are you trying to go about parsing that syntax? Are you trying to do it through the genType function or are you making a separate macro called signal?

[geekrelief]

Going through parseType/genType is the only way. Right now parseType throws away anything that isn't a var, proc, or method. So I'm modifying parseType to use the above signal syntax that allows for default arguments.

Edit: FYI, I'm dealing with some plumbing issues right now, so bear with me if I'm slow to respond. :)

[geekrelief]

So I got word that the proc syntax is only possible if the proc keyword is used. https://forum.nim-lang.org/t/7148

Do you think default arguments are necessary? I'm thinking about implementing two versions of signal, where one accepts a body that will initialize any default arguments.

signal my_signal(a:bool)
# or
signal my_signal(a:bool):
  a = true

Thoughts? Anyway, going to work on the one with out default args first.

Edit: What do you think of the proc syntax?

proc my_signal(a:bool = true) {.signal.}

[geekrelief]

My personal vote is for:

signal my_signal(a:bool): a = true

I think the proc version would be more error prone or confusing if you forget the .signal. pragma.

[zetashift]

My personal vote is for:

signal my_signal(a:bool): a = true

I think the proc version would be more error prone or confusing if you forget the .signal. pragma.

I like this version fwiw!

[arunvickram]

My personal vote is for:

signal my_signal(a:bool): a = true

I think the proc version would be more error prone or confusing if you forget the .signal. pragma.

I second this syntax for sure. I think the problem with using proc is that semantically, Godot signals aren't functions, they're data. It definitely makes more sense to use that syntax because it's very similar to GDScript. If we wanted to use a pragma, we could something like this:

type mySignal {.signal.} = object
  a: bool
  b: bool

where {.signal.} modifies the type declaration to adhere to some concept Signal that allows us to call registerSignals or addUserSignal.

[arunvickram]

@geekrelief I tried working on that solution, any progress? I'm not great with nim macros so that's probably why I'm still struggling

[geekrelief]

@arundilipan I had to deal with some stuff, and fighting a cold too. So I haven't had a whole lot of time to tackle the issue. Thanks for attempting it. Like you I'm somewhat new to nim / godot and macros too. Learning as I go. I did take a closer look at things last night, and it's turned out to be a little trickier than I thought cause I was trying to mimic the godot-nim code base of using templates for registering stuff with godot, but I think I'll just go with a macro and slap something together to get it working first and clean it up afterwards based on feedback.

[zetashift]

@geekrelief Get well soon!

[geekrelief]

Got the most basic signal working

  signal test_sig()
  method ready() =
    discard self.connect("test_sig", self, "on_test_sig")
    self.emitSignal("test_sig")

  proc on_test_sig() {.gdExport.} =
    print "got test_sig"

next working on parameters

[geekrelief]

So with parameters I'm trying to think of an elegant way to go from the parameter type to VariantType. Say a signal has parameters:

signal my_signal(a:bool, b:Vector2, c:PoolByteArray)

I guess for now I'll just match the type strings to VariantType enum in a big case statement. And circle back later to make it more elegant. Going to take a break and circle back to this later tonight.

[arunvickram]

@geekrelief First of all, I hope you get well soon! Second, that's exactly what my macro was trying to do. Basically it takes the type definitions of the declaration and it matches against the VariantType enum for right now. I think for most cases that is plenty, because I haven't figured out how to create converters for Variants

[geekrelief]

Code isn't pretty but I got parameters working. Tomorrow I'll work on default arguments and clean things up.

  signal test_sig(a_bool:bool, a_int8:int8, a_string:string)

  method ready() =
    discard self.connect("test_sig", self, "on_test_sig")
    self.emitSignal("test_sig", true.toVariant, 123.toVariant, "hello".toVariant)
    self.emitSignal("test_sig", false.toVariant, (-128).toVariant, "world".toVariant)

  proc on_test_sig(a_bool:bool, a_int8:int8, a_string:string) {.gdExport.} =
    print &"got test_sig {a_bool = } {a_int8 = } {a_string = }"

output:

got test_sig a_bool = true a_int8 = 123 a_string = hello
got test_sig a_bool = false a_int8 = -128 a_string = world

Edit: So, yes it works, but the generated code isn't passing the signal arguments correctly. See below.

[geekrelief]

oh wow, I just discovered, that you can register a signal without any arguments, call emitSignal with arguments and still receive them! So all you have to do is specify the signal name. WTF? bug is a feature?

[geekrelief]

Hmm I'm running into an issue passing the GodotSignalArguments. I create an array of GodotSignalArgument and pass the address of the first element to GodotSignal's args member, but godot crashes when trying to access the second GodotSignalArgument.

This is the output from expandMacros:

var sigArgs_369099033 = [GodotSignalArgument(name: toGodotString("a_bool"), typ: 1), 
                         GodotSignalArgument(name: toGodotString("a2_bool"), typ: 1)]
var godotSignal`gensym10 = GodotSignal(name: toGodotString("test_sig"),
                                       numArgs: 2, args: addr(sigArgs_369099033[0]))
nativeScriptRegisterSignal(getNativeLibHandle(), "TestComp",
                           godotSignal`gensym10)

Running in the debugger, looks like the next entry points to garbage. Anyone have a clue what's up?

[geekrelief]

I wonder if it's related to this https://github.com/godotengine/godot-cpp/issues/473

I'm getting a crash here: https://github.com/godotengine/godot/blob/3.2/modules/gdnative/nativescript/godot_nativescript.cpp#L155

[0] atomic_conditional_increment (C:\godot\geekrelief_godot\core\safe_refcount.cpp:122)
[1] atomic_conditional_increment (C:\godot\geekrelief_godot\core\safe_refcount.cpp:122)
[2] godot_nativescript_register_signal (C:\godot\geekrelief_godot\modules\gdnative\nativescript\godot_nativescript.cpp:155)
[3] NimMain
[4] NimMain
[5] godot_nativescript_thread_exit
[6] NimMain
[7] godot_nativescript_init

Edit: Definitely, seems related. To @o01eg's issue. From godot-nim's side GodotSignalArgument is 56 bytes and on the godot engine side, godot_signal_argument is 52 bytes.

[geekrelief]

Hmm why is GodotSignalArgument's size 56 when all of its components add up to 52? diff = 4 = sizeof(GodotSignalArgument) = 56 - ( name = 8 + typ = 4 + hint = 4 + hintString = 8 + usage = 4 + defaultValue = 24)

[geekrelief]

Fixed the issue. GodotSignalArgument needs a .packed. pragma. I was thinking it was some kind of alignment issue!

Edit: Reminder to self: godot/internal/godotinternaltypes.nim needs change to GodotSignalArgument with .packed. pragma.

[geekrelief]

Awesome! Got parameters working finally!

[geekrelief]

FYI, the default arguments seem backward. The engine code is looping through the signals from front to back to get their default value.

    for (int i = 0; i < p_signal->num_default_args; i++) {
        Variant *v;
        godot_signal_argument attrib = p_signal->args[i];

        v = (Variant *)&attrib.default_value;

        default_args.push_back(*v);
    }

So if you have a signal like:

  signal my_signal(a_bool:bool, a_string:string):
    a_string = "hello"

Wouldn't work. You'd have to specify a_bool with the default argument. Is this the same for gdscript?

[geekrelief]

Going through the docs it doesn't even look like default arguments for a signal are a thing. https://docs.godotengine.org/en/stable/development/file_formats/gdscript_grammar.html The gdscript grammar doesn't allow for it. I'm going to skip implementing default arguments for now.

Here's my code: https://github.com/geekrelief/godot-nim/tree/added_signals I updated the example in the docs for nim/godotmacros.nim.

Try it out and let me know if you have any issues.

[geekrelief]

Whoops I forgot I removed some old code in those files, I need to add back for older versions of nim. Edit: patched it up

[arunvickram]

@geekrelief This is fantastic! I have one question: if you add the compiled nativescript file to a node in Godot, does it show you the signals that have been registered in genType?

[geekrelief]

@geekrelief This is fantastic! I have one question: if you add the compiled nativescript file to a node in Godot, does it show you the signals that have been registered in genType?

You mean like this?

[arunvickram]

Ok I figured out why it didn't work for me earlier, @geekrelief, it's cause the signals have to be in snake_case. But man you're the GOAT.

[geekrelief]

Ok I figured out why it didn't work for me earlier, @geekrelief, it's cause the signals have to be in snake_case.

cool, yeah seems like anything you send to godot should be snake_case 👍

[geekrelief]

Ok I figured out why it didn't work for me earlier, @geekrelief, it's cause the signals have to be in snake_case. But man you're the GOAT.

Thx, but the real GOAT is @endragor. :)

[geekrelief]

@arundilipan I wonder is there an elegant way to deal with this case sensitivity? Is it the receiving function that needs to be snake_case or the signal name or both?

[arunvickram]

So at the moment, the way it works it seems is that when the methods get compiled, the method names get converted into snake_case, so it seems as though if we're registering the signal with a method, the method name has to be in snake_case when we add the signal to the method.

[geekrelief]

So at the moment, the way it works it seems is that when the methods get compiled, the method names get converted into snake_case, so it seems as though if we're registering the signal with a method, the method name has to be in snake_case when we add the signal to the method.

Cool. Yeah that's "expected" behavior when it comes to godot-nim. I think we're good for now so I put out a PR.

[geekrelief]

Hey guys, I just pushed a new PR based on endragor's feedback. It fixes a potential, unsafe memory issue when using GC and registering a signal with arguments. Please check it out and let me know if you run into any issues.

[arunvickram]

Will do. I noticed that that you had a setup where you were able to reload Nim code into Godot relatively easily, and I was wondering if there was a way we could get that working with nake? I'm fairly ignorant of the nim build system so I'm not sure about how we could get it working on Linux or Mac?

[geekrelief]

Yeah there's probably a way to make it work with nake, but I'm not a fan. Nim has a mismash of configuration files, nimscript, nimble and nake. From what I know nake is deprecated and the future is nimscript. And it was really confusing for a newbie like me that was trying to learn nim and godot-nim several months ago.

Besides using nimble to install libraries, I don't see much of an advantage in using nake or nimble/nimscript. You can't use exportc functions from nimscript so checking file modification times isn't possible. Godot-nim uses all three and it was very confusing to wade through all of it. I wanted to try hacking on godot-nim for hot reloading and try out ARC. So I hacked together my own build system which mimics nimble tasks and I'm happy with it. It's super easy to modify and is just plain nim + a little cfg file. I use my build system to do everything from rebuilding the godot engine, generating the godot-nim api, generating and building components, and launching godot.

Anyway, I think zetashift was trying to get gdnim working for Linux. But I'm not sure if he was successful. If you're interested in checking it out, feel free to chat me up on discord if you have further questions. The gdnim README should give you an overview of how it works.

Edit: I forgot to mention, my system won't work without an engine patch, so updating nake alone is insufficent. I tried 5-6 different ways to get things working (lots of crashes and tears) and using reloading dlls via resources was the most robust.