godot-go icon indicating copy to clipboard operation
godot-go copied to clipboard

[WIP] Generate direct call bindings

Open worrel opened this issue 6 years ago • 14 comments

@ShadowApex not intended for merge, just an idea I'm playing around with - pre-generating all calls to GDNative C API methods to avoid using reflection at runtime. It's mostly a straight rip-off of how the CPP bindings are done:

classes.go

func (o *AStar) AddPoint(id int64, position *Vector3, weightScale float64) {
        GodotCall_void_int_Vector3_float(o, "add_point", id, position, weightScale)
}

godotcalls.go

func GodotCall_void_int_Vector3_float(o Class, methodName string, arg0 int64, arg1 *Vector3, arg2 float64) {

	methodBind := getGodotMethod(o.baseClass(), methodName)
	cArgsArray := C.build_array(C.int(3))
	cArg0 := arg0 // TODO convert arg based on type
	C.add_element(cArgsArray, cArg0, C.int(0))
	cArg1 := arg1 // TODO convert arg based on type
	C.add_element(cArgsArray, cArg1, C.int(1))
	cArg2 := arg2 // TODO convert arg based on type
	C.add_element(cArgsArray, cArg2, C.int(2))

	objectOwner := unsafe.Pointer(o.getOwner())

	C.godot_method_bind_ptrcall(
		methodBind,
		objectOwner,
		cArgsArray, // void**
		nil,        // void*
	)
}

Still TBD is the Go<->Godot type conversions on either side of the C calls. I expect it will be similar to the existing conversion code but executed at code-gen time.

Let me know if you'd entertain using this approach - I suspect it's slightly faster but I haven't benchmarked. Right now it's actually 40K fewer generated LOC than the dynamic call approach, but that delta will shrink when I add the type conversion bits.

worrel avatar Feb 21 '18 23:02 worrel

And yes, it currently doesn't build, but it's close :-P

worrel avatar Feb 21 '18 23:02 worrel

Hi @worrel

I think this would be a great approach! The more we can avoid using reflection, the better.

The one thing I'm unsure about it is the names of the methods in godotcalls.go. They should probably be unexported, and use camelCase (e.g. godotCallVoidIntVector3Float) to keep with the Go naming conventions. There's a few methods that are in snake_case, but those names are required to be exported as C functions to Godot. In generate.go, there are some functions to convert snake case to camel case that could be used.

Related to this, I'm actually working on a significant refactor of the code in the rewrite branch, where I'm working on writing a thin wrapper around the GDNative methods and types. That thin wrapper will end up being a gdnative Go package that we can use to write the godot package in pure Go. Doing it that way will allow others to be more flexible in writing their own Go package to interface with Godot. As an example, right now we are generating the classes for all Godot class types (AStar, Node2D, etc.), but someone might not use all of those types, which just bloats the size of their library.

Right now I'm specifically writing a code generator/parser that will parse the Godot header files for all type definitions (e.g. godot_bool, godot_variant, etc.). From that, I hope to generate Go types for all of the discovered Godot types, along with conversion functions for every basic type so they can be passed to functions like C.godot_method_bind_ptrcall.

Here's an example of the base types I'm trying to generate:

type Variant struct {
    base *C.godot_variant
}

func (v *Variant) getBase() *C.godot_variant {
    return v.base
}

Once we have the code for generating all of the base types, we can write code to generate all of the methods defined in the Godot headers as Go wrappers. So, we might get something like this as an example wrapper:

func MethodBindGetMethod(classname, methodname *GodotString) *MethodBind {
    methodBind := godot_method_bind_get_method(classname.getBase(), methodname.getBase())
    return &MethodBind{base: methodBind}
}

ShadowApex avatar Feb 22 '18 06:02 ShadowApex

@ShadowApex generating the godot types would make things way easier for me.

ghost avatar Feb 22 '18 08:02 ghost

@ShadowApex yup, good point on the name exporting - I've changed it to camelCase. I found a godotCallVoidRidRect2Rect2RidVector2Vector2IntIntBoolColorRid and I never want to see anything but generated code using that :-D

The rewrite branch looks interesting. In particular, while the code in this PR now builds, it segfaults on the godot_method_bind_get_method calls. I'm finding navigating the cgo rock face tricky so anything that eases that would be great. Right now I'm debugging this by creating the equivalent GDNative calls in pure C & comparing to the generated bindings.

I'll ping when I have this branch successfully running the Pong example, or maybe the Dodge the Creeps game from the tutorial.

worrel avatar Feb 22 '18 14:02 worrel

HYPE. You guys are awesome.

1l0 avatar Feb 22 '18 15:02 1l0

it now runs the the Pong example w/o crashing & the tiniest beginnings of Dodge The Creeps (I haven't ported the whole thing yet). But basic stuff GetViewportRect() and GetNode() seem to work. Sure there are plenty of bugs lurking.

worrel avatar Feb 22 '18 16:02 worrel

Comparing to my C examples, I see I'll need to track when I allocate godot_string arguments:

cArg0 := unsafe.Pointer(stringAsGodotString(arg0))

so I can

C.godot_string_destroy(...)

before leaving the godotCallXXX to not leak memory. That's tomorrow's problem :-)

worrel avatar Feb 22 '18 16:02 worrel

I'm a bit stuck now. Many common calls work, but some segfault - notably AnimatedSprite.SetAnimation("someanim") or AnimatedSprite.Play(""). I've checked and SetAnimation also fails on master, though there the error is SIGBUS not SIGSEGV.

What's odd is that certain "similar" calls succeed. i.e. godot.Input.IsActionPressed("someaction") (func with String arg), or AnimatedSprite.Stop() (void func) do work. Also, Node.GetNode("nodename") works (func with object return type), since I'm able to call stop() on the AnimatedSprite that's returned. All this seems to rule out issues with arg passing via CGO or void/return value handling.

I have a separate C-only example and it's able to call "set_animation", "play" etc fine on an AnimatedSprite, so the issue appears to come from the Go side. I've tried replicating the C-only example in pure Go (i.e. without godot-go classes), but I can't get that to run (I've replicated cgateway.go etc from godot-go but Godot can't find my constructor func O_o).

Anyway, if someone has time to pull this branch, try a minimal example & report back, that would be cool. I've put my test code here: https://github.com/worrel/godot-go-test/

worrel avatar Feb 23 '18 02:02 worrel

Comparing to my C examples, I see I'll need to track when I allocate godot_string arguments before leaving the godotCallXXX to not leak memory.

I've found this has been tricky. Sometimes Godot will call godot_free or godot_string_destroy for you, in which case, if you free it again, it will crash.

I'm a bit stuck now. Many common calls work, but some segfault - notably AnimatedSprite.SetAnimation("someanim") or AnimatedSprite.Play(""). I've checked and SetAnimation also fails on master, though there the error is SIGBUS not SIGSEGV.

Yeah, I've also encountered those issues, and haven't had the opportunity to fully debug those. We may need to dig into exactly what is different about the calls that work and the calls that don't work. Looking at the differences between Node.GetNode("nodename") and AnimatedSprite.SetAnimation("someanim") might lead us to why it's segfaulting.

ShadowApex avatar Feb 23 '18 07:02 ShadowApex

I made some progress debugging the segfaults. I added a few variations of pure C calls for GetNode and SetAnimation (see classes.go on debug-failures branch) and then tested out combinations to find ones that worked (see my test Go shared lib).

I didn't spend too much time trying to narrow down exactly why certain combinations failed, but rather implemented a version of this branch using generated pure C calls (see godotcalls.go on direct-c-calls branch). I quite like the separation of responsibilities here:

  • classes.go provides the Go API structure and doesn't call any C.* methods
  • godotcalls.go has Go and C functions: -- godotCallsXXX Go funcs are responsible just for marshalling C<->Go types -- godot_call_XXX C functions are responsible for interacting with the GDNative API

One of the advantages of using pure C interaction for GDNative is it's easier to compare with existing GDNative examples or solicit help since there aren't any distracting Go/Cgo details. A second advantage appears to be library size: a build of my test code using master results in a 53mb .dylib on OSX, while the same code built against the direct-c-calls branch is only 11mb!

I'm still working out the kinks in the generated C code. It's already more functional than this branch, but I need to fix allocation of the core type structs. There's no GDNative example code on this, it appears you have to malloc/new any godot_vector2, godot_node_path etc structs that are return types prior to calling godot_method_bind_ptrcall. This is probably obvious to a seasoned C coder, which is something that I am not :-P

worrel avatar Feb 24 '18 16:02 worrel

Any movement on this?

SolarGranulation avatar Dec 30 '18 20:12 SolarGranulation

@ShadowApex @worrel

Wondering if there is any will to finish this or a technical dead end was hit on the memory leaks.

Have a look at this : https://github.com/xlab/c-for-go About 10 c libs have been wrapped with golang using code generation. Its battle tested from what i can see.

Can you reach out if you get stuck..

ghost avatar Jan 17 '19 16:01 ghost

@SolarGranulation @gedw99 Sorry, I haven't worked on this in months & I don't see time opening up soon with my workload & other projects. I haven't actually tried out @ShadowApex's rewrite from back in March yet so I can't comment on where this project is at currently. The xlab/c-for-go thing looks interesting though. If it ends up snowing & sleeting all this long weekend I might give it a go.

worrel avatar Jan 18 '19 12:01 worrel

i'm going to attempt a rewrite on top of c-for-go. will post updates once i reach a milestone

pcting avatar Feb 12 '20 18:02 pcting