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 detecting when building as a GDExtension #1339

Merged
merged 1 commit into from
Dec 29, 2023

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Dec 17, 2023

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 GDExtension or not in my code, like this:

#ifdef GDEXTENSION
#include <godot_cpp/core/class_db.hpp>
#include <godot_cpp/variant/string.hpp>
#elif defined(GODOT_MODULE)
#include "core/object/class_db.h"
#include "core/string/ustring.h"
#else
#error "Must build as Godot GDExtension or Godot module."
#endif

See also godotengine/godot#86269 - 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.

Without this PR, I have to put this in my own SConstruct file. This works fine... but this would be generally useful for lots of projects, and is super tiny, so I think we should make it a part of godot-cpp.

@aaronfranke aaronfranke added enhancement This is an enhancement on the current functionality topic:gdextension This relates to the new Godot 4 extension implementation labels Dec 17, 2023
@aaronfranke aaronfranke requested a review from a team as a code owner December 17, 2023 18:07
@aaronfranke aaronfranke added this to the 4.3 milestone Dec 17, 2023
@AThousandShips
Copy link
Member

Should it not just be GDEXTENSION? It's not prefixed to other options like precision, and the text server uses that naming so should be uniform

@aaronfranke
Copy link
Member Author

aaronfranke commented Dec 17, 2023

@AThousandShips I can change it if desired, but I think that we should have the GODOT_ prefix in this case.

In Godot 3.x with GDNative we had most things prefixed with GODOT_. For Godot 4.x we removed nearly all of the prefixes for the sake of making things consistent with engine module development, which I think was a good move. However, this feature in particular is exclusive to GDExtension, so to me it makes sense to be explicit here.

Also, on the flip side, we may want to add a GODOT_MODULE define to the engine itself, so somebody could theoretically make a library that is compatible with both Godot, Unreal, and more like #ifdef GODOT_GDEXTENSION ... #elif defined(GODOT_MODULE) ... #elif defined(UNREAL_SOMETHING) ... #else #error "Unsupported engine" #endif. In this case, GODOT_GDEXTENSION would be consistent with a hypothetical GODOT_MODULE (and we shouldn't use MODULE by itself, that's way way too generic).

Also, I say hypothetical, but this is pain I've already went through in the past trying to make a C# project compatible with both Unity and Godot.

Also, the Godot C# bindings use flags like GODOT_REAL_T_IS_DOUBLE.

Also, Zylann is using ZN_GODOT_EXTENSION in his Voxel module.

@AThousandShips
Copy link
Member

AThousandShips commented Dec 17, 2023

Also, the Godot C# bindings use flags like

Indeed but godot-cpp does not, I think it should be uniform, as both can be used in extensions

I'd say that the use in text_server_adv should be a reference here, it's an extension available in the main repo so a point of reference of sorts

@dsnopek
Copy link
Collaborator

dsnopek commented Dec 18, 2023

I think adding a standard define for this in godot-cpp is a great idea! I'm not crazy about the name GODOT_GDEXTENSION in particular, though.

The text_server_adv module is using GDEXTENSION, which I think would be acceptable. GDExtension is already a Godot-specific technology, so I don't think we really need the GODOT_* prefix. I suppose it's theoretically possible that something else unrelated could be called GDExtension and create a conflict when trying to make a Godot GDExtension that integrates with it, but that seems fairly unlikely.

In one of my extensions, I'm using USE_GDEXTENSION, which I liked because it kind of fit in with other defines, although, thinking about it now, most of the other USE_* defines are for things that are scons arguments and this wouldn't be one of those.

@aaronfranke
Copy link
Member Author

Updated to use GDEXTENSION instead of GODOT_GDEXTENSION

Copy link
Collaborator

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

This seems good to me!

Probably best to wait on merging this for a bit to see if anyone else has feedback, though.

@Zylann
Copy link
Collaborator

Zylann commented Dec 18, 2023

Would it make sense to have this define in a "top-most" header (such as defs.hpp) rather than build scripts? Because that change requires to use SCons in both cases, and GodotCpp users can actually use a different build system. Even though we could think a project supporting both would use SCons, separate build scripts have to be written anyways.

Edit: nevermind, forgot that anything to include doesnt even match^^" I worked too long with my unified header system

@aaronfranke
Copy link
Member Author

aaronfranke commented Dec 18, 2023

@Zylann No, because you need to check for the define to decide which headers you are including.

#ifdef GDEXTENSION
#include "defs.hpp" // Too late!
#endif // GDEXTENSION

If you want to also make your own unconditionally-included top-level header with #define GDEXTENSION, you can do that, but then that header will break building as a Godot module, so it's not something you should commit to version control. Also note that such a header would not conflict with SCons defining it if guarded - they could theoretically co-exist fine.

@dsnopek
Copy link
Collaborator

dsnopek commented Dec 29, 2023

I think probably enough time has passed! Let's merge this :-)

Thanks for the contribution!

@dsnopek dsnopek merged commit 3f44e9b into godotengine:master Dec 29, 2023
12 checks passed
@aaronfranke aaronfranke deleted the detect-gdext branch December 29, 2023 23:17
@dsnopek
Copy link
Collaborator

dsnopek commented Jan 22, 2024

Cherry-picking to 4.2 in #1372

@dsnopek
Copy link
Collaborator

dsnopek commented Jan 22, 2024

Cherry-picking to 4.1 in #1373

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This is an enhancement on the current functionality topic:gdextension This relates to the new Godot 4 extension implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants