-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Migeran LibGodot Feature #90510
base: master
Are you sure you want to change the base?
Migeran LibGodot Feature #90510
Conversation
You can fix some of the github ci actions tests with Here are the rest of the errors. https://github.com/V-Sekai/godot/actions/runs/8642913827 |
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! I'm so excited to finally see this PR :-)
I looked only at the GDExtension-related changes so far. There's a bunch of rendering and display server related changes that I haven't looked at.
At high-level, this seems like a pretty good approach. Also, I really appreciate that the code changes to GDExtension are fairly minimal. :-)
On PR #72883, we discussed the possibility of introducing a new concept, perhaps called GDExtensionLoader
, that would encapsulate the loading behavior, so that we could have GDExtensionLibraryLoader
that used OS::open_dynamic_library()
(so the current way) and then maybe a GDExtensionEmbeddedLoader
that would just take an initialization function.
However, given how simple the changes are here, I think we could perhaps save something like that for a future refactor? It could make things a little cleaner by not having to constantly check if GDExtension::library
is null or not, though.
Error err = libgodot->initialize_extension_function(p_init_func, "lib_godot"); | ||
if (err != OK) { | ||
ERR_PRINT("Error initializing extension function"); | ||
} else { | ||
libgodot->set_path("res://__LibGodot"); | ||
GDExtensionManager::get_singleton()->load_extension("res://__LibGodot"); |
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.
This PR introduces a bunch of new names, and, while I don't necessarily want to start bikeshedding, we probably should discuss if these are the names we want to use, and, in particular, are they internally consistent, and consistent with names already used in other parts of Godot.
So, here we have:
"lib_godot"
as the fake entry point name"res://__LibGodot"
as the fake library path. (Also, this needs to be unique among all extensions - with this PR, are developers able to register only one faux GDExtension from their application or multiple? In @Faolan-Rad's PR, if I'm remembering correctly, more than one faux GDExtension could be registered. If multiple are allowed, we'll need a unique path for each.)
And elsewhere we have:
- The
LIBRARY_ENABLED
define - The "Embedded" feature tag
- The general term "embedded" to describe a faux GDExtension registered through libgodot
These seem a little bit inconsistent, using the terms "libgodot", "embedded" and "library" in different places.
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 agree, and we are open to naming suggestions. I agree that it is important to have consistent naming. Should we just try to use LIBGODOT everywhere as the key for this? Or should we move to the more generic "LIBRARY" name?
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.
Coming back to the naming question because it seems there's still some mixed names (although, it's much improved from the last time I went through this PR).
Personally, I don't like the super generic "library".
I'd be for either "LibGodot" or "embedded Godot", since they are more descriptive, with maybe a slight preference for "LibGodot" because that sounds more like a product/feature name, making it easier to refer to.
If we did go for "LibGodot", then we'd end up with a LIBGODOT_ENABLED
define, libgodot://
path, libgodot
feature tag, and "LibGodot" in the docs and other human-readable places.
I'm surprised there hasn't been more bike-shedding about the name :-)
I've tried to get the Godot Engine Github actions continuous integration to pass. Most tests pass except for Windows. https://github.com/V-Sekai/godot/tree/vsk-libgodot-migeran-4.3 Edited: Time is limited for me to work on this, so I hope this can help your efforts. |
Thank you, and thank you for taking the time to review it!
I think introducing the concept of library loaders would benefit not just this use case, but also other language support that are implemented as GDExtensions. If you recall, I proposed a feature like that in my .NET integration proposal: https://docs.google.com/document/d/1QwZpo3oIKHyita1mDOIB_xIxdwJ5rtAmpY4_vP9S02Y/edit?usp=sharing However, I am also not sure if we should try to push this whole loader concept into this PR, we could do that in a follow-up PR. We can discuss this more in the GDExtension meeting. |
Thank you @fire ! Would it be ok if I squash your changes into a single commit, and add it to our branch? |
Do what what works for you. Edited: As the initiator of the libgodot-style patch, I've successfully persuaded several individuals, including |
@kisg Can you review the changes to libgodot from here V-Sekai#35? Here's the modified libgodot_project project working on Windows. https://github.com/V-Sekai/libgodot_project. You may need to force mingw. |
With this latest update most review comments should be fixed. New implementation uses GDExtensionLoader PR as the base: #91166 Also updated to a recent master. Still on the TODO list:
|
311360a
to
e2dfa2b
Compare
17cb62c
to
41a69e1
Compare
- Implements the concept of GDExtension loaders that can be used to customize how GDExtensions are loaded and initialized. - Moves the parsing of `.gdextension` config files to the new `GDExtensionLibraryLoader`. - `GDExtensionManager` is now meant to be the main way to load/unload extensions and can optionally take a `GDExtensionLoader`. - `EditorFileSystem` avoids unloading extensions if the file still exists, this should prevent unloading extensions that are outside the user project.
* Based on top of the GDExtensionLoader PR: godotengine#91166 * 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. * Allow specifying an external native rendering surface handle to render Godot into the UI of a host application. * It is also possible to embed multiple Godot windows into the UI of a host application. * Currently supported on MacOS and iOS 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 Co-Authored-By: Gabor Koncz <gabor.koncz@migeran.com>
See #72883 (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.
Overall, this is a pretty good job, there's some suggestions I've made to improve this PR.
@YakoYakoYokuYoku Where did you leave suggestions? I can't see any code suggestions |
@AThousandShips [Update] 24.07.2024 => now that we have
to create sample app |
@GeorgeS2019 We're in feature freeze for 4.3, and this PR is slated for 4.4. Please be patient. |
@@ -162,7 +162,6 @@ env.__class__.use_windows_spawn_fix = methods.use_windows_spawn_fix | |||
|
|||
env.__class__.add_shared_library = methods.add_shared_library | |||
env.__class__.add_library = methods.add_library | |||
env.__class__.add_program = methods.add_program |
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.
This line shouldn't be removed as it can be used later on with the addition of an option to build a program wrapper around the library.
else: | ||
env.__class__.add_program = methods.add_program |
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.
Per above, these two lines should be removed.
// Iterate the libraries, finding the best matching tags. | ||
String best_library_path; | ||
Vector<String> best_library_tags; | ||
for (const String &E : libraries) { |
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.
Nit, change this E
into lib
or something else.
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.
E is the standard godot C++ style.
@@ -25,6 +25,7 @@ SConscript("coremidi/SCsub") | |||
SConscript("winmidi/SCsub") | |||
|
|||
# Graphics drivers | |||
SConscript("apple/SCsub") |
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.
This results in build failures in platforms other than Apple, please use an if
statement.
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'd not recommend doing a git move here, please undo it and create a new file instead.
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.
Do you know how to force git do this operation? This is a automatic action.
ERR_PRINT(vformat("Failed to initialize %s context", rendering_driver)); | ||
memdelete(rendering_context); | ||
rendering_context = nullptr; | ||
return; |
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.
Can r_error
be set here?
|
||
#if defined(GLES3_ENABLED) | ||
if (rendering_driver == "opengl3") { | ||
ERR_FAIL_MSG("Embedded OpenGL rendering is not yet supported."); |
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.
Before this line r_error
could be set to ERR_UNAVAILABLE
or another value.
//#if defined(GLES3_ENABLED) | ||
// drivers.push_back("opengl3"); | ||
//#endif |
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.
Maybe add a TODO
comment.
if (p_id != INVALID_WINDOW_ID) { | ||
_window_callback(input_event_callbacks[p_id], p_event); | ||
} else { | ||
for (const KeyValue<WindowID, Callable> &E : input_event_callbacks) { |
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.
This E
could be changed for something more meaningful, like cb
or something.
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.
E is the typical coding style of godot engine, but can be better!
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.
Some characteristics, like DPI, maximum and minimum size, focus, plus many more can be queried from the native surface. For now TODO
comments can suffice as what I've said gets out of the scope of this PR.
I've messed up with the PR review, my bad. Regardless, here they go. |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
[WIP 18 Aug 2024] There is now working examplemigeran/libgodot_project#1 (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.
From the perspective of GDExtension, this PR is looking really good now!
Now that it's built on top of the Raul's "GDExtension loaders" PR, the changes here are really pretty minimal.
I'm still not crazy about exposing GodotInstance
, but now that it's abstract, there shouldn't be any risk of developers getting ahold of one when they shouldn't. I think I can live with that.
At this point, I think it's primarily the rendering and platform-related code that needs review and discussion.
@@ -63,6 +66,7 @@ class GDExtensionManager : public Object { | |||
|
|||
public: | |||
LoadStatus load_extension(const String &p_path); | |||
LoadStatus load_function_extension(const String &p_path, GDExtensionConstPtr<const GDExtensionInitializationFunction> p_init_func); |
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.
Another naming nitpick: "function extension" is an unclear term that we don't really use anywhere else.
Maybe load_extension_from_function()
?
Or, if we can decide on an overall name for this feature we could use that term. So, if we standardize on "LibGodot", then this could be load_libgodot_extension()
? Or, if we standardize on "embedded", then it could be load_embedded_extension()
?
Error err = libgodot->initialize_extension_function(p_init_func, "lib_godot"); | ||
if (err != OK) { | ||
ERR_PRINT("Error initializing extension function"); | ||
} else { | ||
libgodot->set_path("res://__LibGodot"); | ||
GDExtensionManager::get_singleton()->load_extension("res://__LibGodot"); |
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.
Coming back to the naming question because it seems there's still some mixed names (although, it's much improved from the last time I went through this PR).
Personally, I don't like the super generic "library".
I'd be for either "LibGodot" or "embedded Godot", since they are more descriptive, with maybe a slight preference for "LibGodot" because that sounds more like a product/feature name, making it easier to refer to.
If we did go for "LibGodot", then we'd end up with a LIBGODOT_ENABLED
define, libgodot://
path, libgodot
feature tag, and "LibGodot" in the docs and other human-readable places.
I'm surprised there hasn't been more bike-shedding about the name :-)
|
||
#if !defined(ANDROID_ENABLED) | ||
static JavaClassWrapper *java_class_wrapper = nullptr; | ||
#endif | ||
|
||
void register_core_android_api() { | ||
#if defined(ANDROID_ENABLED) | ||
GDREGISTER_ABSTRACT_CLASS(RenderingNativeSurfaceAndroid); |
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.
Will a stub class for RenderingNativeSurfaceAndroid
be registered when not on Android? Ideally, we'd want a class the same name and all the same methods available on all platforms, but only a version that actually works when on Android.
We do a similar thing for JNISingleton
, JavaClassWrapper
, and JavaClass
- those are always registered (although, sometimes in a different way) on all platforms, but when not on Android, their real functionality is removed.
I haven't looked at the other platforms, but we'd want to do the same for them as well. Or, alternatively, we could just expose the base class and a factory method, and then developers never need to interact with the classes for the individual platforms, just the base class (which is sort of like what we do for DisplayServer
), so the child classes don't even need to be exposed.
The point is just that we don't want classes to appear/disappear when on different platforms. Otherwise, developers get errors depending on which platform they opened their project up on, just because they refer to the class name in their GDScript.
Hi, Godot as a lib sounds very great. I ask because to use the lib in a c# test framework to direct run the engine instead of starting an external process. What is the difference to #72883 ? |
Also curious about if it will be published on NuGet. My use case involves deserializing Godot resources from an ASP.NET Core project so we can read values easily from tres/res files. Having this in a NuGet package would make consumption a breeze. |
Features
Why LibGodot?
LibGodot has a number of different use cases.
How to try it out?
We provide a Github repository with preconfigured build environment, sample apps and more information: https://github.com/migeran/libgodot_project
Migeran LibGodot Design
The core idea of the LibGodot feature is to build upon Godot's strong extension capabilities: its modularity, powerful type system and the GDExtension API.
The main advantage of this approach is, that this way LibGodot may be used from any language that has a GDExtension binding (e.g. C++, Swift, Rust, Python, ... etc.), with only minimal changes to the binding.
Godot Instance Lifecycle Management
The main class added by the LibGodot design is the GodotInstance:
This class is made accessible over the GDExtension API. This class can be used to control a Godot instance.
To actually create a Godot instance a new symbol is added to the GDExtension API:
This function can be used to create a new Godot instance and return a GodotInstance object reference to control it. Both samples show how easy it is to bind this function and then use the generated GDExtension API bindings with the returned GodotInstance object.
To properly destroy a Godot instance the GDExtension API is extended by another symbol:
This function is made available through the GDExtension API's getProcAddress mechanism.
Note: Due to Godot's internal architecture (the use of global data structures and singletons) only one Godot instance may be created in a process.
Embedding Godot UI
Note: UI embedding is currently implemented for Apple platforms. Please read the Next Steps section for more information on the status of other platforms.
To allow for embedding the Godot UI into a host process the host process needs to be able to pass the necessary data about a native surface where
Godot may render its contents. The form of this native surface is entirely platform and rendering backend specific.
The Migeran LibGodot design adds the following types to Godot to allow this:
DisplayServerEmbedded
implementation which uses externally provided native surfaces as its rendering targets.RenderingNativeSurface
class and its associated platform specific subclasses, e.g.RenderingNativeSurfaceApple
.Window
class is extended by aset_native_surface(Ref<RenderingNativeSurface>)
method which allows specifying the rendering target of a Window in a typesafe, platform independent manner. It is even possible to change the rendering target of a Window dynamically during its lifecycle.These classes are all exposed to the GDExtension API, so it is easy to use them from the host process.
The
DisplayServerEmbedded
class also exposes methods that allow the injection of input events from the host process into the Godot instance.The RenderingNativeSurface class hierarchy has an additional function: provides a mechanism using the Factory Method design pattern to create compatible RenderingContextDrivers for a native surface.
This allowed us to make the
DisplayServerEmbedded
class platform agnostic.We also refactored the other
DisplayServer
implementations to useRenderingNativeSurface
during initialization.Since all these classes are exposed over the GDExtension API, these can be seamlessly used from any supported language with a GDExtension binding.
Rationale for RenderingNativeSurface Design
For those who are familiar with Godot Engine internals: there was already a way to pass in platform specific native surface data (called
WindowPlatformData
) during initialization.Why is this refactoring necessary to a full fledged reference counted object?
We chose reference counting because it makes it easier to use on the client side, no need to manage the lifecycle manually. Earlier versions of this PR used simple Godot Objects, but they required manual memory management which was error prone.
While on Apple platforms it is very easy to pass in a
CAMetalLayer
reference, and Metal (and thus MoltenVk) will happily render into it, other platforms impose way more restrictions.For example: On Windows and Linux a separate Vulkan instance and the use of external textures is required to render Godot on a separate thread from the host application's main renderer thread.
This is not just a theoretical option: We already implemented a prototype on Linux and Windows based on Godot 4.2, where the host process and Godot are using Vulkan External Textures to pass the rendered frames from Godot to the host process. We intend to upgrade and refactor this patch to the current version of the LibGodot patch.
To use External Textures a thread-safe queue has to be implemented between between the host process and Godot, and a reference-counted RenderingNativeSurface subclass would be an ideal place to implement it, e.g.
RenderingNativeSurfaceVulkanExternalTextureWin32
.Next Steps
Open for Discussion
We are happy to discuss any part of the design, naming conventions, additional features ... etc.
Merging for Godot 4.3 as an Experimental Feature
Godot 4.4 Developments
During the Godot 4.4 development cycle additional features may be added, for Example:
Sponsors & Acknowledgements
* The GDExtension registration of the host process & build system changes were based on @Faolan-Rad's LibGodot PR: #72883
About the Authors
This feature was implemented by Migeran - Godot Engine & Robotics Experts.