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

Allow creating GDExtension plugins from inside the Godot editor #90979

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Apr 21, 2024

This PR implements godotengine/godot-proposals#3367 and godotengine/godot-proposals#9306 (not exactly as the proposals describe, but solves the same problem).

The status quo for GDExtension development is really unfortunate. For the most part the only easy way to get started is to copy-paste an existing GDExtension project and use it as a template. If you don't do this, you'd need to follow the documentation, which is a massive behemoth that manually explains to the user how to create each file from scratch, and it would take a user hours to follow this guide.

Really, I believe it should be as simple as making a GDScript plugin:

  • Click a button to open a wizard to make an extension.
  • Give your extension a name.
  • Select a language from the dropdown menu.
  • Click Create.

This is what this PR implements. Just go into Project Settings -> GDExtension:

Screenshot 2024-05-27 at 12 02 11 AM

Next, click Create GDExtension, type in a base name, click "Create", and then it will create and compile an extension.

Screenshot 2024-05-27 at 1 30 48 AM

The library name and path are optional. If you do not specify these, they will be determined automatically.

The dialog includes hover tooltips if you hover over the boxes, which explain what each field is and what the values you write in it should be.

The language system is designed to be completely language-agnostic. The dialogs have no clue about the existence of C++ or SCons. All of the C++ and SCons specific code is isolated in one class, and in the future we can allow editor plugins to extend this. It's not exposed as of this PR in case we want to iterate on the internal code first, but we can expose it whenever, preferably in a future PR to avoid this one becoming too big.

As mentioned by proposal godotengine/godot-proposals#3367, this sets up the build system with a build command (scons), the .gitignore files, the .gdextension file, the type registration boilerplate, and it even git clones godot-cpp. By default, it will also initiate scons to compile the extension, but this does take awhile, so you can skip this step by unchecking the "Compile now?" box.

Note that this PR does not include setting up system-level dependencies, such as installing a C++ compiler, installing Git, installing SCons, etc. This varies widely by operating system and it would be a slippery slope to try and have that. This PR is purely focused on the files that go in your project, not the system-wide dependencies. This PR does not try to detect a C++ compiler, but it will warn the user if either Git or SCons is not found on their system.

When you create a GDExtension plugin with this PR, one example node is provided called ExampleNode. Like the Blender default cube, this is expected to be replaced by users as soon as possible. However, its existence is useful to both new and experienced users, since it serves 2 functions: 1) As a reference for how to make a node type, including the code, registration, docs, icon, etc, and 2) A minimal test to check if your GDExtension plugin works (don't see it in the editor? It's not working).

Assuming you have the system-wide dependencies installed, and you leave the "Compile now?" checkbox enabled, and you wait for it to finish, the extension immediately works. No fuss, the example node is just there.

Screenshot 2024-05-27 at 1 27 30 AM

Assuming it was compiled, it will show up in a list of GDExtensions. This list will contain every loaded GDExtension, regardless of whether it was been created by the create dialog. It gets this list from GDExtensionManager, so it will not include any non-compiled or otherwise broken extensions.

Screenshot 2024-05-27 at 12 08 11 AM

If you change the code, you just need to run scons in either the extension folder or the project root. A future idea would be to detect this in projects and provide a "Build" button in the editor UI like we already do for C#, but that is technically separate from what this PR does.

The files look like this:

Screenshot 2024-04-21 at 4 25 19 AM

The SConstruct at the project root is extremely barebones, it is only one line to call into another SConstruct. It is designed to be simple enough so that more GDExtension plugins can be added to the same Godot project with no manual work. The _ensure_file_contains helper function in this PR completely handles the merging of more calls into this file. Except for deleting the first plugin's ExampleNode, you can just make 2 plugins back-to-back with no conflicts. If you want to have 20 GDExtension plugins, this PR will let you do that just fine, with one call to scons in the root folder able to compile all of them at once.

This PR provides 2 options for making a GDExtension plugin: "C++ with SCons, GDExtension only" or "C++ with SCons, GDExtension and engine module". The former creates a standalone GDExtension C++ plugin, with the only files stored in the root of the project being a .gitignore and barebones SConstruct.

The engine module option provides a few more files like SCsub and config.py, provides lots of #ifdef directives to support both GDExtensions and modules, and places most of the C++ code in the root of the folder, which is more suitable for making a Godot engine module. This way if users want to target both GDExtension and modules with one codebase, they can do this as easily as selecting it from the dropdown. This option turns the project's root folder into a module, so only one module is allowed per project.

Screenshot 2024-05-27 at 12 10 59 AM

When in this mode, you can either compile for GDExtension the same as with the "only" option, or you can copy-paste the entire Godot project into the engine modules folder, naming the folder the same as the addon folder name, and it works.

The source files are designed to handle the extension+module case, but are automatically simplified by this PR's project generator when making a non-module extension, including by looking at the #if preprocessor directives to determine which lines of code to exclude.

In the extension list, you can click the pencil button on any of these items to open a menu to edit the GDExtension.

Screenshot 2024-09-20 at 2 01 18 AM

Like the create dialog, the dialog includes hover tooltips which describe each field. However, for the Libraries and Icons fields, the hover tooltips are placed on the Label nodes instead of the Dictionary editors.


EDIT: This PR has changed a lot since first opening. For the record, here's the old PR description:

This PR implements godotengine/godot-proposals#3367 (not exactly as that proposal describes, but solves the same problem).

The status quo for GDExtension development is really unfortunate. For the most part the only easy way to get started is to copy-paste an existing GDExtension project and use it as a template. If you don't do this, you'd need to follow the documentation, which is a massive behemoth that manually explains to the user how to create each file from scratch, and it would take a user hours to follow this guide.

Really, I believe it should be as simple as making a GDScript plugin:

  • Click a button to open a wizard to make a plugin.
  • Give your plugin a name.
  • Select GDExtension from the dropdown menu (only step added compared to GDScript plugins).
  • Click Create.

This is what this PR implements. Just go into Project Settings -> Plugins, click Create New Plugin, and select GDExtension.

Screenshot 2024-04-21 at 3 55 37 AM

As mentioned by proposal godotengine/godot-proposals#3367, this sets up the build system with a build command (scons), the .gitignore files, the .gdextension file, the type registration boilerplate, and it even git clones godot-cpp. By default, it will also initiate scons to compile the extension, but this does take awhile, so you can skip this step by unchecking the "Compile now? (slow)" box.

Note that this PR does not include setting up system-level dependencies, such as installing a C++ compiler, installing Git, installing SCons, etc. This varies widely by operating system and it would be a slippery slope to try and have that. This PR is purely focused on the files that go in your project, not the system-wide dependencies. This PR does not try to detect a C++ compiler, but it will warn the user if either Git or SCons is not found on their system.

When you create a GDExtension plugin with this PR, one example node is provided called ExampleNode. Like the Blender default cube, this is expected to be replaced by users as soon as possible. However, its existence is useful to both new and experienced users, since it serves 2 functions: 1) As a reference for how to make a node type, including the code, registration, docs, icon, etc, and 2) A minimal test to check if your GDExtension plugin works (don't see it in the editor? It's not working).

Assuming you have the system-wide dependencies installed, and you leave the "Compile now?" checkbox enabled, and you wait for it to finish, the extension immediately works. No fuss, the example node is just there:

Screenshot 2024-04-21 at 4 19 38 AM

If you change the code, you just need to run scons in the root. A future idea would be to detect this in projects and provide a "Build" button in the editor UI like we already do for C#, but that is technically separate from what this PR does.

The files look like this:

Screenshot 2024-04-21 at 4 25 19 AM

The SConstruct at the root is extremely barebones, it is only one line to call into another SConstruct. It is designed to be simple enough so that more GDExtension plugins can be added to the same Godot project with no manual work. The _ensure_file_contains helper function in this PR completely handles the merging of more calls into this file. Except for deleting the first plugin's ExampleNode, you can pretty much just make 2 plugins back-to-back with no conflicts. If you want to have 20 GDExtension plugins, this PR will let you do that just fine, with one call to scons in the root folder able to compile all of them at once.

This PR provides 2 options for making a GDExtension plugin: "GDExtension C++ only" or "GDExtension C++ and engine module". The former creates a standalone GDExtension C++ plugin, with the only files stored in the root of the project being a .gitignore and barebones SConstruct.

The engine module option provides a few more files like SCsub and config.py, provides lots of #ifdef directives to support both GDExtensions and modules, and places most of the C++ code in the root of the folder, which is more suitable for making a Godot engine module. This way if users want to target both GDExtension and modules with one codebase, they can do this as easily as selecting it from the dropdown. This option turns the project's root folder into a module, so only one module is allowed per project:

