Skip to content
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

added signal declaration #76

Closed
wants to merge 1 commit into from

Conversation

geekrelief
Copy link
Contributor

Now we can define signals without having to resort to addUserSignal like this:

import godot
import godotapi / sprite

gdobj MySprite of Sprite:

  signal my_signal(a:int, b:bool, c:string) 

  method ready() =
    self.connect("my_signal", self, "on_my_signal")
    self.emit_signal("my_signal", 123.toVariant, true.toVariant, "^_^".toVariant)

  proc on_my_signal(a:int, b:bool, c:string) {.gdExport.} =
    print &"got signal {a} {b} {c}"

Copy link
Member

@endragor endragor left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the PR, it's much appreciated! I'm sorry it took me long to reply here.

godot/internal/godotinternaltypes.nim Show resolved Hide resolved
methodData*: pointer
freeFunc*: proc (a2: pointer) {.noconv.}

GodotSignalArgument* {.bycopy, packed.} = object
Copy link
Member

Choose a reason for hiding this comment

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

Why is .packed necessary? Is it also a packed struct in Godot? We need to make sure the struct matches Godot ABI to be the same on every platform/arch.

Copy link
Contributor Author

@geekrelief geekrelief Dec 13, 2020

Choose a reason for hiding this comment

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

I document it here. #74 (comment) Basically I was getting a crash with multiple GodotSignalArguments to nativescriptRegisterSignal. GodotSignalArgument has as size of 56 bytes, but the engine was expected 52 bytes. Adding the .packed. pragma gives the right size.

godot/nim/godotmacros.nim Show resolved Hide resolved
of "poolstringarray": VariantType.PoolStringArray
of "poolvector2array": VariantType.PoolVector2Array
of "poolvector3array": VariantType.PoolVector3Array
of "poolcolorarray": VariantType.PoolColorArray
Copy link
Member

Choose a reason for hiding this comment

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

godotTypeInfo(T).variantType could be used instead. That would also allow any type that implements godotTypeInfo, not just a hardcoded set.

godotTypeInfo also provides hint/hintStr that could be used when registering signal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to refresh my memory on this, but I recall looking into godotTypeInfo, but I didn't know how to convert the NimNode to a type to pass into godotTypeInfo.

I see you're using it in registerGodotField but in a template. So I've modified SignalDecl to hold a NimNode for the type instead of a VariantType and then in the createSignalArgument template I call godotTypeInfo to get the variant type. It seems to work.

What's the purpose of mixin for godotTypeInfo? I tried removing it, and it doesn't seem to break anything.

  template createSignalArgument(argName, argTypeIdent)=
    const typeInfo = when compiles(godotTypeInfo(argTypeIndent)):
                      godotTypeInfo(argTypeIdent)
                    else: GodotTypeInfo()
    GodotSignalArgument(name:argName.toGodotString(),
                        typ:ord(typeInfo.variantType),
                        defaultValue: newVariant().godotVariant[])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Saw the deinit message below. I'll modify this to save a reference to the GodotString to deinit.


result.args.add(SignalArgDecl(name: nexpr[0].repr, vtype: vtype))
else:
parseError(sig, "signals only allow colon expressions")
Copy link
Member

Choose a reason for hiding this comment

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

A more user-friendly message would be Signal declaration must have this format: signal my_signal(param1: int, param2: string). People not aware of Nim macros may not understand what a "colon expression" is.

This also concerns the message above about "object construction syntax".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The new parseSignal looks like this:

proc parseSignal(sig: NimNode): SignalDecl =
  let errorMsg = "Signal declaration must have this format: signal my_signal(param1: int, param2: string)"

  if sig[0].strVal != "signal":
    parseError(sig, errorMsg)
  if sig.kind != nnkCommand:
    parseError(sig, errorMsg)
  if not (sig[1].kind == nnkCall or sig[1].kind == nnkObjConstr):
    parseError(sig, errorMsg)

  result = SignalDecl(
    name: $sig[1][0],
    args: newSeq[SignalArgDecl](),
  )

  if sig[1].kind == nnkObjConstr:
    for i in 1..<sig[1].len:
      var nexpr = sig[1][i]
      case nexpr.kind:
      of nnkExprColonExpr:
        result.args.add(SignalArgDecl(name: nexpr[0].repr, typ: nexpr[1]))
      else:
        parseError(sig, errorMsg)

Thoughts?

Copy link
Contributor Author

@geekrelief geekrelief Dec 14, 2020

Choose a reason for hiding this comment

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

In the latest PR I moved the check for signal to parseType. So in future if we need other commands we can switch on the command value.

godot/nim/godotmacros.nim Show resolved Hide resolved
template createSignalArgument(argName, argType)=
GodotSignalArgument(name:argName.toGodotString(),
typ:argType,
defaultValue: newVariant().godotVariant[])
Copy link
Member

Choose a reason for hiding this comment

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

This would be unsafe with GC enabled. Instead, initGodotVariant should be used with deinit afterwards.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I'm doing this now:

 template initSignalArgumentParameters(argName, argTypeIdent, typeInfoIdent,
                                        godotStringIdent, godotVariantIdent)=
    var godotStringIdent = argName.toGodotString()
    mixin godotTypeInfo
    const typeInfoIdent = godotTypeInfo(argTypeIdent)
    var godotVariantIdent:GodotVariant
    initGodotVariant(godotVariantIdent)

  template deinitSignalArgumentParameters(godotStringIdent, godotVariantIdent)=
    godotStringIdent.deinit()
    godotVariantIdent.deinit()

  template createSignalArgument(typeInfoIdent, godotStringIdent, godotVariantIdent) =
    GodotSignalArgument(name: godotStringIdent,
                        typ:cast[cint](ord(typeInfoIdent.variantType)),
                        defaultValue: godotVariantIdent)

  template registerGodotSignal(classNameLit, signalName, argCount, sigArgs) =
    var sigArgsArr = sigArgs
    var godotSignal = GodotSignal(
      name: signalName.toGodotString(),
      numArgs: argCount,
      args: addr(sigArgsArr[0]))
    nativeScriptRegisterSignal(getNativeLibHandle(), classNameLit, godotSignal)

I don't know why, but when I use const typeInfoIdent = when compiles(godotTypeInfo(argTypeIndent)): it fails, so the VariantType is always nil. Do you know what's causing that?

It's also odd I need to cast ord to cint when I didn't have to before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmmm not sure what I did, but the when compiles and ord issue went away after I changed some things. I pushed a new PR as a result.

godot/nim/godotmacros.nim Show resolved Hide resolved
godot/nim/godotmacros.nim Show resolved Hide resolved
@geekrelief geekrelief closed this Dec 13, 2020
@geekrelief geekrelief deleted the added_signals branch December 13, 2020 22:50
This was referenced Dec 14, 2020
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