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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add custom_modules build option to compile external, user-defined C++ modules #36922

Merged
merged 1 commit into from
May 25, 2020
Merged

Add custom_modules build option to compile external, user-defined C++ modules #36922

merged 1 commit into from
May 25, 2020

Conversation

Xrayez
Copy link
Contributor

@Xrayez Xrayez commented Mar 8, 2020

Closes #31457, namely as suggested by @fire in #31457 (comment), also @willnationsdev.
Closes godotengine/godot-proposals#565.

Description

This PR adds ability to include external, user-defined C++ modules to be compiled as part of Godot:

scons platform=x11 tools=yes target=release_debug custom_modules="../project/modules"

Feature-wise this PR is similar to #36883 in a sense that both provide a command line option to compile something externally, with the following differences:

  • only minimal core changes related to doctool bug, but mostly only build scripts are modified.
  • doesn't introduce any new concepts, but makes existing build process more flexible for module development.
  • can detect multiple modules at custom_modules, while Added game module compilation argument聽#36883 aims to provide a way to compile a single C++ package (which can compile other modules though).
  • full support for per-module custom documentation and editor icons.

One could just create game module and do this:

src:
  - godot:
        - modules
  - project: # contains `project.godot`
        - modules:
            - game # regular C++ module
            .gdignore

Then run this command from within godot directory:

scons platform=x11 tools=yes target=release_debug custom_modules=../project/modules

with the added benefit of having ability to include any other module alongside.

The register types would look like this for a game module:

#include "register_types.h"
#include "game.h"

void preregister_game_types() {

}
void register_game_types() {

}
void unregister_game_types() {

}

Pretty intuitive isn't it? 馃檪

Features

  • Can detect all available custom modules under custom_modules directory the same way as it does for built-in modules (not recursive).
  • Multiple search paths can be specified similarly to PATH env var.
  • Works with both relative and absolute paths on your filesystem.

