Skip to content

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

YeldhamDev
Copy link
Member

@YeldhamDev YeldhamDev commented Mar 6, 2025

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.

  • Checking project settings.
  • Checking classes that require them.
  • And last but definitely not least, import settings. This one required me to add an extra functionality called get_build_dependencies(), that works like get_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/Wayland
  • SIL Graphite Fonts

This PR also fixes some issues necessary to make the feature to work properly:

  • Fix error when scanning resource files.
  • Fix loading profile files doing nothing.
  • Hardcode some classes that are necessary for the engine to function.

Sponsored By: 🐺 Lone Wolf Technology / 🍀 W4 Games.

@Riteo
Copy link
Contributor

Riteo commented Mar 9, 2025

X11/Wayland.

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.

@YeldhamDev YeldhamDev force-pushed the build_detection_improvements branch from ef2ba69 to fe10992 Compare March 9, 2025 01:30
@YeldhamDev
Copy link
Member Author

@Riteo Yep, missed that setting. If it's set to default, both are now selected, otherwise, it will be the one you picked.

@YeldhamDev YeldhamDev force-pushed the build_detection_improvements branch 3 times, most recently from 64b1e52 to 7bcbe7d Compare March 12, 2025 14:54
@YeldhamDev YeldhamDev force-pushed the build_detection_improvements branch from 7bcbe7d to c31d94d Compare March 17, 2025 19:34
@YeldhamDev YeldhamDev force-pushed the build_detection_improvements branch 2 times, most recently from f2797f3 to e8c1c28 Compare March 17, 2025 21:36
@YeldhamDev YeldhamDev force-pushed the build_detection_improvements branch from e8c1c28 to bc8e1f3 Compare March 25, 2025 16:03
@YeldhamDev YeldhamDev added the bug label Mar 25, 2025
@YeldhamDev YeldhamDev force-pushed the build_detection_improvements branch from bc8e1f3 to b80de8a Compare March 25, 2025 16:18
@YeldhamDev YeldhamDev force-pushed the build_detection_improvements branch from b80de8a to 619cf35 Compare April 3, 2025 13:00
@YeldhamDev YeldhamDev force-pushed the build_detection_improvements branch from 619cf35 to b9c6cae Compare April 3, 2025 13:25
@YeldhamDev YeldhamDev marked this pull request as ready for review April 3, 2025 13:25
@YeldhamDev YeldhamDev requested review from a team as code owners April 3, 2025 13:25
@YeldhamDev
Copy link
Member Author

This PR is ready for review.

@YeldhamDev YeldhamDev force-pushed the build_detection_improvements branch from ccd1c8f to 10047c7 Compare April 8, 2025 18:08
@YeldhamDev
Copy link
Member Author

@Calinou Fixed.

@Calinou
Copy link
Member

Calinou commented Apr 8, 2025

Tested locally on the 3D platformer demo, I noticed issues with the detection (master also had issues, but different ones).

Build profiles: build_profiles_2.zip

  • In the detected features, I noticed GridMap and Decal is missing in master, even though the project uses both nodes. In the PR, GridMap is still missing, but Decal is correctly enabled this time.

  • In master, I can't start the exported project with this error:

ERROR: Class 'InputEventKey' isn't exposed.
   at: _instantiate_internal (core/object/class_db.cpp:549)
ERROR: Error parsing '/home/hugo/Documents/Git/godotengine/godot-demo-projects/3d/platformer/project.godot' at line 28: Can't instantiate Object() of type 'InputEventKey' File might be corrupted.
   at: _load_settings_text (core/config/project_settings.cpp:827)
ERROR: Couldn't load file '/home/hugo/Documents/Git/godotengine/godot-demo-projects/3d/platformer/project.godot', error code 43.
   at: _load_settings_text_or_binary (core/config/project_settings.cpp:861)
Error: Couldn't load project data at path ".". Is the .pck file missing?
If you've renamed the executable, the associated .pck file should also be renamed to match the executable's name (without the extension).

With the PR, it loads, but it crashes before rendering anything:

