-
-
Notifications
You must be signed in to change notification settings - Fork 21.6k
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
base: master
Are you sure you want to change the base?
GDScript: Implement @if_features
annotation for feature-based code stripping
#102083
Conversation
0c53ccb
to
7f29eb5
Compare
7f29eb5
to
9c1483a
Compare
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 Tool scripts don't play with this yet. So, |
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 |
I would suggest to add Also I'm not so sure about this. This looks and feel tacked upon.
I suggest to create a new annotation Mmm. Thinking of it, I think a preprocessor should be preferred here, as it could be really easier to do if/else. |
Does it support custom feature tags too? |
@RandomShaper Is the plugin from @dalexeev could solve your issues without this PR? |
Thanks for the ping!
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 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. |
btw, will there be |
Yes.
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. |
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 When running in the editor, all scripts are included, so scripts can be parsed without error. When exporting to PC, 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? |
9c1483a
to
cba88ba
Compare
@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). |
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. |
@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. |
cba88ba
to
e7c0baa
Compare
Quoting @vnen on RocketChat:
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: |
Sorry if I missed a reply @RandomShaper but |
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 |
Is an old UX issue with tool scripts, you can't simply null a 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. |
e7c0baa
to
e176bda
Compare
e176bda
to
1e4ee3d
Compare
@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") { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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 :)
I was testing the artifact a bit and the new design options created by features Thanks for considering it @RandomShaper ! |
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:Example (more in the test case included in the commit):
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:
Additional work for the future:
- Support tool scripts. UPDATE: A tool script can just perform regular runtime feature checks.Implements, kind of, godotengine/godot-proposals#4234.