SimulaVR / godot-haskell

Haskell bindings for GdNative
BSD 3-Clause "New" or "Revised" License
171 stars 17 forks source link

Many improvements and official demo game #23

Closed abarbu closed 3 years ago

abarbu commented 5 years ago

This is a large patch. Among the many things it does:

Probably the most controversial change is the rework of the method and base class system. The method system was entirely removed. This was for a few reasons: the code it leads to isn't the kind of code we write in Haskell, it's way too dynamic, hard to track down, Haddock makes no sense with it, and the error messages are a complete & painful mess. Including what you need, when you need it, is painless and the Haskell way for every other package.

The base class system was reworked and simplified. Instead of relying on overlapping instances, we generate the instances that are needed. Yeah, it's a bit more annoying, but it works far better, and the error messages make a lot more sense.

There's more in here that makes games like the one in the official docs viable. Also, I found the project really difficult to get going with, despite how easy it turned out to be in the end, because of the lack of docs in the readme. There's a lot more info now.

Upgrades to this version from the original are not a big deal and mostly involve removing "Godot" from a lot of places, adding some imports, the deriveBase for each class, and making sure that defaultExports is called.

lboklin commented 5 years ago

Edit: On reviewing some parts it became clear that the changes are so fundamental my comments below are probably not applicable. I'll have to spend more time reviewing this.

Rename to remove redundant "Godot" everywhere

When it comes to the types such as GodotString, GodotVector2, etc, the reason they're prefixed with Godot is to indicate that they are low-level type which are unusable in Haskell except for the Godot methods. There is an ongoing effort (by me) to expose all methods with high-level type signatures (see https://github.com/lboklin/godot-haskell/tree/high-level-typesigs), which also aliases the Haskell-native types to more obviously correspond to the Godot-native types.

defaultExports to register internal classes

Can you elaborate on this a bit? Have you seen the registerClass in src/Godot/Nativescript.hs and its usage of it in template/demo/src/Lib.hs?

KaneTW commented 5 years ago

Thanks a lot for the PR.

Where did you run into bad type errors?

Also, huge HUGE kudos for getting docs in. I've been putting them off because I was basically in a permanent time crunch and I'm really glad you implemented them.

abarbu commented 5 years ago

Hi folks,

@KaneTW The error messages when passing in the wrong arguments to methods were confusing to me and were often of the "There's no instance for this"-type. I constantly had to look up types, instead of just letting the type system guide me to what they should be. When types were being inferred but I also got the number of arguments wrong to a method, it was rough to see what's wrong. The error messages for upcasting (safeCast) can also be rough. The messages when the two interact are sketchy.

Basically, I'm used to writing Haskell code the lazy way. I put in some type holes, GHC/intero tell me what to fill in, and we just go from there. I rarely look at docs or think much about types. But that just didn't work.

@lboklin The original demo was trivial and it was hard to see where to go from there. I didn't for example see how to create new classes (not instances) or how to use await. It was also hard to see the big picture from it. I didn't feel I learned enough to follow the Godot docs.

When it comes to the types such as GodotString, GodotVector2, etc, the reason they're prefixed with Godot is to indicate that they are low-level type which are unusable in Haskell except for the Godot methods.

I feel like we always have this problem in Haskell and there's a standard solution to it that works. Every datatype has its own methods and we're all used to just including the modules we need, qualifying anything that conflicts, and then calling the right functions. It's no different here. In any case, I had to keep the prefix for strings and for variant.

Having the wrapper functions is good, but enabling folks to easily use the low-level bindings still matters. The cost of constantly marshaling data will be high. Personally, I don't want to use Haskell datatypes to talk to Godot because in the long term GC pressure alone (never mind runtime) will make it unviable and moving back to the low-level bindings will a total pain.

With slight improvements over what you've already built, it's easy to just stick to Godot datatypes and forget about marshalling to Haskell except in a few cases.

defaultExports to register internal classes

Can you elaborate on this a bit? Have you seen the registerClass in src/Godot/Nativescript.hs and its usage of it in template/demo/src/Lib.hs?

I have. The problem is, you need to register at least one class in the bindings. Or at least, I do not see any way around this that isn't much more annoying and much slower. No built in class lets you wrap a Haskell object (well a stable pointer to one) and pass it to Godot. This is critical for efficiently implementing await.

This change puts a callback in the FLib so that the library also gets a chance to run its own exports function before the user. In this case it registers that class.

It also happens to be important to record the descriptor you get passed to you when the bindings are initialized. It's a string that points to the resource that represents your bindings. Maybe there's some other way to get a handle to it, but I didn't see one and other bindings record it too.

In the future, I imagine higher-level bindings will need to more in that function so it's good to have anyway.

abarbu commented 5 years ago

Also, huge HUGE kudos for getting docs in. I've been putting them off because I was basically in a permanent time crunch and I'm really glad you implemented them.

Thanks! You guys are definitely more patient than I am! I just couldn't use the bindings without docs. Particularly since this was my first exposure to Godot.

abarbu commented 5 years ago

I was asked about memory safety, leaks in particular. Leaks aren't the main issue right now, memory corruption is.

Haskell cannot hold a reference to a Godot object without some other Godot-managed object keeping the reference alive. Such an object will be freed at some point without that reference. To avoid this, I earlier intentionally created a memory leak and added a comment in Nativescript.hs about this.

Looking into it, the leak can easily be resolved. I just did so. But a longer-term solution is needed to let Haskell hold safe references to Godot. Something like an MVar that manages reference counts.

lboklin commented 5 years ago

Sorry for the long delay. I welcome the explicit imports, improved type feedback and removal of the Godot- prefix.

While dreading the idea of rebasing my own fork on top of this I have no particular objections once all issues have been cleared up (I saw at least one issue that needs fixing). After that I'll let @KaneTW make the final call.

Also, thanks a lot for adding docs and a proper demo!

lboklin commented 5 years ago

The diffs in my commits are a bit blown up in parts because of automatic formatting with brittany which I recommend as a general style guideline.

lboklin commented 5 years ago

One thing I'm not too keen on is being required to use Template Haskell as a user (for deriveBase). I would greatly prefer to keep everything very simple and as community-standard Haskell as possible. If this is to be sacrificed we might as well go all the way and create a DSL that feels much closer to GDScript.

abarbu commented 4 years ago

Hi folks,

Just checking if this is on the path to getting merged one day. 4.0 is coming up at some stage and I was thinking of playing around with Godot again. Seems like other people are too: https://www.reddit.com/r/haskell/comments/gtthp8/yourfirstgame_with_haskell_godot_and_godothaskell/

KaneTW commented 3 years ago

Ok, finally got around to reviewing it more fully. Looks good. Please make it mergeable onto master and I'll do it.

I think a more elegant solution to the memory corruption+leaks is to reference Godot objects manually, and then either statically (ResourceT/linear types/etc) or dynamically (ForeignPtr) unreference them.

Our workaround for some of our custom gdwlroots types having that issue was to use Ref<> in gdwlroots for custom object parameters and to add a G.reference when it was passed as an argument. That ended up being fairly leak free and cleared up the corruption issues.

For Haskell objects passed into Godot, you need to free them manually anyway, so there shouldn't be too much of an issue there.

KaneTW commented 3 years ago

merged using david eichmann's rebase