Screenshot 2024-04-21 at 4 28 47 AM

When in this mode, you can either compile for GDExtension the same as with the "only" option, or you can copy-paste the entire Godot project into the engine modules folder, naming the folder the same as the base name (the first name you entered in the Create GDExtension dialog), and it works.

The source files are designed to handle the extension+module case, but are automatically simplified by this PR's project generator when making a non-module extension, including by looking at the #if preprocessor directives to determine which lines of code to exclude.

TODO list before undrafting this PR:

@Riteo
Copy link
Contributor

Riteo commented Apr 21, 2024

Wow, this looks great! I wonder, does this also resolve godotengine/godot-proposals#9306 ? It looks like the same problem IMO.

@Naros
Copy link
Contributor

Naros commented Apr 21, 2024

This looks great, but I also definitely think its worthwhile to consider support for other build systems beyond scons. There has been a tremendous amount of work put into the cmake build in godot-cpp, and I think its at least worthwhile for supporting that build system if someone picks "GDExtension C++ only".

@Lazy-Rabbit-2001
Copy link
Contributor

Lazy-Rabbit-2001 commented Apr 22, 2024

This looks wonderful as I've been annoying with handling the preparation of making GDExtension manually for days.
However, I think there could be an option to decide if godot-cpp is global or not, as some devs may use the same godot-cpp to avoid repeated compilations on this folder because of their parallel development on multiple GDExtensions.
For example, according to current status of pr, a dev may produce two GDExtensions like this:

addons

exm_a

src

godot_cpp

exm_b

src

godot_cpp

See? There are two godot_cpp-s in seperate projects, but the dev may hope to use only one for these two GDExtensions, like this:

addons

_bin

godot_cpp

exm_a

src

...

exm_b

src

...

As the direct placement of folder godot_cpp may bring ambiguity and vagueness to devs, it's better to place it in a single common folder named _bin or something else clear that resonates with the devs.

@aaronfranke
Copy link
Member Author

@Naros The big question I have is what would this feature look like? Should cmake be included as one of the supported options out of the box? Should it be plugable? If so how? I guess plugins should add more options to the dropdown menu, so we would have "GDExtension C++ only (scons)" and "GDExtension C++ only (cmake)"? Or is it worth refactoring this menu to have a more generalized "GDExtension" option and a second dropdown for "C++ SCons", "C++ CMake", etc?

@Lazy-Rabbit-2001 I agree, but there are some technical challenges when trying to make the plugins share godot-cpp. When I try to do this, and both call into the same SConstruct, it is unable to compile from the top-level SConstruct:

scons: warning: Two different environments were specified for target gen/src/variant/utility_functions.cpp,
        but they appear to have the same action: scons_generate_bindings(target, source, env)
File "/Users/aaronfranke/workspace/projects/gdextension-test/addons/godot-cpp/tools/godotcpp.py", line 463, in _godot_cpp

scons: *** Two environments with different actions were specified for the same target: src/godot.macos.template_debug.universal.o

I would need some help from the build system gurus to make this work.

@dsnopek
Copy link
Contributor

dsnopek commented Apr 23, 2024

Thanks for taking this on! It looks really cool :-)

I did a quick test of the PR, and the "Compile on" checkbox didn't seem to work for me. It detected scons and git, but it encountered an error when trying to run it:

scons: Reading SConscript files ...
Auto-detected 32 CPU cores available for build parallelism. Using 31 cores by default. You can override it with the -j argument.
Building for architecture x86_64 on platform linux
KeyError: 'arch_suffix':
  File "/home/dsnopek/Sync/Projects/games/gdext-creation-test/SConstruct", line 4:
    SConscript("addons/zap/SConstruct")
  File "/usr/lib/python3/dist-packages/SCons/Script/SConscript.py", line 661:
    return method(*args, **kw)
  File "/usr/lib/python3/dist-packages/SCons/Script/SConscript.py", line 598:
    return _SConscript(self.fs, *files, **subst_kw)
  File "/usr/lib/python3/dist-packages/SCons/Script/SConscript.py", line 287:
    exec(compile(scriptdata, scriptname, 'exec'), call_stack[-1].globals)
  File "/home/dsnopek/Sync/Projects/games/gdext-creation-test/addons/zap/SConstruct", line 35:
    env["arch_suffix"],
  File "/usr/lib/python3/dist-packages/SCons/Environment.py", line 405:
    return self._dict[key]

