Skip to content

Override for debug_features in SCons and CMake #1737

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

Closed
wants to merge 1 commit into from

Conversation

enetheru
Copy link
Collaborator

@Naros raised that after the recent CMake changes his builds now have the DEBUG_ENABLED flag set.

This is due to using the godot-cpp build target being editor or debug_template.
The CMake scripts now match the SCons scripts, which was not previously the case.

In SCons there is an attribute debug_features which is dependent on the target being either template_debug or editor
this is then used to add DEBUG_ENABLED and DEBUG_METHODS_ENABLED to the CPPDEFINES.

This PR adds a new option to both SCons and CMake, debug_features and GODOTCPP_DEBUG_FEATURES respectively, which when unset uses the existing logic, and allows a user to explcitly enable or disable the debug_features.

This works exactly the same way as the use_hot_reload option.

@enetheru enetheru marked this pull request as ready for review March 10, 2025 13:55
@enetheru enetheru requested a review from a team as a code owner March 10, 2025 13:55
@enetheru enetheru added the topic:buildsystem Related to the buildsystem or CI setup label Mar 10, 2025
@dsnopek
Copy link
Collaborator

dsnopek commented Mar 13, 2025

We do have env.debug_features in SCons, but I don't think it was ever intended to be toggled separately. I think we want for DEBUG_ENABLED to be defined if you're building with a target of template_debug or editor. That would also match Godot's build process.

Why is desirable to not have DEBUG_ENABLED while doing a debug or editor build?

@enetheru
Copy link
Collaborator Author

Why is desirable to not have DEBUG_ENABLED while doing a debug or editor build?

I cant find the conversation @Naros and I were having, it might be somewhere in https://github.com/CraterCrash/godot-orchestrator/

From what I grasped, they were building a single release binary with EDITOR_TOOLS enabled, but DEBUG_ENABLED and DEBUG_METHODS_ENABLED disabled that could be used by game developers to build and ship with. And gave a more performant user experience.

Found the conversation, it was in their Discord under General, https://discord.com/channels/1049457206648655912/1150508215411413084

Quoting @Naros

Well keep in mind, at least for GDExtension libraries, they're always editor builds so enabling DEBUG features may not be best and could lead to performance drops, even in release builds.
For us, we don't have a separate library for editor vs game runtime.

@enetheru enetheru force-pushed the debug_features branch 2 times, most recently from 0bb02b2 to 5749ef6 Compare March 14, 2025 03:14
@dsnopek
Copy link
Collaborator

dsnopek commented Mar 14, 2025

Hm. I still feel like disabling DEBUG_ENABLED and DEBUG_METHODS_ENABLED means it's no longer a debug build, and doesn't really match up with what our editor or template_debug targets are. And if they want to make runtime performance better, that means making a template_release build, which is why we have the mechanism to provide multiple builds. Are they just avoiding that to have smaller downloads?

So, I'm still not totally convinced that we should expose this. But if it we do, I think it should be entirely internal. Like, the developer of a GDExtension that wants to do this non-standard thing can configure their extension to build this way, but it isn't an option we provide on the command-line.

Add debug_features/GODOTCPP_DEBUG_FEATURES as an option to override the conditional use of  DEBUG_ENABLED and DEBUG_METHODS_ENABLED
@enetheru
Copy link
Collaborator Author

So, I'm still not totally convinced that we should expose this. But if it we do, I think it should be entirely internal. Like, the developer of a GDExtension that wants to do this non-standard thing can configure their extension to build this way, but it isn't an option we provide on the command-line.

Perhaps something for the meeting? I was going to argue reasons why I think it might make sense, but realised that my thoughts were speculation at best. Like if someone wants t make an editor only plugin like PowerMode, do these flags effect performance in a detrimental way.

We need @Naros to weigh in since he was the catalyst for this change.

@Naros
Copy link
Contributor

Naros commented Mar 15, 2025

Are they just avoiding that to have smaller downloads?

Hi @dsnopek, we aren't necessarily trying to do something non-standard; perhaps it's just a misunderstanding on my part.

There have not been a lot of well-documented examples, particularly for those who create new scripting languages for Godot using GDExtension. So a lot of the work we've done has been primarily due to trial and error.

In our code, we use things like TOOLS_ENABLED to mimic behavior we see inside CSharp and GDScript. Those are only enabled in editor builds, as I understand. That means when Godot creates their builds, the editor target is specifically used when building the editor executables, the other two targets are for the runtime export templates.

So is it then expected for a GDExtension project that a plug-in that has editor UI integration and that is also used for runtime gameplay such as a scripting language subsystem that we should be shipping two binaries per platform, one for the editor and the other for the template_release (assuming debug isn't distributed, but used internally)?

I've done a lot of looking around at other script language projects online and that did not seem to be what they were doing, so perhaps that the root cause for this confusion and we are simply not doing what is expected.

Can you confirm?

@dsnopek
Copy link
Collaborator

dsnopek commented Mar 17, 2025

So is it then expected for a GDExtension project that a plug-in that has editor UI integration and that is also used for runtime gameplay such as a scripting language subsystem that we should be shipping two binaries per platform, one for the editor and the other for the template_release

Yes.

There are the three targets, so you could make three builds (for editor, template_debug and template_release), but what most GDExtensions do is include the editor stuff in a build for both editor and template_debug. The editor will load entries from the .gdextension file with the debug feature tag, so this will usually look like the .gdextension file only includes a debug and release build, but the editor stuff is in the one tagged with debug.

If you had really a lot of editor-only code, and wanted for it not to be in template_debug then you could make all three builds, but it's frequently not worth it.

I've done a lot of looking around at other script language projects online and that did not seem to be what they were doing, so perhaps that the root cause for this confusion and we are simply not doing what is expected.

For an example, here's the .gdextension file for the Luau extension which provides a scripting language:

https://github.com/Fumohouse/godot-luau-script/blob/master/bin/luau_script.gdextension

Notice how it's using, for example, libluau-script.linux.editor.x86_64.so (so the editor target) for the debug feature tag.

@enetheru
Copy link
Collaborator Author

@Naros is this wanted anymore? if not i can close it.

@dsnopek
Copy link
Collaborator

dsnopek commented Apr 15, 2025

Let's close this one for now. We can re-open later if necessary

@dsnopek dsnopek closed this Apr 15, 2025
@Naros
Copy link
Contributor

Naros commented Apr 15, 2025

Hi @enetheru I don't think so. With the TOOLS_ENABLED toggled for editor/debug builds, which I think you fixed in another PR, that should be sufficient.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic:buildsystem Related to the buildsystem or CI setup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants