Implement LibGodot, a currently-minimal API to embed Godot as a library.#110441
Implement LibGodot, a currently-minimal API to embed Godot as a library.#110441zorbathut wants to merge 8 commits intogodotengine:masterfrom
Conversation
Co-authored-by: Faolan <46481567+Faolan-Rad@users.noreply.github.com> Co-authored-by: Gergely Kis <gergely.kis@migeran.com> Co-authored-by: "K. S. Ernest (iFire) Lee" <ernest.lee@chibifire.com>
zorbathut
force-pushed
the
pr/libgodot
branch
from
8d24949 to
caf6f81
Compare
dsnopek
left a comment
dsnopek
left a comment
There was a problem hiding this comment.
Thanks!
I haven't had a chance to fully review this yet, but I've copied over some of my unaddressed review from #90510. It appears like one of the points was somewhat handled, and one doesn't apply since this PR doesn't have any of platform-specific rendering "surface" stuff from that PR
I'll try to do some more in-depth review when I have a chance!
| #include "gdextension.h" | ||
|
|
||
| Error GDExtensionFunctionLoader::open_library(const String &p_path) { | ||
| ERR_FAIL_COND_V_MSG(!p_path.begins_with("libgodot://"), ERR_FILE_NOT_FOUND, "Function based GDExtensions should have a path starting with libgodot://"); |
There was a problem hiding this comment.
I like "libgodot://" better than the "res://__LibGodot" that I commented on here on the predecessor PR.
However, we should make sure that everyone is OK with using this invalid path prefix!
There was a problem hiding this comment.
Happy to change it if someone would prefer something else :)
There was a problem hiding this comment.
res://__LibGodot doesn't follow the convention of the other invalid path we have .godot. Maybe libgodot.
That said embedded:// sounds cool.
Open to discussion.
There was a problem hiding this comment.
I admit I like embedded:// also, it feels more general than libgodot://.
There was a problem hiding this comment.
I don't know that we want something that's more general? That increases the risk of it colliding with some future unrelated feature, whereas "libgodot" is very specific and can only refer to exactly one thing.
That said, I don't know if we ever decided to officially name this "libgodot" - that was the topic of my review on the other PR in Sept 2024. We should decide what we want to officially name this, and then be consistent everywhere
There was a problem hiding this comment.
I don't know that we want something that's more general?
So, the "more general" I'm referring to is the general idea of "an extension that doesn't exist on the filesystem". I don't specifically know how that will turn out to be useful, but if it does, it would be nice to not have it tagged libgodot (or whatever we end up naming this thing).
I think this was originally @Faolan-Rad 's, I don't know if it was designed specifically for this or if there are other uses in mind.
We should decide what we want to officially name this, and then be consistent everywhere
Agreed! It's kind of awkward right now because the C++ GDExtension bindings also use the name libgodot.
But this is kind of out of my pay grade, so to speak; who should be handling this?
There was a problem hiding this comment.
Two thoughts:
-
I do like the idea of "embedded godot"
-
but also, in retrospect, the obvious prefix for a Function Extension is probably something like
functionextension://
|
FYI: I just pushed our update: #90510 (comment) |
dsnopek
left a comment
dsnopek
left a comment
There was a problem hiding this comment.
I did a deeper review of the C++ code, and at a high level, this looks like a really good base! Given that it's largely a subset of #90510, we can continue to build on it, and bring more and more of those features in.
My review is mostly nitpicky stuff, with maybe 1 or 2 important things to figure out. I didn't review any of the build system code, because that's not my area
core/extension/libgodot.h
Outdated
| * | ||
| * @return A pointer to created \ref GodotInstance GDExtension object or nullptr if there was an error. | ||
| */ | ||
| LIBGODOT_API GDExtensionObjectPtr libgodot_create_godot_instance(int p_argc, char *p_argv[], GDExtensionInitializationFunction p_init_func, LibGodotExtensionParameter *p_params); |
There was a problem hiding this comment.
I'm not sure about the LibGodotExtensionParameter *p_params argument.
I'm guessing this is here to allow for the extra parameters that #90510 has?
I'll need to spend some more time looking into what the latest version of that PR is doing with all those new parameters, but I'm a little skeptical. The whole idea with LibGodot was that the API will be GDExtension. So, ideally, we'd only have the minimum API to get GDExtension initialized, and then everything else should be done through the GDExtension interface.
I'm not crazy about the idea of the unique API surface of LibGodot getting bigger.
There was a problem hiding this comment.
The fundamental problem here is that "initialize GDExtension" requires loading a lot of the engine. Anything that needs to be introduced to the engine before GDExtension is initialized needs to live here. I'm not completely sure if this includes the render-target replacement code, nor am I completely sure if this includes the Mac input/output callbacks, but it definitely includes loggers at the very least, and it won't surprise me at all if eventually there are other things needed for some use cases.
My hope is that "no parameters" always works fine for "embedded Godot with no render target redirection and no fancy input passthrough". But we'll always need a parameter for logging hooks, and we might need parameters for other things, and while I agree this is a little ugly, it's a lot less ugly than ending up with libgodot_create_godot_instance2 and libgodot_create_godot_instance3 as we keep adding more parameters.
I will note that nothing I'm doing requires more parameters, so if you insist I'm going to shrug and say okiedokie, but I think @kisg might end up objecting to the result :) And they know the rendering stuff better than I do anyway; might be worth asking them if their requirements are needed at this point in the init or not.
There was a problem hiding this comment.
I think doing libgodot_create_godot_instance2 and libgodot_create_godot_instance3 is less ugly as the api surface area isn't infinite. The area only grows when we need it.
There was a problem hiding this comment.
The other problem here is that a bunch of this functionality is OS-specific, so we either end up with functions with weird callbacks that only exist on some platforms, or libgodot_create_godot_instance_osx, or those literally not existing on some platforms. But I'll defer to whatever people want on this one, honestly.
There was a problem hiding this comment.
Anything that needs to be introduced to the engine before GDExtension is initialized needs to live here.
Yeah, I'll need to read through the latest on #90510 and see what they are all used for. The last time I reviewed it, I don't think it had any of those additional arguments!
while I agree this is a little ugly, it's a lot less ugly than ending up with
libgodot_create_godot_instance2andlibgodot_create_godot_instance3as we keep adding more parameters.
Similar to @fire, I'd probably rather have libgodot_create_godot_instance2, libgodot_create_godot_instance3, etc as well. And rather than taking so many arguments, I'd prefer having an info struct with all the additional stuff, similar to how we do it in the GDExtension interface
But we don't need to make a libgodot_create_godot_instance2 within the same dev cycle. Everything we can get in during Godot 4.6's development can change the original libgodot_create_godot_instance :-)
There was a problem hiding this comment.
Fair :) Want me to just yank that parameter out for now, then?
(In theory we could also take a page from the Windows API and have a LibGodotCreateInstance[|Ex|Ex2|Ex3] structure that's versioned through a dwSize parameter; that's a weird diversion from how Godot APIs traditionally work, but this entire setup is always going to be a bit strange simply because it can't live in GDExtension.)
There was a problem hiding this comment.
Want me to just yank that parameter out for now, then?
Sure, we can always add parameter(s) in future PRs that need them
In theory we could also take a page from the Windows API and have a LibGodotCreateInstance[|Ex|Ex2|Ex3] structure that's versioned through a dwSize parameter
I feel like we should mimick the scheme used in the GDExtension interface. It would be odd for LibGodot's functions do this differently than GDExtension.
|
Hi, I reviewed this PR, and to me this looks like our (Migeran's) 4.4 version of libgodot, which we did not submit for review, because it was too late in the review cycle and were working on other additions. You can find that version here: https://github.com/migeran/godot/tree/libgodot_44_stable This PR only:
We don't agree with how this is being handled, and this is not how contributions were handled in the past in this community. We were working on an update for after the Godot 4.5 release, the only reason for about a ~1 month delay was personal on our side (we targeted a mid-August release). To be clear: We are happy to break up our PR into smaller pieces. Even when we first submitted our PR we offered that, but back then we were told it was not necessary. Of course, we are still happy to do it, and discuss and implement any other changes that the maintainers see necessary. However, we feel that the future direction of this code should be discussed on the original PR: #90510 I also think that we should set up a time next week on the Core meeting to discuss the strategy of merging LibGodot into Godot master. Kind regards, |
|
For what it's worth, my main goal of this is just to get something mergeable in. People have been working on this for two and a half years now, but I'm using it in production and I don't want to keep messing around with custom patches, because I am extremely lazy. You're correct that I removed a lot of stuff; the plan is to get it in piecemeal afterwards. Note that this doesn't include code I need! I'll need another PR after this, even if my sole goal is "my needs are met", which it isn't. Git makes it kind of a pain to change the author of a change, but if that's the biggest issue, I'll happily jump through the hoops to make it "your" change; you'll also note that I asked for feedback on authors to include in this PR as well as in the notification I posted in your PR. I (mostly :V) do not care about credit here, I care about getting the code in. All that said, I do want to say that I'm worried that this will end up with another multi-month delay and get bumped to 4.7 or later. For reference, I offered to help out back in May and ended up sending you a fix PR for various issues and regressions that was never touched or acknowledged. A month later I provided the code needed for C# support which has also not been included. Instead we've got a 7000-line diff that fails 19/20 tests; meanwhile I've already gotten a small code change merged specifically to fix a problem I ran into with updating libgodot in May. Your code is valuable and I am not trying to diminish your contributions but there's logistical work needed for practical upstream inclusion, and that's what I'm trying to handle. I am volunteering to do the unexciting grunt work to merge one reasonable chunk at a time. Please let me! Then you can focus on the parts you care about the most, I can focus on the parts I care about the most, and we can all end up better off. And if that means you want to provide me with a paragraph or two of credit to be included in the eventual commit message, please provide me with that paragraph and I'll get it in, and will make sure it remains in any eventual squash. |
|
Dear Ben, I saw your message last week, and I also clearly (or at least I hoped) answered then, that we are finishing up the update to our PR as well: #90510 (comment) Obviously, such a big change needs to be split up, which we always intended and offered to do even during the earliest discussions of our PR, and I also clearly stated in our latest update: #90510 (comment) To be clear, my colleagues and I have been developing our Migeran LibGodot version for about 3 years now (first public mention of it is from November 2022), independently from other attempts. I believe that you only wanted to help with your PR, but the way it was created was unfortunate. As an example, how a similar situation is usually handled in Godot, here is our work on the Shader Baker feature, which was included in 4.5: #102552 Here, we made a significant contribution: implemented the Metal support for the Shader Baker, which then Stuart Carnie refactored, and then we worked together to iron out a few more issues. But we did not open a new PR and downgrade Dario (the original developer) to a Co-Author. At the same time, we were properly attributed together with Stuart on the final commit by Dario as Co-Authors. We (Migeran) will split up our current PR and open new PRs next week, before Wednesday (the next scheduled Core meeting), not just for the core part, but also for the other, more advanced features that build on the core. We will list you on the core PR as Co-Author for your contribution to the splitting of our Migeran LibGodot PR. This should provide a good basis for future collaboration, because I know a lot of people would love to see .NET support, and it would be great to get your work included as a follow-up PR on top of our PRs. I hope that this will settle the issue. Kind regards, |
|
#110863 checked in, this is no longer needed! |
5 participants
What is it?
LibGodot allows you to embed Godot into a program as a library and control Godot directly, instead of expecting Godot to be the coordinator and overseer.
Why?
Here's a not-comprehensive list of reasons you might want to do this.
As an example, the author has been using an earlier version of this for native C# tests. The C# test APIs can't run the Godot binary directly, they must be run within a native .NET environment, but by running Godot as a library within that .NET environment, C# testing is made far easier.
Other people have integrated Godot into Blender and Unreal Engine for their own reasons.
Features
This is a minimum-viable-product of the basic concepts of LibGodot. It includes:
It does not yet include:
I plan to include several of these as followup PRs in the near future, but this is complicated enough as-is; in addition, getting the foundation in place allows others to contribute features, especially for things I can't do on my own like Mac support.
How do I try it?
https://github.com/zorbathut/libgodot_example
This contains a miniscule Godot project; one scene with a Label, and a C++ program that starts Godot as a shared library. The C++ framework finds and modifies the label at runtime for ten seconds, then shuts down on its own. Porting the C++ program to other languages should be reasonably easy (as long as they support GDExtension.)
Details . . .
. . . are long and complicated. I can talk at great length about some of the decisions I made here and why I diverged from kisg's original patch. I could probably write an entire small novel that nobody would read. Ask questions!
I will note that the name "libgodot" isn't great because some people are already using this for GDExtension APIs. I don't have a better name though. Happy to change it.
Credits
This has been built on the work of multiple people, including:
@Faolan-Rad - #72883
@kisg - #90510
@fire - considerable support on Discord
There's probably people I'm missing, because this has been bouncing around for two years now. Let me know if someone should be added (or wants to be removed).
This supercedes #72883 and #90510, and closes godotengine/godot-proposals#6267.