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

Add Preprocessors to GDScript #80619

Draft
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

nonunknown
Copy link
Contributor

@nonunknown nonunknown commented Aug 14, 2023

implements godotengine/godot-proposals#7496

Important:

The system doesnt affect any of gdscript internal code, basically its done by removing non used features before parsing source_code in GDScriptParser::parse

Ps: Its barebones, but should be done in time for next gdscript meeting

@dalexeev
Copy link
Member

  1. Note that the preprocessor must respect comments and string literals. I would advise you to do this in the tokenizer (modify the scan() method), rather than using line-by-line reading and regular expressions.
  2. Note that the main point of the preprocessor is to get different code for different export targets (based on different feature tags for different platforms/presets). Therefore, you will have to interact with the Godot export, for example via EditorExportPlugin. Now Godot 4.x does not support exporting compiled bytecode, only source code.

@nonunknown nonunknown force-pushed the add-preprocessors branch 2 times, most recently from a9d06d2 to 0a229f9 Compare August 18, 2023 15:20
core/os/os.h Outdated Show resolved Hide resolved
core/os/os.h Outdated Show resolved Hide resolved
modules/gdscript/gdscript_preprocessor.cpp Outdated Show resolved Hide resolved
modules/gdscript/gdscript_preprocessor.cpp Outdated Show resolved Hide resolved
modules/gdscript/gdscript_preprocessor.h Outdated Show resolved Hide resolved
@nonunknown nonunknown force-pushed the add-preprocessors branch 2 times, most recently from ad5fce8 to 6caa410 Compare August 18, 2023 15:42
@@ -390,7 +390,7 @@ class TextServer : public RefCounted {
virtual void font_set_opentype_feature_overrides(const RID &p_font_rid, const Dictionary &p_overrides) = 0;
virtual Dictionary font_get_opentype_feature_overrides(const RID &p_font_rid) const = 0;

virtual Dictionary font_supported_feature_list(const RID &p_font_rid) const = 0;
virtual Dictionary font_supported_feature_list_compiled(const RID &p_font_rid) const = 0;
Copy link
Member

Choose a reason for hiding this comment

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

I guess it's a search/replace result, there's nothing to preprocess at compile tine here, it's a features list of the specific font that can be determined only after the font is loaded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didnt changed this file, what your change has to do with the PR, sorry didnt got it!

// Feature system - char is used so we can have 64 features at max.
enum Feature {
ANDROID,
MOBILE_SYSTEM_FONTS,
Copy link
Member

Choose a reason for hiding this comment

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

MOBILE_SYSTEM_FONTS and SYSTEM_FONTS?. Why there are two features for the same thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in some platforms system_fonts can be calculated at compile time, others not, but still in gdscript you just do:

~if system_fonts

~endif

its called mobile, because in mobile its at compile time, but just discovered that in Macos is at compile time too, which is weird!

Comment on lines 120 to 121
virtual void _register_compiled_feature() override;
virtual bool OS_MacOS::_check_internal_feature_support(const String &p_feature) override;
Copy link
Member

Choose a reason for hiding this comment

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

Implementation in the os_macos.mm seems to be missing.

Copy link
Member

@AThousandShips AThousandShips Aug 18, 2023

Choose a reason for hiding this comment

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

And OS_Web from the abstract errors, possibly all OS save Linux perhaps as windows also fails

Copy link
Member

Choose a reason for hiding this comment

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

Still needed for OS_Web

@nonunknown nonunknown force-pushed the add-preprocessors branch 2 times, most recently from afa7dc7 to 2718e21 Compare August 18, 2023 16:24
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.

Add Preprocessors to GDScript
5 participants