Benefits

  • No need to manually copy module files directly to godot/modules, just point the path where your custom modules reside in your project.
  • No need to symlink modules either to simulate the same behavior (I can finally remove all symlink hacks in my own project, which sometimes don't work on all platforms reliably).
  • No need to fork Godot, makes it user-friendly for those using Godot+modules to be versioned with git as submodules, as C++ modules can be independent from Godot repository.

Implementation details

See commit description.

Suggestions

Option name is subject to discussion, feedback welcome.

@Xrayez
Copy link
Contributor Author

Xrayez commented Mar 9, 2020

Managed to resolve remaining issues with collecting and generating custom documentation for external modules. I had to modify the doctool to distinguish between relative and absolute paths respectively.

Similarly fixed modules editor icons collection by editor icon builder.

All-in-all, this makes it work exactly like built-in modules, making this PR feature-complete IMO.

馃 馃

@AndreaCatania
Copy link
Contributor

This version is much better then the one that I did. Really good Job!

@fire
Copy link
Member

fire commented Mar 9, 2020

The flag modules_search_path is a bit long and wordy. Any suggestions?

I'm looking at the other examples like target or bits or platform.

@Zireael07
Copy link
Contributor

--module(s)_path? (We can drop the s to shorten it even further?)

@fire
Copy link
Member

fire commented Mar 9, 2020

There is only one path to a folder of modules. So module_path=~/Documents/game_project/modules/ sounds great.

@Xrayez
Copy link
Contributor Author

Xrayez commented Mar 9, 2020

--module(s)_path? (We can drop the s to shorten it even further?)

Multiple modules can be detected at provided path, so the option should suggest that, else it may lead to confusion that only a single module can be linked, which is not the case here.

There is only one path to a folder of modules

See above. If the option supported multiple paths, the name would be module_paths otherwise. I'd personally go for modules_path, or even modules_dir.

Some more suggestions:

We could go further and introduce either package or framework keywords for that. 馃槢

Especially if the option could support multiple search paths. You could combine several module packages residing at different locations, but that's too much to ask I guess.

@Xrayez Xrayez changed the title Add modules_search_path build option to compile external C++ modules Add custom_modules build option to compile external C++ modules Mar 9, 2020
@Xrayez
Copy link
Contributor Author

Xrayez commented Mar 9, 2020

Renamed modules_search_path to custom_modules to reduce verbosity. The rationale behind this:

  • the name encodes necessary information that multiple modules can be detected (plural form), the name is also visually similar to suggest that's C++ modules specifically, but custom.
  • the help page is descriptive and the converter is smart enough to suggest you that directory path should be fed to detect a list of custom modules in case of an invalid input.
  • "Custom modules" comes from documentation, it can be added later to documentation page without any friction with existing terminology, and familiar to most Godot'ers using modules.

I've also added an explicit check to prevent passing a path to a single module (parent directory should be used instead in that case).

@Xrayez
Copy link
Contributor Author

Xrayez commented Mar 10, 2020

I've took some time to test this further.

Fixed an issue with path includes. Namely when you include modules as dependencies of other modules:

#ifndef YOUR_MODULE_H
#define YOUR_MODULE_H

#include "modules/my_module/my_class.h"

// ...

Now, scons treats the custom_modules's parent directory as if it was Godot's base directory, so I advice putting your custom modules under modules directory (even if it's outside Godot) similarly to built-in modules (if you have module dependencies like this) so that include paths remain the same. But you can name it whatever you like of course, depending on whether you need to interact with built-in modules from within custom modules, and vice versa.

@Xrayez
Copy link
Contributor Author

Xrayez commented Mar 10, 2020

To test this with existing project, I've had to port this PR to 3.2, so to whoever want to test this against 3.2, here's the commit: https://github.com/Xrayez/godot/commit/e237bf51f58e388a639e821927058ce1cabce93d.

Also providing a raw link to 3.2 patch, and some commands:

Linux (not tested)

curl https://github.com/Xrayez/godot/commit/e237bf51f58e388a639e821927058ce1cabce93d.patch | git apply -v

Windows

curl https://github.com/Xrayez/godot/commit/e237bf51f58e388a639e821927058ce1cabce93d.patch | Select-Object -Expand Content | git apply -v

@Xrayez
Copy link
Contributor Author

Xrayez commented Mar 12, 2020

Here's an astonishing way to test this PR:

  1. Move all (or some) built-in modules to whatever place on your filesystem under modules directory (except for gdnative module, it seems like it has core dependencies). Note: don't move an entire godot/modules directory, just individual modules.

  2. Build. I'm using MSVC so it looks like this on my side:

scons platform=windows tools=yes target=release_debug bits=64 custom_modules="D:/tmp/modules/"

godot-builtin-modules-as-custom

Compilation successful. Classes, editor icons and documentation are still there!

Notice absolute paths when compiling modules, that's the way scons knows about what modules are external.

@Xrayez
Copy link
Contributor Author

Xrayez commented Mar 16, 2020

Added ability to specify multiple search paths for custom modules, not much changes internally.

scons platform=windows custom_modules="$env:USERPROFILE/godot/modules;../project/modules"

Notice that paths must be separated either by ; or : (Windows/Unix respectively).

That way, you can define your C++ packages as a collection of modules, so to speak.

I haven't added support for checking GODOT_CUSTOM_MODULES_PATH environment variable or alike. This can be achieved by exporting/extending SCONSFLAGS instead:

Unix:

export SCONSFLAGS="custom_modules=../project/modules"

Windows:

$env:SCONSFLAGS = "custom_modules=../project/modules"

Also added a note that custom modules can override built-in ones at build-time. For instance, it's perfectly possible to maintain your own gdscript module within your project, and it will always shadow the built-in one. You can use this to allow something akin to local module fork, so to speak.

@Xrayez Xrayez changed the title Add custom_modules build option to compile external C++ modules Add custom_modules build option to compile external, user-defined C++ modules Mar 17, 2020
@AndreaCatania
Copy link
Contributor

@Xrayez I'm using this PR for some time now and it works really well. With this I'm not anymore constrained to have a specific godot repository per each proejct. Indeed, I just need to update my main godot source code once to be able to use it with all projects.

Also, the various compilations takes much much less time since the 90% of the code is already compiled.

But the most important thing is that now it's really simple handles the shared modules and I don't need anymore perform cherry-picks.

I didn't used docs and icons yet, but for me even without those two features is a really good job. Thanks for this contribution!

@realkotob
Copy link
Contributor

realkotob commented Mar 27, 2020

The code looks fine to me, the changes are straightforward and not invasive.

I would love for this to be merged soon, looking forward to it!

@fire
Copy link
Member

fire commented Apr 4, 2020

Is your 3.2 branch still updated?

@Xrayez
Copy link
Contributor Author

Xrayez commented Apr 4, 2020

@fire I've updated modules-search-path-3.2 branch with recent changes to this PR just now: https://github.com/Xrayez/godot/commit/22c0adc4afd5a6bf18e61d4ffe306e1ebd1e15e4 namely: #36922 (comment).

I don't think there's anything left to be added to this PR feature-wise. Depending on my own needs, I might keep updating the 3.2 version of this patch as well, but I hope that there won't be any merge conflicts to be resolved on the 3.2-stable for that matter.

@AndreaCatania
Copy link
Contributor

Any idea when this can be merged or what is needed to start the process to merge this?

@ghost
Copy link

ghost commented May 6, 2020

Any idea when this can be merged or what is needed to start the process to merge this?

i don't think c++ is getting enough love :(

@Calinou
Copy link
Member

Calinou commented May 6, 2020

@RaTcHeT302 If C++ wasn't getting any love, then nobody would be contributing to Godot 馃槈

It's just that pull request reviews are currently slowed down because the master branch is undergoing refactoring.

@ghost
Copy link

ghost commented May 8, 2020

@RaTcHeT302 If C++ wasn't getting any love, then nobody would be contributing to Godot 馃槈

It's just that pull request reviews are currently slowed down because the master branch is undergoing refactoring.

i mean i would help if i knew how, but i'm not really familiar with the engine yet, and i haven't had the time to figure things out - my end goals would clash with godot either way, and i have a weird style, where i tend to write comments everywhere, as i tend to forget everything i've learned if i stop looking at the same thing for a month or two

i find myself relying on comments a lot, otherwise most source code ends up looking like complete gibberish to me as a whole, so i tend to waste a lot of time trying to dissect things and to figure out how everything actually works - i don't think i would be of much help with godot, considering how long it would take me to learn how to do anything properly

i know what i want from the engine as a whole, but i have no idea as to how to actually implement anything in it yet, i think if i tried to, it would all be very hacky \ half assed

i really hope to see these make it in though, i don't have the dexterity to learn both gd script and c++ at the same time, i'd rather deal with the quirks of c++ and focus on that

godotengine/godot-proposals#565
godotengine/godot-proposals#573

@fire
Copy link
Member

fire commented May 10, 2020

Can you add the custom_modules flag to vsproj for Windows Visual Studio?

@Xrayez
Copy link
Contributor Author

Xrayez commented May 10, 2020

@fire tbh I rarely use Visual Studio IDE (if at all), mostly VSCode, so not sure what needs to be made in order for this to work. I've read through the Sconstruct vsproj generation and there are some commands which can be modified I guess:

godot/methods.py

Lines 504 to 515 in 6a0473b

env["MSVSBUILDCOM"] = build_commandline(
"scons --directory=\"$(ProjectDir.TrimEnd('\\'))\" platform=windows progress=no target=$(Configuration) tools=!tools! -j"
+ str(num_jobs)
)
env["MSVSREBUILDCOM"] = build_commandline(
"scons --directory=\"$(ProjectDir.TrimEnd('\\'))\" platform=windows progress=no target=$(Configuration) tools=!tools! vsproj=yes -j"
+ str(num_jobs)
)
env["MSVSCLEANCOM"] = build_commandline(
"scons --directory=\"$(ProjectDir.TrimEnd('\\'))\" --clean platform=windows progress=no target=$(Configuration) tools=!tools! -j"
+ str(num_jobs)
)

I suspect the !option! syntax is there to denote user-defined vars or so (?).

In any case this can be added as part of another PR I guess, lets review this one first. 馃檪

SConstruct Outdated Show resolved Hide resolved
@Xrayez
Copy link
Contributor Author

Xrayez commented May 24, 2020

I want to report that this also works from within WSL with VSCode integration, I've just compiled scons platform=javascript custom_modules=../project/modules with no issues (the absolute paths resolve to something like /mnt/c/src/project/modules/...).

@akien-mga akien-mga added this to the 4.0 milestone May 25, 2020
@akien-mga
Copy link
Member

akien-mga commented May 25, 2020

Thanks for bearing with me, I'm now ready to review this :)

I can already say 馃憤 for the proposal/use case, I'll review the implementation more in-depth but a priori this should be good to integrate in the engine. Once merged in master, I'm also fine having this cherry-picked for 3.2 (ideally via a dedicated PR as the black formatting in master will make cherry-picking bothersome).

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Some nitpicks, but looks great overall.

SConstruct Outdated Show resolved Hide resolved
SConstruct Outdated Show resolved Hide resolved
SConstruct Outdated Show resolved Hide resolved
SConstruct Show resolved Hide resolved
This patch adds ability to include external, user-defined C++ modules
to be compiled as part of Godot via `custom_modules` build option
which can be passed to `scons`.

```
scons platform=x11 tools=yes custom_modules="../project/modules"
```

Features:

- detects all available modules under `custom_modules` directory the
same way as it does for built-in modules (not recursive);
- works with both relative and absolute paths on the filesystem;
- multiple search paths can be specified as a comma-separated list.

Module custom documentation and editor icons collection and generation
process is adapted to work with absolute paths needed by such modules.

Also fixed doctool bug mixing absolute and relative paths respectively.

Implementation details:

- `env.module_list` is a dictionary now, which holds both module name as
  key and either a relative or absolute path to a module as a value.
- `methods.detect_modules` is run twice: once for built-in modules, and
  second for external modules, all combined later.
- `methods.detect_modules` was not doing what it says on the tin. It is
  split into `detect_modules` which collects a list of available modules
  and `write_modules` which generates `register_types` sources for each.
- whether a module is built-in or external is distinguished by relative
  or absolute paths respectively. `custom_modules` scons converter
  ensures that the path is absolute even if relative path is supplied,
  including expanding user paths and symbolic links.
- treats the parent directory as if it was Godot's base directory, so
  that there's no need to change include paths in cases where custom
  modules are included as dependencies in other modules.
SConstruct Show resolved Hide resolved
SConstruct Show resolved Hide resolved
@Xrayez
Copy link
Contributor Author

Xrayez commented May 25, 2020

All in all I think this is ready to merge, I've just left some notes for some future changes if some more advanced features are needed.

@akien-mga akien-mga merged commit 78f554a into godotengine:master May 25, 2020
@akien-mga
Copy link
Member

Thanks!

@Xrayez
Copy link
Contributor Author

Xrayez commented May 25, 2020

Once merged in master, I'm also fine having this cherry-picked for 3.2

Thanks, I'll work on the 3.2 version now then!

@Xrayez Xrayez deleted the modules-search-path branch May 25, 2020 15:03
Calinou added a commit to Calinou/godot-builds-ci that referenced this pull request Jun 11, 2020
Calinou added a commit to Calinou/godot-builds-ci that referenced this pull request Jun 11, 2020
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.

C++ Game module Allow putting modules that compile into the engine in a project's folder
7 participants