WARNING: Property not found: 'editor/movie_writer/mix_rate'.
     at: get_setting_with_override (core/config/project_settings.cpp:382)
WARNING: Property not found: 'editor/movie_writer/speaker_mode'.
     at: get_setting_with_override (core/config/project_settings.cpp:382)
WARNING: Property not found: 'editor/movie_writer/mjpeg_quality'.
     at: get_setting_with_override (core/config/project_settings.cpp:382)
Vulkan 1.4.303 - Forward+ - Using Device #0: NVIDIA - NVIDIA GeForce RTX 4090

ERROR: Cannot get class 'Skin'.
   at: _instantiate_internal (core/object/class_db.cpp:546)
ERROR: 'res://enemy/enemy.glb': Resource of unrecognized type in file: 'Skin'.
   at: load (core/io/resource_format_binary.cpp:779)
ERROR: Failed loading resource: res://.godot/imported/enemy.glb-d895c1723b6b5633e6a9ea1829adeaa2.scn. Make sure resources have been imported by opening the project in the editor at least once.
   at: _load (core/io/resource_loader.cpp:343)
ERROR: Failed loading resource: res://enemy/enemy.glb. Make sure resources have been imported by opening the project in the editor at least once.
   at: _load (core/io/resource_loader.cpp:343)
ERROR: res://enemy/enemy.tscn:444 - Parse Error: [ext_resource] referenced non-existent resource at: res://enemy/enemy.glb.
   at: _printerr (scene/resources/resource_format_text.cpp:39)
ERROR: Cannot get class 'Skin'.
   at: _instantiate_internal (core/object/class_db.cpp:546)
ERROR: 'res://player/player.glb': Resource of unrecognized type in file: 'Skin'.
   at: load (core/io/resource_format_binary.cpp:779)
ERROR: Failed loading resource: res://.godot/imported/player.glb-d5e59c3624fa2635da7ea043f9526ccc.scn. Make sure resources have been imported by opening the project in the editor at least once.
   at: _load (core/io/resource_loader.cpp:343)
ERROR: Failed loading resource: res://player/player.glb. Make sure resources have been imported by opening the project in the editor at least once.
   at: _load (core/io/resource_loader.cpp:343)
ERROR: res://player/player.tscn:116 - Parse Error: [ext_resource] referenced non-existent resource at: res://player/player.glb.
   at: _printerr (scene/resources/resource_format_text.cpp:39)
ERROR: Cannot get class 'GridMap'.
   at: _instantiate_internal (core/object/class_db.cpp:546)
WARNING: Node GridMap of type GridMap cannot be created. A placeholder will be created instead.
     at: instantiate (scene/resources/packed_scene.cpp:277)
ERROR: Node not found: "Enemy/AnimationPlayer" (relative to "/root/Game/Enemies/Enemy1").
   at: get_node (scene/main/node.cpp:1872)
ERROR: Node not found: "Enemy/Skeleton/RayFloor" (relative to "/root/Game/Enemies/Enemy1").
   at: get_node (scene/main/node.cpp:1872)
ERROR: Node not found: "Enemy/Skeleton/RayWall" (relative to "/root/Game/Enemies/Enemy1").
   at: get_node (scene/main/node.cpp:1872)
ERROR: Node not found: "Enemy/AnimationPlayer" (relative to "/root/Game/Enemies/Enemy2").
   at: get_node (scene/main/node.cpp:1872)
ERROR: Node not found: "Enemy/Skeleton/RayFloor" (relative to "/root/Game/Enemies/Enemy2").
   at: get_node (scene/main/node.cpp:1872)
ERROR: Node not found: "Enemy/Skeleton/RayWall" (relative to "/root/Game/Enemies/Enemy2").
   at: get_node (scene/main/node.cpp:1872)
ERROR: Node not found: "Enemy/AnimationPlayer" (relative to "/root/Game/Enemies/Enemy3").
   at: get_node (scene/main/node.cpp:1872)
ERROR: Node not found: "Enemy/Skeleton/RayFloor" (relative to "/root/Game/Enemies/Enemy3").
   at: get_node (scene/main/node.cpp:1872)
