Allow detecting when building as an engine module

[aaronfranke]

Intended use case: I am working on a project that can be built as either an engine module or a GDExtension. I need to be able to detect whether I am building for an engine module or not in my code.

See also godotengine/godot-cpp#1339 - having at least one of these PRs is highly useful, but having both is even better, because it lets you explicitly error on invalid configurations:

#ifdef GDEXTENSION
#include <godot_cpp/classes/engine.hpp>
#elif defined(GODOT_MODULE)
#include "core/config/engine.h"
#else
#error "Must build as Godot GDExtension or Godot module."
#endif

Plus single statements can avoid negatives which makes them more readable, such as #ifdef GODOT_MODULE instead of #ifndef GODOT_GDEXTENSION or vice versa.

Bikeshed: Is module clear enough? We could also do GODOT_ENGINE_MODULE if that helps.

[bruvzg]

TextServer modules (which support building as both module and GDExtensions) use #ifdef GDEXTENSION for the same purpose. So if this change is merged, TextServer modules should be updated for consistency.

[YuriSizov]

TextServer modules (which support building as both module and GDExtensions) use #ifdef GDEXTENSION for the same purpose.

So what is the problem with this approach? Why do we need new defines?

[AThousandShips]

For the servers I'd keep the current setup, they should be expected to be modules unless explicitly specified

[aaronfranke]

@YuriSizov The TextServer modules define GDEXTENSION in their SConstruct file, which works, but this feature is extremely common and so it should be a built-in part of the engine and GDExtension. We already have 4+ examples of projects that need this: both TextServer modules, Zylann's Voxel module, and my projects.

[AThousandShips]

The two declarations are mutually exclusive and I'd lean to using one, preferably GDEXTENSION, having both could be confusing including assuming there's a third option

[bruvzg]

It's pretty much a style choice, but defining it in the engine might be better since it will guarantee it's the same for all modules.

[aaronfranke]

@AThousandShips The declarations are mutually exclusive in the sense that only one may appear at a time, but it is beneficial to include both as available options, because there is a third possible state (neither declaration exists).

[AThousandShips]

What situation are you neither building as a module or an extension?

I think there's possible confusion that's all 🙂, we don't define a REAL_T_IS_FLOAT for example

[aaronfranke]

What if I want to explicitly detect when the build system does not explicitly say if it's a module or GDExtension and throw an explicit error, instead of letting the build system fail with a cryptic error message?

What if I wanted to make a C++ library portable between engines? I have already done this in the past with a C# library in Unity and Godot. Having every target define something is useful.

[YuriSizov]

@AThousandShips I think the main point here would be explicitness. If you have both, you can explicitly write code that is only compiled when it's a module or code that is only compiled when it's an extension. Instead of it being "when it's a module" or "when it's not a module", if that makes sense.

#ifdef GDEXTENSION
// vs
#ifndef GODOT_MODULE

So I'd say it's just cleaner this way, but it is indeed a style choice.

[AThousandShips]

I agree, all I was saying was that it was in some ways redundant and could cause confusion 🙂

I wasn't saying "we can't have both", was just raising a point, I fully understand the idea and the benefits of it (and I repeated the mutual exclusive part before I noticed the edit)

[YuriSizov]

@YuriSizov The TextServer modules define GDEXTENSION in their SConstruct file, which works, but this feature is extremely common and so it should be a built-in part of the engine and GDExtension. We already have 4+ examples of projects that need this: both TextServer modules, Zylann's Voxel module, and my projects.

Okay then. I would suggest updating this PR to migrate TextServers to this new define and remove the adhoc one. Will serve as a good demo of how it should be used as well as keep us away from tech debt. Regarding the godot-cpp PR, I'll leave it to @dsnopek and @bruvzg to decide 🙂

[aaronfranke]

and remove the adhoc one.

We still want to have a define for GDExtension, so we should either 1) Update the PR I made on the godot-cpp side to use GDEXTENSION or 2) Replace the adhoc one with GODOT_GDEXTENSION. But we still need to bikeshed on that 🙃 In either case I think it would be best to merge the godot-cpp pull request first (either modified or as-is).

[ValorZard]

@aaronfranke Now that godotengine/godot-cpp#1339 is merged, will this PR be able to merged in as well now?

[aaronfranke]

@ValorZard Yes.

[dsnopek]

I think overall the GODOT_MODULE define makes sense to have.

I'm not sure we really need the #elif defined(GODOT_MODULE) changes in text_server_adv, but I'll leave that up to @bruvzg

[aaronfranke]

@dsnopek The TextServer changes are not strictly necessary but it does make the intention clearer so I'm inclined to keep it. Also it ensures the only times you will have a successful build are when the build system explicitly chooses GDExtension or module.

[akien-mga]

Thanks!