-
-
Notifications
You must be signed in to change notification settings - Fork 23.8k
LibGodot: Core - Build Godot Engine as a Library #110863
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
Conversation
f8c7523 to
b2a570a
Compare
|
Pretty excited to see this - how will this be handled in conjunction with #110441? |
It's basically the same thing as #110441, plus Mac support, plus a few minor features that I personally think shouldn't be included but overall don't care. I am happy for the core team to pick whichever they prefer as long as one of them makes it into 4.6 (ideally sooner rather than later so I can also get proper C#-within-C# support on top of it :V) |
|
To @fire's point in this PR alongside #110441 - I see a lot of issues with the name What about something like |
|
We should discuss if we want to call this "libgodot", however:
I'm not crazy about "embedded". That name is generic enough that it could conflict with some completely unrelated thing we want to call "embedded" in the future. What I like about "libgodot" is that it is very specific, and sounds kind of like a product/feature name, so it seems unlikely to conflict with something else in the future. It may be the best option we've come up with so far. |
Yeah that makes sense, so long as there's no naming conflicts which it sounds like there aren't. |
b2a570a to
d01d0b6
Compare
|
As discussed in the Core meeting, I just updated the PR: * Removed restart support - and the associated StringName changes
In the destroy function I added this code instead of introducing a new static variable: My rationale is that this should be a temporary state, until the reinitialization PR is merged. |
|
With regards to Android: our Android version (still working on updating from 4.4 to 4.5 and master) actually builds as "libgodot_android.so" and it is very close to the non-LibGodot version, so it might be possible that on Android we just use the LibGodot build, if we can make it acceptable to @dsnopek and @m4gr3d. |
|
Anything I can poke to keep this moving along? Happy to help with it if possible. |
|
@zorbathut If I understand it correctly we are waiting on @Repiteo 's review on the build system part, and then we should be good to go. @dsnopek Do we need anything else to move forward? |
|
@zorbathut In the meantime you can prepare your .NET PR on top of this PR. That is what I am doing with our start / stop and UI embedding / platform support PRs. |
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.
Excellent job so far! This is obviously something that has been in the works for a looong time.
The buildsystem side of things is mostly accounted for, but there's a few things that need tweaking. Most notably: we shouldn't be hardcoding the platforms which support compiling as a library. This information can be conveyed through the "supports" flag that platforms can pass. As this is a more involved change, I've opted to put this implementation specifically in a dedicated diff:
diff
SConstruct | 14 +++++++-------
platform/linuxbsd/detect.py | 2 +-
platform/macos/detect.py | 2 +-
platform/windows/detect.py | 2 +-
4 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/SConstruct b/SConstruct
index 4ccf237cc8..326b4ab88a 100644
--- a/SConstruct
+++ b/SConstruct
@@ -356,13 +356,6 @@ if not env["platform"]:
if env["platform"]:
print(f"Automatically detected platform: {env['platform']}")
-# Library Support
-if env["library_type"] != "executable":
- if env["platform"] not in ["linuxbsd", "macos", "windows"]:
- print_error(f"Library builds not yet supported for {env['platform']}")
- Exit(255)
- env.Append(CPPDEFINES=["LIBGODOT_ENABLED"])
-
# Deprecated aliases kept for compatibility.
if env["platform"] in compatibility_platform_aliases:
alias = env["platform"]
@@ -571,6 +564,13 @@ if not env["deprecated"]:
if env["precision"] == "double":
env.Append(CPPDEFINES=["REAL_T_IS_DOUBLE"])
+# Library Support
+if env["library_type"] != "executable":
+ if "library" not in env.get("supported", []):
+ print_error(f"Library builds unsupported for {env['platform']}")
+ Exit(255)
+ env.Append(CPPDEFINES=["LIBGODOT_ENABLED"])
+
# Default num_jobs to local cpu count if not user specified.
# SCons has a peculiarity where user-specified options won't be overridden
# by SetOption, so we can rely on this to know if we should use our default.
diff --git a/platform/linuxbsd/detect.py b/platform/linuxbsd/detect.py
index 5f929c5925..03fdddd5de 100644
--- a/platform/linuxbsd/detect.py
+++ b/platform/linuxbsd/detect.py
@@ -67,7 +67,7 @@ def get_doc_path():
def get_flags():
return {
"arch": detect_arch(),
- "supported": ["mono"],
+ "supported": ["library", "mono"],
}
diff --git a/platform/macos/detect.py b/platform/macos/detect.py
index 9873233baa..c5846c34a1 100644
--- a/platform/macos/detect.py
+++ b/platform/macos/detect.py
@@ -61,7 +61,7 @@ def get_flags():
"arch": detect_arch(),
"use_volk": False,
"metal": True,
- "supported": ["metal", "mono"],
+ "supported": ["library", "metal", "mono"],
}
diff --git a/platform/windows/detect.py b/platform/windows/detect.py
index d5a8eb5f85..973be3e6b2 100644
--- a/platform/windows/detect.py
+++ b/platform/windows/detect.py
@@ -233,7 +233,7 @@ def get_flags():
return {
"arch": arch,
- "supported": ["d3d12", "dcomp", "mono", "xaudio2"],
+ "supported": ["d3d12", "dcomp", "library", "mono", "xaudio2"],
}
Everything else is covered in the following suggestions
Repiteo
left a comment
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.
That applies most of the suggested changes, but you still need to apply the above diff. If you missed it, the > diff part can be expanded by clicking
771f375 to
47233df
Compare
|
@Repiteo @AThousandShips @dsnopek All review comments should be addressed now. |
dsnopek
left a comment
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.
Thanks! This looks great to me from the GDExtension side :-)
Repiteo
left a comment
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.
Looks good from the SCons side of things!
|
Gonna make this a priority topic for this week's core meeting, just to make sure we aren't overlooking anything. I know it's been a long wait and hate to prolong it even further, but I see light at the end of the tunnel! 🚂 |
* Add a new GodotInstance GDCLASS that provides startup and iteration commands to control a Godot instance. * Adds a libgodot_create_godot_instance entry point that creates a new Godot instance and returns a GodotInstance object. * Adds a libgodot_destroy_godot_instance entry point that destroys the Godot instance. Sample Apps: https://github.com/migeran/libgodot_project Developed by [Migeran](https://migeran.com) Sponsors & Acknowledgements: * Initial development sponsored by [Smirk Software](https://www.smirk.gg/) * Rebasing to Godot 4.3 and further development sponsored by [Xibbon Inc.](https://xibbon.com) * The GDExtension registration of the host process & build system changes were based on @Faolan-Rad's LibGodot PR: godotengine#72883 * Thanks to Ben Rog-Wilhelm (Zorbathut) for creating a smaller, minimal version for easier review. * Thanks to Ernest Lee (iFire) for his support Co-Authored-By: Gabor Koncz <[email protected]> Co-Authored-By: Ben Rog-Wilhelm <[email protected]>
7eceddc to
6c44c80
Compare
|
Thanks! Excellent work to everyone who helped get this together & set the groundwork for this heavily-anticipated feature! |
| os = new OS_MacOS_NSApp(p_argv[0], remaining_args, remaining_args > 0 ? &p_argv[1] : nullptr); | ||
|
|
||
| @autoreleasepool { | ||
| Error err = Main::setup(p_argv[0], remaining_args, remaining_args > 0 ? &p_argv[1] : nullptr, 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.
Note: this is not how macOS app lifecycle works.
OS_MacOS_NSApp is calling it automatically (from applicationDidFinishLaunching), so it will be called twice and likely won't work.
Normal init is:
- create
OS. - call
OS::run()(which will NOT return until app is finished), it will callMain::setup,Main::startand adds main loop observer which runsMain::iteration().
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.
LibGodot introduces a new way of instantiating and controlling Godot, which is the whole point of it, so calling OS::run() would make no sense for us. As stated in the PR (and in the original PR) here are a few sample apps: https://github.com/migeran/libgodot_project/tree/libgodot_migeran_core.
You are right, that probably we should have used our own OS_MacOS subclass (or the embedded one), but since we never call OS::run(), the NSApplication won't run, so the applicationDidFinishLaunching() call will never be made, so it is not an immediate problem. We will make a PR for this, to make it cleaner.
Mickeon
left a comment
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.
There was a pending review I forgot to submit.
| instance = memnew(GodotInstance); | ||
| if (!instance->initialize(p_init_func)) { | ||
| memdelete(instance); | ||
| // Note: When Godot Engine supports reinitialization, clear the instance pointer here. |
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've never seen a Note in the source code comments, personally? Just a nitpick. The note implies Godot will support it someday, so it may be more appropriate a TODO here
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.
It already works, and it is used in production by many users. Check the original PR for the code: #90510
As requested, the original PR is being broken up into smaller pieces, this functionality is one of the next PRs we are working on to submit for reviews.
|
Will Android and iOS be supported in the future? |
Both iOS and Android already work on the 4.5 branch, we just need to split all the code up into multiple PRs to get it reviewed and merged: https://github.com/migeran/libgodot/tree/libgodot_migeran_45 Both iOS and Android support is used in production already: Xogot is based on LibGodot iOS (and also sponsored by): https://xogot.com/ |
LibGodot: Core - Build Godot Engine as a Library
This PR is Part 1 of the refactoring of the original Migeran LibGodot PR: #90510
It includes the core parts required for building and loading Godot Engine as a library.
Includes:
Sample Apps: https://github.com/migeran/libgodot_project
Developed by Migeran
Sponsors & Acknowledgements:
on @Faolan-Rad's LibGodot PR: LibGodot with GDExtension #72883