-
Notifications
You must be signed in to change notification settings - Fork 26
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
Nim 0.19/devel compatibility #42
Conversation
I wasn't on the latest devel. If I use latest devel I don't get the error above, but rather the compiler crashes with an internal error :(. This is also what the travis build says. Hm, pretty hard to reproduce because it involves so much auto-generated code...
|
The internal error was caused by this Nim issue: nim-lang/Nim#10136 Fortunately it should be simple to work-around the issue, because the Now I'm again stuck with the issue in the first post :(. |
It looks like the problem is this Nim bug: nim-lang/Nim#7375 Looking for a work-around... |
Trying to run this with the latest devel which fixed the typedesc
Also got a nakefile.nim error but that was solvable by removing the isNil check. |
I don't know if that's the expected behaviour, but I think the problem is that the return value of toGodotName is set to string. This way the returned string literal is immedietly treated as a non const(aka static) string. It's probably solved by changing the return type to untyped. |
Changing the return type of toGodotName to
But yeah my metaprogramming experience is really limited so I'm not sure what I'm doing. |
For me this works fine with a ~2 days old devel. The travis builds were successful as well back then. Can we retrigger them to see if travis can reproduce the problem? Note that I had a workaround for the typedesc |
the `is issue has been solved in latest devel though: nim-lang/Nim@ab99bdf |
So I got around this error but now I'm getting linking errors(which is weird other Nim projects of mine compile fine), using Window 10 x64:
Edit: I'm a fool: #35 |
Any news on this one? We would like to include |
It's been a while that I worked on this, but from my point of view it was ready to merge... @endragor doesn't seem very active recently. Anyone else with merge rights on this repo? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to keep the library compatible with the earlier version of Nim (0.18.0), because it's being used in production. I pointed out where incompatibilities currently exist. Would you be able to update the PR based on the comments I provided?
Thanks for your work and apologies for the huge delay in responding.
godot/nim/godotmacros.nim
Outdated
@@ -197,7 +197,7 @@ proc parseType(definition, callSite: NimNode): ObjectDecl = | |||
parseError(option, "valid type specifier expected") | |||
result.isTool = isTool | |||
|
|||
if result.parentName.isNil: result.parentName = "Object" | |||
if result.parentName == "": result.parentName = "Object" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use .len == 0
check to keep this compatible with earlier versions of Nim.
godot/nim/godotmacros.nim
Outdated
@@ -447,7 +444,7 @@ proc genType(obj: ObjectDecl): NimNode {.compileTime.} = | |||
let objTy = newNimNode(nnkObjectTy) | |||
typeDef.add(newNimNode(nnkRefTy).add(objTy)) | |||
objTy.add(newEmptyNode()) | |||
if obj.parentName.isNil: | |||
if obj.parentName == "": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use .len == 0
check to keep this compatible with earlier versions of Nim.
godot/nim/godotmacros.nim
Outdated
@@ -506,23 +503,25 @@ proc genType(obj: ObjectDecl): NimNode {.compileTime.} = | |||
result.add(meth.nimNode) | |||
|
|||
# Register Godot object | |||
let parentName = if obj.parentName.isNil: newStrLitNode("Object") | |||
let parentName = if obj.parentName == "": newStrLitNode("Object") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use .len == 0
check.
godot/nim/godotmacros.nim
Outdated
else: newStrLitNode(obj.parentName) | ||
let classNameLit = newStrLitNode(obj.name) | ||
let classNameIdent = ident(obj.name) | ||
let isRef: bool = if obj.parentName.isNil: false | ||
let isRef: bool = if obj.parentName == "": false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use .len == 0
check.
godot/nim/godotnim.nim
Outdated
@@ -204,7 +202,7 @@ macro baseNativeType(T: typedesc): cstring = | |||
if typeName == "NimGodotObject": | |||
break | |||
t = getType(t[1][1]) | |||
if baseT.isNil: | |||
if baseT == "": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use .len == 0
check.
godot/nim/godotnim.nim
Outdated
|
||
macro toGodotName(T: typedesc): string = | ||
var godotName: string | ||
proc toGodotName(T: typedesc): string = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mark this explicitly as {.compileTime.}
to make sure it's never executed in runtime.
godot/nim/godotnim.nim
Outdated
@@ -717,7 +709,7 @@ proc fromVariant*(s: var string, val: Variant): ConversionResult = | |||
if val.getType() == VariantType.String: | |||
s = val.asString() | |||
elif val.getType() == VariantType.Nil: | |||
s = nil | |||
s = "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be set to nil
in versions prior to 0.19.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would one achieve this? Using a when
switch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, such as when (NimMajor, NimMinor, NimPatch) < (0, 19, 0):
@@ -546,15 +545,15 @@ proc genType(obj: ObjectDecl): NimNode {.compileTime.} = | |||
methodNameLit, minArgs, maxArgs, | |||
argTypes, methFuncIdent, hasReturnValue) = | |||
{.emit: """/*TYPESECTION*/ | |||
N_NOINLINE(void, setStackBottom)(void* thestackbottom); | |||
N_NOINLINE(void, nimGC_setStackBottom)(void* thestackbottom); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setStackBottom
should still be used for versions prior to 0.19.0 here and in godotnim.nim.
Also, please bump the minor version. |
@bluenote10 any update on this, should I make a PR on your branch with the requested changes? |
@zetashift That would be great, I wanted to work on it, but I simply couldn't find the time... |
So the |
Currently getting a big error and I have no idea how to solve this, this is using bluenote's branch opening the godot-nim-stub:
|
With the latest Nim devel as of yesterday, I can get your changes to compile and run the example code (with some modifications). I need to change import godot
import scene_tree, resource_loader, packed_scene, panel, global_constants,
input_event_mouse_button
gdobj MainPanel of Panel:
method ready*() =
setProcessInput(true)
method input*(event: InputEvent) =
if event of InputEventMouseButton:
let ev = event as InputEventMouseButton
if ev.buttonIndex == BUTTON_LEFT:
getTree().setInputAsHandled()
let scene = load("res://scene.tscn") as PackedScene
# This line gives me a "stack smashing error" at runtime. You can also add
# a flag to the nakefile.nim's compilation line to disable bounds checking,
# but we should probably find out why this happens. Maybe it only happens for me?
# getTree().root.addChild(scene.instance())
let root = getTree().root
let panel = root.getChild(0)
root.addChildBelowNode(panel, scene.instance())
queueFree() I should note that I'm also using a Godot binary compiled from latest master on the godot repo. |
Using Godot 3.1, latest devel, compiles indeed fine(also added this on my PR to @bluenote10 branch). However the runtime error, even with your changes still happens:
Sorry for the lengthly log this is what the godot console gives me. |
Using Godot 3.1, latest devel, I had the same issue as @zetashift when attempting to switch scenes. I accidentally fixed it by adding: to the resource_loader.nim proc load(path: string; typeHint: string = ""; noCache: bool = false): Resource =
if isNil(singletonResourceLoader):
singletonResourceLoader = getSingleton[ResourceLoader]()
let self = singletonResourceLoader
if isNil(resourceLoaderLoadMethodBind):
resourceLoaderLoadMethodBind = getMethod(cstring"_ResourceLoader",
cstring"load")
var
argsStatic: array[2, pointer]
argsToPassToGodot = cast[ptr array[MAX_ARG_COUNT, pointer]](argsStatic.addr)
var argToPassToGodot0 = toGodotString(path)
argsToPassToGodot[][0] = unsafeAddr(argToPassToGodot0)
var argToPassToGodot1 = toGodotString(typeHint)
argsToPassToGodot[][1] = unsafeAddr(argToPassToGodot1)
argsToPassToGodot[][2] = unsafeAddr(noCache)
################ Fixed Issue ???
echo(repr self)
echo(repr argToPassToGodot1)
#################
var ptrCallRet: pointer
var ptrCallVal: ptr GodotObject
ptrCallRet = addr(ptrCallVal)
resourceLoaderLoadMethodBind.ptrCall(self.godotObject, argsToPassToGodot,
ptrCallRet)
deinit(argToPassToGodot0)
deinit(argToPassToGodot1)
result = asNimGodotObject[type(result)](ptrCallVal, false, true)
var resourceLoaderGetRecognizedExtensionsForTypeMethodBind {.threadvar.}: ptr GodotMethodBind
method ready*() =
setProcess(true)
btn = getNode("ActiveScene/btn").as(Button)
discard btn.connect("pressed", self, "on_btn_pressed", newArray())
All in all, @endragor, thank you greatly for these amazing bindings! : ) Edit: There seems to have been an off-by-one error in the bindings generator for the var
argsStatic: array[3, pointer] or, in var
argsStatic: array[5, pointer] fixes the scene loading bug @zetashift had, as well as that signal connecting issue I had. |
godot/godotapigen.nim
Outdated
@@ -450,13 +456,13 @@ proc doGenerateMethod(tree: PNode, methodBindRegistry: var HashSet[string], | |||
ident("array"), ident("MAX_ARG_COUNT"), | |||
if isVarargs: newNode(nkPtrTy).addChain(ident("GodotVariant")) | |||
else: ident("pointer")))) | |||
staticArgsLen = if varargsName.isNil: meth.args.len | |||
staticArgsLen = if varargsName.isSome: meth.args.len |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo? Should it be if varargsName.isNone
?
it's working now too, not sure why the bindings generate this :(. |
Small changes to add 0.18.0 compatibility
@endragor do you have any idea on the off-by-one error? |
@zetashift Have you tried a clean rebuild? |
@adrianperreault yep works fine for me too now with a fresh install. |
Currently the only annoying thing is the Edit: https://github.com/pragmagic/godot-nim/blob/master/godot/nim/godotmacros.nim#L487 seems I should look here? |
Latest Nim support is added in v0.7.21. |
Attempt to fix #41
I addressed the most obvious issues regarding Nim 0.19.
Unfortunately, I still can't compile the nim-godot-stub, because there is now a very strange macro error:
This is referring to the code line:
As far as I can see from stdout debugging none of the arguments is an
int literal (0)
. Is this a Nim bug?