-
-
Notifications
You must be signed in to change notification settings - Fork 22.5k
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
base: master
Are you sure you want to change the base?
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. |
ccd1c8f
to
10047c7
Compare
@Calinou Fixed. |
Tested locally on the 3D platformer demo, I noticed issues with the detection ( Build profiles: build_profiles_2.zip
With the PR, it loads, but it crashes before rendering anything:
Notice the MovieWriter warnings on startup as well, likely because we forgot to add a check in Backtrace:
Binary sizes I'm getting so far (Linux x86_64 release export template with
That said, since too many classes are being disabled right now, I expect binary sizes will need to increase somewhat to resolve the issue. |
10047c7
to
b0e45f9
Compare
@Calinou Alright, the problem was that class extraction from binary resources was broken (like almost everything about this feature before this PR. 🫠). I have now fixed it, please try again. |
It works well now – I've updated the binary size in my original comment (it still excludes GridMap). One remaining issue is that GridMap is still not being added using feature detection, so you fall through the void when starting the project. Everything else works, so if you manually check GridMap or add it to Forced Classes on Detect, it's fine. I wonder how we can fix this issue about GridMap not being detected. Is it caused by the GridMap node being part of a module? |
That's not right, it manages to find |
It works after removing These messages are still printed on startup though:
|
b0e45f9
to
00595d8
Compare
Silenced, try again. |
00595d8
to
995e1f4
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.
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.
|
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
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.