ERROR: Node not found: "Enemy/Skeleton/RayWall" (relative to "/root/Game/Enemies/Enemy3").
   at: get_node (scene/main/node.cpp:1872)
ERROR: Node not found: "Enemy/AnimationPlayer" (relative to "/root/Game/Enemies/Enemy4").
   at: get_node (scene/main/node.cpp:1872)
ERROR: Node not found: "Enemy/Skeleton/RayFloor" (relative to "/root/Game/Enemies/Enemy4").
   at: get_node (scene/main/node.cpp:1872)
ERROR: Node not found: "Enemy/Skeleton/RayWall" (relative to "/root/Game/Enemies/Enemy4").
   at: get_node (scene/main/node.cpp:1872)
ERROR: Node not found: "%CoinCount" (relative to "/root/Game/Player").
   at: get_node (scene/main/node.cpp:1872)
ERROR: Node not found: "%CoinCount" (relative to "/root/Game/Player").
   at: get_node (scene/main/node.cpp:1872)
[1]    98558 segmentation fault (core dumped)  bin/godot.linuxbsd.template_release.x86_64.profile_pr --path 

Notice the MovieWriter warnings on startup as well, likely because we forgot to add a check in main.cpp.

Backtrace:

#0  0x000000000102faba in Node::get_node_or_null (this=this@entry=0x0, p_path=...) at scene/main/node.cpp:1807
#1  0x000000000103012f in Node::get_node (this=0x0, p_path=...) at scene/main/node.cpp:1864
#2  0x0000000001014168 in call_with_validated_variant_args_retc_helper<__UnexistingClass, Node*, NodePath const&, 0ul> (p_instance=<optimized out>, 
    p_method=<optimized out>, p_args=<optimized out>, r_ret=0x7fffffffb918) at ./core/variant/binder_common.h:390
#3  call_with_validated_object_instance_args_retc<__UnexistingClass, Node*, NodePath const&> (base=<optimized out>, p_method=<optimized out>, 
    p_args=<optimized out>, r_ret=0x7fffffffb918) at ./core/variant/binder_common.h:677
#4  MethodBindTRC<Node*, NodePath const&>::validated_call (this=<optimized out>, p_object=<optimized out>, p_args=<optimized out>, 
    r_ret=0x7fffffffb918) at ./core/object/method_bind.h:625
#5  0x0000000000747aab in GDScriptFunction::call (this=<optimized out>, p_instance=<optimized out>, p_args=<optimized out>, 
    p_argcount=<optimized out>, r_err=..., p_state=<optimized out>) at modules/gdscript/gdscript_vm.cpp:2244
#6  0x000000000062a915 in GDScriptInstance::callp (this=0x8b24dd0, p_method=..., p_args=0x7fffffffbe68, p_argcount=1, r_error=...)
    at modules/gdscript/gdscript.cpp:2051
#7  0x0000000001004e72 in Node::_gdvirtual__physics_process_call (this=0x8b42910, arg1=0.0083333333333333332) at scene/main/node.h:385
#8  Node::_notification (this=0x8b42910, p_notification=<optimized out>) at scene/main/node.cpp:57
#9  0x0000000001207a9d in Node::_notification_forwardv (this=<optimized out>, p_notification=<optimized out>, this=<optimized out>, 
    p_notification=<optimized out>) at ./scene/main/node.h:48
#10 Node3D::_notification_forwardv (this=<optimized out>, p_notification=<optimized out>, this=<optimized out>, p_notification=<optimized out>)
    at ./scene/3d/node_3d.h:51
#11 CollisionObject3D::_notification_forwardv (this=0x8b42910, p_notification=16) at ./scene/3d/physics/collision_object_3d.h:37
#12 PhysicsBody3D::_notification_forwardv (this=<optimized out>, p_notification=<optimized out>, this=<optimized out>, p_notification=<optimized out>)
    at ./scene/3d/physics/physics_body_3d.h:39
