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

GDScript: Implement @if_features annotation for feature-based code stripping #102083

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

RandomShaper
Copy link
Member

@RandomShaper RandomShaper commented Jan 27, 2025

After checking some of the proposals on adding some preprocessing abilities to GDScript, and aware of certain real-world needs on this, I started trying to implement something that would be as unintrusive as possible. The result is this PR.

What I'm adding here is a @if_features annotation that works this way:

  • It can only be applied to function members of classes, either the main class in a source file or an inner class.
  • It accepts zero or more string arguments. They are meant to correspond to features declared by platforms and export presets.
  • Users have to apply the annotation to each function in a group of equally-named functions, knowing that only the one matching will be taken into account at runtime.
  • If multiple features are stated, they are treated with an implicit and.
  • The annotation having zero arguments means the function annotated is the default one, the one to be picked in case none of the others with the same name fits.
  • From a set of equally-named functions using the annotation, only the first one fitting is taken into account, unless it's a default one, which can be overtaken by the first one with one or more features that comes later.
  • At script edit time, the annotation is only honored to make it possible to have multiple equally-named functions.
  • At runtime from an editor build, the annotation is resolved at runtime, against the features advertised at the moment.
  • At export time, the script is preprocessed to effectively strip out the functions whose target features don't fit what the target platform and export preset declare. (Stripping is performed by replacing the lines corresponding to the unmatching functions' annotations, declaration and body, with blank lines.)

Example (more in the test case included in the commit):

extends Node2D

func _ready():
	_implementation()

@if_features() # No features means this is the default. Used in case no other fits.
func _implementation():
	print("Other OS...")

@if_features("windows")
func _implementation():
	print("Windows!")

@if_features("linuxbsd", "s3tc")
func _implementation():
	print("Linuxxx with S3TC texture compression!")

@if_features("linuxbsd", "astc")
func _implementation():
	print("Linuxxx with ASTC texture compression!")

UPDATE:
A runtime feature is added, which is advertised when the script is run in the game, regardless it's an editor build or not, but not if run on the editor. See this comment.

Example:

@tool
extends Node

func _ready():
	do_some_setup_needed_for_editor_and_runtime()

@if_features("runtime")
func _process(time):
	run_the_gameplay_part()

