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

[WIP] Generate direct call bindings #34

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

worrel
Copy link
Contributor

@worrel worrel commented Feb 21, 2018

@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
Copy link
Contributor Author

worrel commented Feb 21, 2018

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

@ShadowApex
Copy link
Owner

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}
}

@ghost
Copy link

ghost commented Feb 22, 2018

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

@worrel
Copy link
Contributor Author

worrel commented Feb 22, 2018

@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.

@1l0
Copy link
Contributor

1l0 commented Feb 22, 2018

HYPE. You guys are awesome.

@worrel
Copy link
Contributor Author

worrel commented Feb 22, 2018

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
Copy link
Contributor Author

worrel commented Feb 22, 2018

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
Copy link
Contributor Author

worrel commented Feb 23, 2018

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/

@ShadowApex
Copy link
Owner

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.

@worrel
Copy link
Contributor Author

worrel commented Feb 24, 2018

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

@SolarGranulation
Copy link

Any movement on this?

@ghost
Copy link

ghost commented Jan 17, 2019

@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..

@worrel
Copy link
Contributor Author

worrel commented Jan 18, 2019

@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.

@pcting
Copy link

pcting commented Feb 12, 2020

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

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.

None yet

5 participants