If I change the SConstruct to remove the env["arch_suffix"] part, then it builds fine when running scons manually. After that, the ExampleNode class appears and I can add it to my scene!

It looks like this creates a plugin.cfg file, which leads to an entry on the "Plugins" tab in "Project Settings". However, GDExtensions aren't plugins in this sense, and enabling/disabling the created plugin there will actually have no effect. We do plan to eventually add the ability to enable/disable GDExtensions in "Project Settings", but work hasn't started on that yet.

However, GDExtensions are quite different from plugins in many ways. It may make sense for GDExtensions to have their own tab distinct from "Plugins"? And, if so, then the UI here should probably be disentangled from the "Create New Plugin" form and process.

Anyway, this is a great start!

@Naros
Copy link
Contributor

Naros commented Apr 23, 2024

The big question I have is what would this feature look like? Should cmake be included as one of the supported options out of the box? Should it be plugable? If so how? I guess plugins should add more options to the dropdown menu, so we would have "GDExtension C++ only (scons)" and "GDExtension C++ only (cmake)"? Or is it worth refactoring this menu to have a more generalized "GDExtension" option and a second dropdown for "C++ SCons", "C++ CMake", etc?

So first, I think David is right, we should completely separate GDExtension from GDScript plug-ins here, 💯 . An independent UI has several benefits:

  • Avoids confusing GDScript plug-in creators with C++ / GDExtension noise.
  • Allows creating a screen tailored for GDExtension and its toolings.

On the GDExtension create dialog, I could imagine this being a bit similar to Project Settings with a tree on the left, and a panel on the right where we display context-specific options based on the choice on the left. The left panel tree might look like this:

- General 
- Build, Execution, and Deployment
  - Toolchains
    - Custom compiler
  - CMake
  - Meson
  - Scons
  - ...
- Plugins
  - ... 

The General tab allows you to define basic data about the extension, its name, a custom entry point name, the base path for where the compiled binaries should be copied, i.e. res:\\addons\<addon-name>\bin being the default. Additionally, this is where the user can define the minimum compatibility, hot-reload, etc.

Under the Build tab, we'd have sections for each supported build system that would be output for the user. These tabs could include specific options that can be customized for that specific build system. For example, here's what I could imagine being the case for CMAKE:

image

The Plugins section is really some future scope I would say, but could be where the build files get customized, additional classes added, or some other customizations are done that can be tailored to an organization's needs.


And circling back to Limbo's idea of a shared godot-cpp, this got me thinking about whether the C++ code should really reside in the project folder itself or would it make sense to reside in something like user://, with top-level paths like:

  • user://godot-cpp (shared godot-cpp)
  • user://gdextension/<extension-name>

I'm just not sure I like the idea of embedding the C++ code into the Godot project, but I'm not 100% sold either way. But now I do question if some of these settings belong configurable in the Project Settings and not in the "Create Extension" dialog 🤔

@HeadClot
Copy link

HeadClot commented May 20, 2024

Bit of a question about this Draft PR will this trigger a compile after the GDExtention is created or do will we have to do that manually?

@aaronfranke
Copy link
Member Author

aaronfranke commented May 20, 2024

@HeadClot It attempts a compile for you, but this may fail if Git, SCons, or a C++ compiler/linker/etc are missing.

@HeadClot
Copy link

@HeadClot It attempts a compile for you, but this may fail if Git, SCons, or a C++ compiler/linker/etc are missing.

Got it thank you for the info :)

@dsnopek
Copy link
Contributor

dsnopek commented May 23, 2024

Decide how we want this to work for non-C++ languages (currently the code is not designed to be plugable).

I think it probably makes sense to make this pluggable via an editor plugin, similar to the EditorExtensionSourceCodePlugin proposed in godotengine/godot-proposals#9806 (or even just being additional methods on that same plugin - keeping everything related to managing GDExtension source together makes sense).

Also, it could make sense to include the C++ support in a godotcpp module? That way, we can disable the module on certain platforms (for example, web) or folks could make custom builds of Godot that don't include this additional integration with C++ (if, for example, they wanted an editor build that focused on a different GDExtension language).

@aaronfranke aaronfranke force-pushed the create-gdextension branch 4 times, most recently from 3199f9a to 6d83772 Compare May 27, 2024 09:21
@aaronfranke aaronfranke marked this pull request as ready for review May 27, 2024 09:31
@aaronfranke aaronfranke requested review from a team as code owners May 27, 2024 09:31
@aaronfranke
Copy link
Member Author