#13 CharacterBody3D::_notification_forwardv (this=0x8b42910, p_notification=16) at scene/3d/physics/character_body_3d.h:37
#14 0x000000000201b5a1 in Object::_notification_forward (this=0x8b42910, p_notification=16) at core/object/object.cpp:911
#15 0x000000000101e046 in Object::notification (this=<optimized out>, p_notification=<optimized out>, p_reversed=<optimized out>, 
    this=<optimized out>, p_notification=<optimized out>, p_reversed=<optimized out>) at ./core/object/object.h:889
#16 SceneTree::_process_group (this=this@entry=0x7525e90, p_group=<optimized out>, p_physics=p_physics@entry=true) at scene/main/scene_tree.cpp:1069
#17 0x0000000001033466 in SceneTree::_process (this=this@entry=0x7525e90, p_physics=p_physics@entry=true) at scene/main/scene_tree.cpp:1153
#18 0x0000000001033c38 in SceneTree::physics_process (this=0x7525e90, p_time=0.0083333333333333332) at scene/main/scene_tree.cpp:540
#19 0x00000000004dd497 in Main::iteration () at main/main.cpp:4637
#20 0x000000000044bf81 in OS_LinuxBSD::run (this=0x7fffffffbe20) at platform/linuxbsd/os_linuxbsd.cpp:980
#21 OS_LinuxBSD::run (this=this@entry=0x7fffffffc3f0) at platform/linuxbsd/os_linuxbsd.cpp:963
#22 0x0000000000416b15 in main (argc=3, argv=0x7fffffffca38) at platform/linuxbsd/godot_linuxbsd.cpp:85

Binary sizes I'm getting so far (Linux x86_64 release export template with production=yes lot=full):

  • master: 73,650,072 bytes
  • master with profile: 53,234,192 bytes
  • This PR: 44,974,448 bytes 49,473,880 bytes 49,596,792 bytes

That said, since too many classes are being disabled right now, I expect binary sizes will need to increase somewhat to resolve the issue.

@YeldhamDev YeldhamDev force-pushed the build_detection_improvements branch from 10047c7 to b0e45f9 Compare April 9, 2025 13:52
@YeldhamDev YeldhamDev requested a review from a team as a code owner April 9, 2025 13:52
@YeldhamDev
Copy link
Member Author

@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.

@Calinou
Copy link
Member

Calinou commented Apr 9, 2025

@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?

@YeldhamDev
Copy link
Member Author

it still excludes GridMap

That's not right, it manages to find GridMap on my end. Are you deleting the cache before re-detecting?

@Calinou
Copy link
Member

Calinou commented Apr 9, 2025

That's not right, it manages to find GridMap on my end. Are you deleting the cache before re-detecting?

It works after removing .godot/ 🙂
The project works just fine with the custom build profile now.

These messages are still printed on startup though:

WARNING: Property not found: 'editor/movie_writer/mix_rate'.
     at: get_setting_with_override (core/config/project_settings.cpp:405)
WARNING: Property not found: 'editor/movie_writer/speaker_mode'.
     at: get_setting_with_override (core/config/project_settings.cpp:405)
WARNING: Property not found: 'editor/movie_writer/mjpeg_quality'.
     at: get_setting_with_override (core/config/project_settings.cpp:405)

@YeldhamDev YeldhamDev force-pushed the build_detection_improvements branch from b0e45f9 to 00595d8 Compare April 9, 2025 22:56
@YeldhamDev
Copy link
Member Author

Silenced, try again.

@YeldhamDev YeldhamDev force-pushed the build_detection_improvements branch from 00595d8 to 995e1f4 Compare April 14, 2025 21:29
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Works great now!

@Calinou
Copy link
Member

Calinou commented Apr 14, 2025

While doing further testing, I found an issue on the gui/control_gallery demo. After compiling with the autodetected profile, I get this:

ERROR: In Object of type 'TabBar': Attempt to connect nonexistent signal 'mouse_exited' to callable ''.
   at: connect (core/object/object.cpp:1454)