Additional work for the future:

  • Support the annotation on other items, not only functions.
  • Add tests of the export-time stripping.
  • Support the annotation in other languages (i.e., C#). UPDATE: Other languages may not really need this.
    - Support tool scripts. UPDATE: A tool script can just perform regular runtime feature checks.

Implements, kind of, godotengine/godot-proposals#4234.

@RandomShaper RandomShaper added this to the 4.x milestone Jan 27, 2025
@RandomShaper RandomShaper requested review from a team as code owners January 27, 2025 10:29
@RandomShaper RandomShaper force-pushed the gdscript_if_features branch 2 times, most recently from 0c53ccb to 7f29eb5 Compare January 27, 2025 10:52
@RandomShaper RandomShaper requested a review from a team as a code owner January 27, 2025 10:52
@Lazy-Rabbit-2001
Copy link
Contributor

Is this a friendly alternative of #ifdef for gds? I think this would be cool if it allows some special defines about whether it is running in the editor or in the debugger or inthe release build.

@RandomShaper
Copy link
Member Author

Is this a friendly alternative of #ifdef for gds? I think this would be cool if it allows some special defines about whether it is running in the editor or in the debugger or inthe release build.

For editor builds, you have editor_hint an editor_runtime. For template builds, you have template, debug, release, template_debug, template_release.

Tool scripts don't play with this yet. So, editor can only mean you're running the project from an editor binary, without the ability of using editor_hint to tell if you're actually running within the editor, at the moment. However, for those cases, you can just use editor and make the other checks within that function, at runtime.

@clayjohn
Copy link
Member

I think this looks really cool and seems to resolve a lot of issues for people that are shipping on multiple platforms that may not have open APIs.

I would be interested in hearing some feedback from people like @allenwp who has experience shipping games on multiple platforms

@adamscott
Copy link
Member

adamscott commented Jan 27, 2025

I would suggest to add @if_feature() as an alias to @if_features(), as it looks weird when only one feature is in the list.

Also I'm not so sure about this. This looks and feel tacked upon.

@if_features() # No features means this is the default. Used in case no other fits.

I suggest to create a new annotation @if_features_default or something.

Mmm. Thinking of it, I think a preprocessor should be preferred here, as it could be really easier to do if/else.

@KoBeWi
Copy link
Member

KoBeWi commented Jan 27, 2025

Does it support custom feature tags too?

@adamscott
Copy link
Member

@RandomShaper Is the plugin from @dalexeev could solve your issues without this PR?
https://github.com/dalexeev/gdscript-preprocessor

@allenwp
Copy link
Contributor

allenwp commented Jan 27, 2025

I would be interested in hearing some feedback from people like @allenwp who has experience shipping games on multiple platforms

Thanks for the ping!

At export time, the script is preprocessed to effectively strip out the functions whose target features don't fit what the target platform and export preset declare.

From looking only at the syntax, this wasn't entirely obvious to me. Not saying that's a bad thing, but if this is also the case for most other users, it might be a relevant point.

Because runtime evaluation based on feature tags and script remapping at export time already exist, I believe the core value of this feature is the stripping functionality.

This comment from KoBeWi describes exactly how I've used #if preprocessing in C# classes in Unity when using different SDK and modules.

Because of this, it would be nice to have some way to strip code inline so things don't need to be separated into function calls. In an extreme case, I could see a bit of an anti-pattern emerging where logic needs to be duplicated between a function for one feature tag and another function for a different feature tag; these functions would need to be separate because of an SDK call, but the SDK call might be only a small part of the function.

I will be honest that I haven't gone through the process of making a solution to this in Godot with GDScript, so maybe the SDK integration isn't as big of an issue as it is with C# in Unity... But after reading the comment that I linked, it sounds like the challenge in Godot is the same as C# Unity.

@Lazy-Rabbit-2001
Copy link
Contributor

btw, will there be @if_no_features() as the opposite version of the annotation, when there are few features that should be filtered out and the corresponding code should not be compiled but you have to list the rest long string of features, which would take more time and increase the complicatability?

@RandomShaper
Copy link
Member Author

Does it support custom feature tags too?

Yes.

@RandomShaper Is the plugin from @dalexeev could solve your issues without this PR? https://github.com/dalexeev/gdscript-preprocessor

Looks like it would.

As said in the description of the PR, this is made so it's as concise and unintrusive to the codebase as possible. While doing this, I bore in mind both the theoretical design and practical implementation sides of the question. A full preprocessor, extra features, etc. would be a bad tradeoff if we keep that goal in mind. To me, if this solves real-world use cases, it'd be already good, even if we consider it experimental for now.

@allenwp
Copy link
Contributor

allenwp commented Jan 28, 2025

This is the primary use-case I see for this PR. Please let me know if I'm incorrect in my understanding...

Project has Android SDKs (VR packages, ad SDKs, etc.). These SDKs have GDScript addon files in the project that should not be included in other exports of the project and are thus excluded using the existing Export/Resources settings.

This PR adds the following: Any user code that interfaces with the Android SDK scripts would be within these @if_features("android") annotations.

When running in the editor, all scripts are included, so scripts can be parsed without error.

When exporting to PC, android-specific script elements are stripped. This allows the scripts to be parsed without issue.

Without this PR, this would not be as easy to do because scripts would have parsing issues due to Android SDK scripts not being included in the PC exports.

Am I understanding how this would be used?

@RandomShaper
Copy link
Member Author

@allenwp, you nailed it.

Elaborating, while it's true that one could still use this for features that can be checked at runtime, that'd be an antipattern. The idea is exactly what you describe: to prevent parse errors in exported projects not including certain symbols while still being able to enjoy full capabilities while at edit time (autocomplete, static typing).

@JoNax97
Copy link
Contributor

JoNax97 commented Jan 29, 2025

Would it be feasible to have this annotation work for any given scope, including whole classes and arbitrarily indentations inside a function?

That way you retain the simplicity of a single annotation, without needing a closing statement like #endif

It would also allow for stripping individual parts of methods, just by indenting them one level and addin the annotation.

@RandomShaper
Copy link
Member Author

@JoNax97, while that's technically possible, it would involve much heavier changes and put a bigger maintenance burden on all this. I chose to limit it to functions because it looked as a sweet spot where the implementation was not too complex while the flexibility was big enough.

@RandomShaper
Copy link
Member Author

Quoting @vnen on RocketChat:

My main concern with this solution is type checking, since you can have multiple functions with the same name but there's no guarantee they'll have the same signature. In the editor you'll only be testing one, so the others might be broken and you don't realize, which might cause problems or even crashes after exporting. Worse: you might have some subtle conversion issue that's hard to notice and to find.
Of course, having different functions is in the nature of the feature, but the static analyzer should still be able to catch problems in all code paths.

Because of that, I've updated the PR so all the functions with the same name must have the exact same signature; otherwise, an error happens:
image

@eon-s
Copy link
Contributor

eon-s commented Jan 30, 2025

Sorry if I missed a reply @RandomShaper but editor or tool_only or better skip_editor could help to avoid running game logic in the editor or editor tool logic in a game, the editor_hint is too error prone, unlike disabling the whole method (particularly with engine callbacks)
That's a feature that was requested and needed for a long time and may fit on this one.

@RandomShaper
Copy link
Member Author

Sorry if I missed a reply @RandomShaper but editor or tool_only or better skip_editor could help to avoid running game logic in the editor or editor tool logic in a game, the editor_hint is too error prone, unlike disabling the whole method (particularly with engine callbacks) That's a feature that was requested and needed for a long time and may fit on this one.

Remember this is basically meant for stripping out code that shouldn't even exist in exported games.

On an editor build you can still use OS.has_feature() regardless in-editor or in a running game. editor_hint will only be advertised if running on the editor. I'm curious about what sort of issue you are having with it.

@eon-s
Copy link
Contributor

eon-s commented Jan 31, 2025

On an editor build you can still use OS.has_feature() regardless in-editor or in a running game. editor_hint will only be advertised if running on the editor. I'm curious about what sort of issue you are having with it.

Is an old UX issue with tool scripts, you can't simply null a _process call or other functions with game logic in a tool script, you need to remember to include a early returns, an annotation is more visible, the scope is more clear.

So using this kind of annotation to allow/disable functions in the editor helps a lot.

@RandomShaper
Copy link
Member Author

Is an old UX issue with tool scripts, you can't simply null a _process call or other functions with game logic in a tool script, you need to remember to include a early returns, an annotation is more visible, the scope is more clear.

So using this kind of annotation to allow/disable functions in the editor helps a lot.

Sounds indeed like a good use of this annotation, which makes me think I should make it work when tool scripts are run as well. I'll give it a go.

@RandomShaper RandomShaper requested a review from a team as a code owner February 3, 2025 13:19
@RandomShaper
Copy link
Member Author

@eon-s, check the updated description. 😁

@@ -441,12 +441,15 @@ bool OS::has_feature(const String &p_feature) {
}
if (p_feature == "editor_hint") {
return _in_editor;
} else if (p_feature == "editor_runtime") {
} else if (p_feature == "editor_runtime" || p_feature == "runtime") {
Copy link
Contributor

@MarianoGnu MarianoGnu Feb 3, 2025

Choose a reason for hiding this comment

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

@RandomShaper i think this comparison is breaking this usecase:

@if_feature("editor_runtime")
func _process(delta: float) -> void:
>   # editor only process

@if_feature("runtime")
func _process(delta: float) -> void:
>   # runtime only process

Copy link
Member Author

Choose a reason for hiding this comment

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

editor_runtime means the game being run from a tools-enabled build.

For editor-only process, you'd use editor_hint.

Copy link
Contributor

@MarianoGnu MarianoGnu Feb 6, 2025

Choose a reason for hiding this comment

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

i see, thanks for clarification :)

@eon-s
Copy link
Contributor

eon-s commented Feb 3, 2025

I was testing the artifact a bit and the new design options created by features "editor_hint", "editor_runtime" and "runtime" is huge!
More clean code, allows tool code and extra code for isolated scene tests with no risky remnants on builds.

Thanks for considering it @RandomShaper !

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.

10 participants