aaronfranke commented May 27, 2024

I overhauled the entire UI frontend of this PR. Instead of using the same dialog as GDScript plugins, now there is an entirely separate tab with 2 dialogs for creating and editing GDExtensions.

I have updated the OP, please read that for a summary of how the workflow looks like. In addition to what's in the OP, here is a short description of what the classes are:

  • ProjectSettingsGDExtension: Lists GDExtensions as a tab in Project Settings. Allows opening the dialogs.
  • GDExtensionCreateDialog: Dialog for creating new GDExtensions. Uses classes that extend GDExtensionCreatorBase to do the heavy lifting. Which one the dialog uses depends on which is selected in the dropdown.
  • GDExtensionConfigDialog: Dialog for configuring/editing GDExtensions.
  • GDExtensionCreatorBase: Base class for GDExtension creator classes, the classes which are able to take in a few parameters and generate a GDExtension for some language. In the future when we expose the ability for users to add custom classes for new languages or build systems, they would extend this class.
  • CppSconsGDExtensionCreator: Extends GDExtensionCreatorBase, used for generating C++ with SCons extensions. This is the only class that knows about C++ and SCons, the other classes are language-agnostic.

If I change the SConstruct to remove the env["arch_suffix"] part, then it builds fine when running scons manually.

This should be fixed now. It will now add arch_suffix if it's missing.

It looks like this creates a plugin.cfg file, which leads to an entry on the "Plugins" tab in "Project Settings".

This is gone now.

And circling back to Limbo's idea of a shared godot-cpp, this got me thinking about whether the C++ code should really reside in the project folder itself or would it make sense to reside in

The create dialog now allows specifying any path. You can use res://gdextension/my_extension if you want, or even use a top-level folder using res://my_extension.

@aaronfranke aaronfranke force-pushed the create-gdextension branch from 6d83772 to 8f4b938 Compare May 29, 2024 23:37
@KoBeWi
Copy link
Member

KoBeWi commented Jun 4, 2024

EDIT:
The solution might be adding a new tab called Addons, which would have sub-tabs Plugins and GDExtension (similar to how Localization has sub-tabs.

  • When creating extension and typing a name, there is a noticeable delay between each letter (at least in debug build), likely caused by field updating. There could be some debouncing applied here.
godot.windows.editor.dev.x86_64_WmCRfH6YBH.mp4

EDIT:
The dialog also checks for git and scons, which might be another thing slowing it down. This should be cached, e.g. when opening the dialog.

  • While performing initial compilation the editor is completely unresponsive. The compilation should run in an independent process, to not stall the editor. You could display indeterminate progress bar, or if you are ambitious, you can show compilation progress and output in real time, using the new execute_with_pipe() method.

  • After compiling finishes, the new GDExtension does not appear in the list until you click the window. Seems like editor ends unfocused somehow and clicking it refreshes the list or something. (might need further testing)

  • The layout for editing extension is awkward.
    image
    You could add some text to the checkbox (e.g. On or Enabled) and align it to the right.

Despite these minor problems, the extension is created correctly and is usable out-of-the-box, which is very nice.

@Ughuuu
Copy link
Contributor

Ughuuu commented Oct 26, 2024

Tested it locally, the compilation of the addon (if checkbox is checked) takes a long time and is not done on threads, so it freezes the editor.
As for other extensions, what is the plan to support those (eg. rn this support godot-cpp with c++, what about godot-rust, godot-go, etc.?). Will there be a way to add dropdown values to this programatically? Or should a PR be opened to add more options here?

@dsnopek
Copy link
Contributor

dsnopek commented Nov 13, 2024

  • Overall, I'm not sure whether we should hardcode a lot of the SCons setup for godot-cpp extensions here, instead of just using Git to clone https://github.com/godotengine/godot-cpp-template/ or a new one if it's not minimal enough. The SCons setup for godot-cpp changes frequently and hardcoding bits of it here doesn't seem too appealing to me.

I definitely don't think we should use 'godot-cpp-template' for this:

  1. It uses a very specific project layout, where the Godot project and the C++ source code are in sibling directories, but you can't really retrofit that to an existing project that wasn't already setup that way
  2. I think we should reserve the right to change the godot-cpp-template without needing to maintain any sort of compatibility. Folks can copy the template, and it should work at any given moment, but for the Godot editor to use it, we'd need to have branches for old Godot versions or something like that (ie. a 4.4 branch that we make sure will continue to work with Godot 4.4, even after the latest stable is Godot 4.12)

We could make a new repo on GitHub that exists purely for this purpose, and I think that'd be OK.

However, we're really talking about a small amount of code. With this PR checked out, I ran find editor/plugins/gdextension/cpp_scons/template/ -type f | xargs wc -l and the template is only 328 lines of code. It seems simpler just to embed that?

I understand the risk of godot-cpp's SCons configuration changing, but I don't think we've made many (or perhaps any?) breaking changes to it in the past? And we already maintain version-specific branches (ie. 4.3, 4.4, etc) for godot-cpp that should continue working, so that shouldn't add an additional maintenance burden.

@aaronfranke aaronfranke requested a review from a team as a code owner November 14, 2024 11:02
@aaronfranke
Copy link
Member Author

aaronfranke commented Nov 14, 2024

I attempted to move the compiling to a thread, but I ran into some issues. Most importantly, the console will show errors for the first frame of the editor when it detects an uncompiled GDExtension, and we don't want that to happen since the user has done nothing wrong. I don't know how to block the editor from refreshing the file system while still allowing it to redraw frames, or only block GDExtension, the best solution I can think of right now is to block the main thread of the editor (what this PR already does). Here's the branch if anyone wants to try (or tell me what to do).

@aaronfranke aaronfranke force-pushed the create-gdextension branch 2 times, most recently from 2a6a8f4 to 1bd79bf Compare November 26, 2024 11:59
@ckaiser
Copy link
Contributor

ckaiser commented Dec 15, 2024

Tested on Windows, but it couldn't find my scons installation, turns out it's because I'm using pyenv-win, so after pointing my PATH straight to where the specific scons.exe was located I managed to get it to detect it and compile.

I'm assuming this is because pyenv sets things up with a .bat file to redirect to the environment instead of it being a straight up executable, so it works fine when ran from the terminal but not otherwise? I doubt my installation is particularly unique so perhaps a check to also attempt to run cmd.exe /C scons --version on Windows might be necessary.

@Repiteo Repiteo modified the milestones: 4.4, 4.x Jan 20, 2025
@aaronfranke aaronfranke force-pushed the create-gdextension branch 5 times, most recently from a371613 to 3ae8150 Compare February 17, 2025 10:37
Copy link
Contributor

@Ivorforce Ivorforce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All in all I really like this PR. I think it will make GDExtension development and integration a lot more accessible!

I do have a few comments about the proposed structure of the GDExtension itself. I think some of them should be addressed before a merge — It wouldn't be great to "settle" for one structure only to have to patch it around later. I also haven't gone through the whole PR yet so I'll probably revisit the rest later.

I agree with @dsnopek that embedding the template is the right call. If we use an externally defined template (like godot-cpp-template), we'll just have sync issues whenever we want to make interesting changes to either the template creation code or the template itself.
It's a little unfortunate that this means we'll have a similar boilerplate twice, but perhaps we'll find a good solution for that some time.

)
else:
library = env.SharedLibrary(
"{}/lib{}.{}.{}.{}{}".format(
Copy link
Contributor

@Ivorforce Ivorforce Feb 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name should be

f"{env.subst('$SHLIBPREFIX')}{extension_name}{env['suffix']}{env.subst('$SHLIBSUFFIX')}"

The reason is that other suffixes, like .threads, are included in env["suffix"], making it more future proof with less explicity. env["suffix"] is meant for exactly this job :)

Edit: I'm not actually sure if I tested SHLIBPREFIX and SHLIBSUFFIX are needed. I think SCons may insert them automatically. lib should definitely not be included explicitly though, as it only applies on UNIX systems.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this changes behavior by naming the file .template_debug. instead of .debug.. This means that we no longer have the feature where the Godot editor can load a debug template build of an extension.

Copy link
Contributor

@Ivorforce Ivorforce Feb 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I understand, could you explain why the changed name implies a change of capability?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the .gdextension file would need to point to 2 different files for editor and non-editor builds. The editor one would point to .editor. and the template one would point to .template_debug.. So the editor won't see the existence of a .template_debug. file and vice versa.

Copy link
Contributor

@Ivorforce Ivorforce Feb 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I think I see now.
It's true that, without calling target=editor builds the same as target=template_debug builds in the GDExtension, the .gdextension file will need to settle for linking only one of the two to load in the editor.

The distinction comes from whether the user wants separate .editor and .template_debug builds for the GDExtension at all:

  • The user wants both builds separately: The two files should not be called the same. They should result in differently called binaries, and be linked in the .gdextension file separately.
  • The user wants no separate .editor build (use debug for editor, too): . In this case, they would want to error in SConstruct if target=editor is passed, because this is not a valid target for this GDExtension.

I don't think calling the target=editor and target=template_debug builds the same suits either use-case well.
Disabling editor builds sounds like the more sensible default to me, for most GDExtensions.

Copy link
Contributor

@Ivorforce Ivorforce Feb 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I almost forgot - env["suffix"] currently adds .dev to the suffix for dev_build=yes. This is unfortunate because it means that the .gdextension needs to load either dev_build=yes or dev_build=no (the default) in the editor, and cannot do either depending on what the user wants to test right now. Arguably, this should be 'expected', but there are regular reports from users that run into this as a pitfall.

I have an open PR to change this (remove .dev), but @Faless has expressed scepticism that this is the right call.

An alternative would be to use env["suffix"].replace(".dev", "") in the SConstruct for the binary name. This is what I do in my own GDExtension right now.

We can always patch this after the merge, but it's worth discussing now too since there's probably some time before we merge this PR.

Copy link
Member Author

@aaronfranke aaronfranke Feb 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, they would want to error in SConstruct if target=editor is passed, because this is not a valid target for this GDExtension.

In my case, I want it so that if the user builds with scons then that will work in the editor even without editor features, or alternatively the user can write scons target=editor to build with editor features. I want to avoid a situation where the user compiles with scons and gets something the editor can't load at all. It would be weird to make the default scons build impossible for the editor to load.

Copy link
Contributor

@Ivorforce Ivorforce Feb 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand the use case. Do you want your extension to load from the editor with features missing? This sounds like it could be confusing to someone newly introduced to use the extension.
Is there a reason you don't just add the editor features to the debug build, instead of using a separate editor target?

Copy link
Member Author

@aaronfranke aaronfranke Feb 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also consider this case:

  • If we make this change, the "Compile Now?" checkbox will need to run scons target=editor instead of scons to let the extension work in the editor.
  • User changes the C++ code, then manually runs scons.
  • User wonders why their code changes aren't being reflected.

I think if we made target=editor the default target for godot-cpp, we could make this change. But as-is, I think this would cause confusion.

# Create the library target (e.g. lib__LIBRARY_NAME__.linux.debug.x86_64.so).
if env["platform"] == "macos":
library = env.SharedLibrary(
"{0}/lib{1}.{2}.{3}.framework/{1}.{2}.{3}".format(
Copy link
Contributor

@Ivorforce Ivorforce Feb 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.framework is not required or useful on macOS, but is required on iOS.
But we're currently trying to figure out if .framework is even worth generating at all, because for iOS, the godot exporter upgrades .dylib to .framework anyway. This should probably be cleared up before merging this PR. I'm waiting for feedback from @bruvzg for this :)

@Ivorforce
Copy link
Contributor

I also have the general concern of cross-platform builds. Adding a GDExtension to your project will make you unable to export to other platforms 'out of the box'. I think this was discussed in the proposal (godotengine/godot-proposals#3367). I'm not sure a consensus was reached, but I think it warrants a clear path ahead before we decide to commit to GDExtension integration into a project.

@aaronfranke aaronfranke force-pushed the create-gdextension branch 2 times, most recently from 0de6fdb to 17de6f7 Compare February 26, 2025 00:14
@aaronfranke aaronfranke requested a review from bruvzg February 26, 2025 00:20
Copy link
Contributor

@Ivorforce Ivorforce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I've went through most of the code now. I've found some more stuff to nitpick! :D

I think we should wait for the .framework issue to be resolved before merging. I really hope we can just use .dylib on macOS and iOS, because it simplifies things.
So I'm clicking the 'request changes' button again to convey my intent on that. But other than that, i think it looks great! Good work 🙏


[libraries]

macos.debug = "bin/lib__LIBRARY_NAME__.macos.debug.framework"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There have recently been some changes to the godot-cpp-template version that I think should be applied here too. In particular, relative paths require a leading ./, and the binary itself should be linked instead of the .framework, because of an unfixed bug #96403.

(this is one of the reasons i'm trying to get rid of .framework in the first place, because then we could just use .dylib and have less trouble with this weird bug)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.