WARNING: Property not found: 'gui/common/default_scroll_deadzone'.
     at: get_setting_with_override (core/config/project_settings.cpp:405)

Build profile: pr.gdbuild.zip

@YeldhamDev YeldhamDev force-pushed the build_detection_improvements branch from 995e1f4 to b8c0986 Compare April 15, 2025 23:33
@YeldhamDev YeldhamDev requested a review from a team as a code owner April 15, 2025 23:33
@YeldhamDev
Copy link
Member Author

@Calinou The PR has been updated with the following:

  • Improved the extraction of classes from binary scene files. This removes the need for Skin to be hardcoded.
  • Added ADD_CLASS_DEPENDENCY(). This is used inside _bind_methods() so classes can mark if they depend on other classes being registered. This fixes the error with TabBar.
  • Removed the logic to toggle advanced GUI in the build profile, as stated by Akien, that was made before this feature existed, and it's kinda redundant now.

@AThousandShips Changes made.

@Repiteo Repiteo modified the milestones: 4.x, 4.5 Apr 16, 2025
@YeldhamDev YeldhamDev force-pushed the build_detection_improvements branch from b8c0986 to 7f67afb Compare May 4, 2025 20:17
@Calinou
Copy link
Member

Calinou commented May 6, 2025

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:

WARNING: Property not found: 'gui/common/default_scroll_deadzone'.
     at: get_setting_with_override (core/config/project_settings.cpp:404)

While rebasing on master, to account for the conflict generated by 55dd5d5, I chose to kept current changes only (i.e. what's in master). It performs the check slightly differently, since it checks each MovieWriter individually (which makes more sense to me). It seems to work fine.

@akien-mga
Copy link
Member

akien-mga commented May 16, 2025

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 Window class is wrongly disabled, so the one script fails parsing:

SCRIPT ERROR: Parse Error: Native class Window used in script doesn't exist or isn't exposed. [tmp_js_export.js:477:18](http://localhost:8060/tmp_js_export.js)
   at: GDScript::reload (res://game.gd:16)

Since the root node is of type Window, I believe Window should never be disabled?

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 scons p=web target=template_release production=yes lto=none (disabling LTO as for Clang is actually increases binary size), and the profile build with build_profile generated from this PR.

-rw-r--r--.  1 akien akien 8.6M May 19 15:48 godot.web.template_release.wasm32.master.zip
-rw-r--r--.  1 akien akien 5.9M May 19 16:01 godot.web.template_release.wasm32.profile.zip
-rw-r--r--.  1 akien akien 5.2M May 19 16:21 godot.web.template_release.wasm32.profile_manual.zip

So there's 31% size reduction from using the build profile.

profile_manual is a custom profile based on the generated one, removing more stuff not needed for my Web export:

  • Navigation2D - not sure why this one wasn't detected as unused
  • TextServerAdvanced - using Fallback instead, WOFF2 fonts, SIL Graphite fonts
  • Node3D and AcceptDialog - those were kept due to an addon, but the scene is unused

Copy link
Member

@akien-mga akien-mga left a 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

Comment on lines +226 to +229
{ BUILD_OPTION_DYNAMIC_FONTS, {
BUILD_OPTION_TEXT_SERVER_ADVANCED,
} },
Copy link
Member

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

Copy link
Member

@Calinou Calinou May 21, 2025

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.

Copy link
Member

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.

Copy link
Member

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.

Comment on lines +741 to +744
#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
Copy link
Member

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.

Copy link
Member Author

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.

Comment on lines +1997 to +2120
ADD_CLASS_DEPENDENCY("LineEdit");
ADD_CLASS_DEPENDENCY("MenuButton");
ADD_CLASS_DEPENDENCY("PopupMenu");
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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.

@YeldhamDev YeldhamDev force-pushed the build_detection_improvements branch from 7f67afb to 342a1f7 Compare May 20, 2025 23:24
Copy link
Member

@akien-mga akien-mga left a 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.

@YeldhamDev YeldhamDev force-pushed the build_detection_improvements branch from 342a1f7 to f9960bf Compare May 30, 2025 00:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants