-
-
Notifications
You must be signed in to change notification settings - Fork 24k
Make build profile project detection also set build options #103719
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
Make build profile project detection also set build options #103719
Conversation
Isn't there a platform-specific display driver option somewhere in the project settings? I think you could use that to at least add the default driver. Although arguably it would make more sense to keep both by default I think, from a compatibility perspective. |
ef2ba69 to
fe10992
Compare
|
@Riteo Yep, missed that setting. If it's set to |
64b1e52 to
7bcbe7d
Compare
7bcbe7d to
c31d94d
Compare
f2797f3 to
e8c1c28
Compare
e8c1c28 to
bc8e1f3
Compare
bc8e1f3 to
b80de8a
Compare
b80de8a to
619cf35
Compare
619cf35 to
b9c6cae
Compare
|
This PR is ready for review. |
It works after removing These messages are still printed on startup though: |
b0e45f9 to
00595d8
Compare
|
Silenced, try again. |
00595d8 to
995e1f4
Compare
Calinou
left a comment
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.
Works great now!
|
While doing further testing, I found an issue on the Build profile: pr.gdbuild.zip |
995e1f4 to
b8c0986
Compare
|
@Calinou The PR has been updated with the following:
@AThousandShips Changes made. |
b8c0986 to
7f67afb
Compare
|
Tested again, it works as expected. The Control Gallery demo now generates the profile correctly. I get a warning when running that demo with a compiled export template that uses the detected profile: While rebasing on |
|
I tested the PR on the 2D platformer demo: https://github.com/godotengine/godot-demo-projects/tree/master/2d/platformer The auto-detection works relatively well, but the Since the root node is of type Surprisingly, with Window disabled, the 2D platformer demo does work fine though. Only the functionality from the Game script (fullscreen and pause toggles) is broken. Tested on another project of mine: https://github.com/johncoffee/ngj-2024/ There it works well, Window is kept because some thirdparty asset uses some Dialogs. Comparison of a Web release template compiled with So there's 31% size reduction from using the build profile.
|
akien-mga
left a comment
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.
Did a code review pass, looks pretty good overall.
Let's finalize this and get it merged :D
| { BUILD_OPTION_DYNAMIC_FONTS, { | ||
| BUILD_OPTION_TEXT_SERVER_ADVANCED, | ||
| } }, |
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.
Is this dependency correct? I think you can use TTF/OTF dynamic fonts with TextServerFallback.
CC @bruvzg
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.
Yes, TextServerFallback supports all font file formats that TextServerAdvanced does. Specific font features may not work though.
In general, using TextServerFallback will introduce various subtle differences to behavior (e.g. #105497 (comment)). This means detecting whether we need TextServerAdvanced automatically is difficult. Even if you don't have any RTL language translations in your project, you may still be relying on OpenType features, variable fonts, and so on.
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.
Indeed, TextServerFallback support all font formats TextServerAdvanced does. But OpenType font features (like variations) are not supported.
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.
The opposite is also true, TextServerAdvanced can support RTL/BiDi with bitmap fonts. So it might be something like BUILD_OPTION_COMPLEX_TEXT_LAYOUTS, but not dynamic fonts.
| #ifdef MODULE_MONO_ENABLED | ||
| text += "\n\n" + TTR("Warning: Class detection for C# scripts is not currently available, and such files will be ignored."); | ||
| #endif // MODULE_MONO_ENABLED |
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.
FYI @godotengine/dotnet - would be good to implement as a follow-up.
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.
Oh believe me, I tried, and it's currently not possible in a way that makes it worth it. You can ask @raulsntos for more details.
| ADD_CLASS_DEPENDENCY("LineEdit"); | ||
| ADD_CLASS_DEPENDENCY("MenuButton"); | ||
| ADD_CLASS_DEPENDENCY("PopupMenu"); |
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.
How do you determine which classes should be added as dependencies?
I see color_picker.cpp has an internal LineEdit but there's also e.g. SpinBox, HSlider, etc.
Is this a requirement only when signals are used?
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.
Is this a requirement only when signals are used?
Seems so, as connecting signals require the class to be registered.
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.
FYI @KoBeWi. I wonder if we could find a more elegant way to do this so we don't need to hardcode such list only for internal classes that have signal connections.
It's fine for now for this PR though.
7f67afb to
342a1f7
Compare
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.
Looks great to me!
The configuration system isn't perfect yet but this is much better than what was before, and we can build upon it.
I have a couple minor outstanding comments but this should be good to merge soon.
342a1f7 to
f9960bf
Compare
f9960bf to
dbfe184
Compare
dbfe184 to
454e4f8
Compare
|
Thanks! |
Currently, the build profile editor only updates the used classes when attempting to detect from the project. This PR makes so that the other settings are also updated from the project information with the following.
get_build_dependencies(), that works likeget_classes_used()but is geared towards resource importers. And just like the latter, the former is also exposed to scripting.However, that are a couple of build options that I couldn't find a way to detect, because as far as I'm aware, there are no settings that could imply if the project is using them or not:
X11/WaylandThis PR also fixes some issues necessary to make the feature to work properly:
Sponsored By: 🐺 Lone Wolf Technology / 🍀 W4 Games.