Skip to content

Conversation

@YeldhamDev
Copy link
Member

This PR is a little intrusive I'm afraid. It does two things:

  • Moves files of Controls that are categorized as "advanced" to a folder called, well, "advanced". Due to this, a lot of files are touched to updated the path for header includes.
  • Makes so that those files aren't included when disable_advanced_gui is turned on. Before, it just didn't register the classes.

Here are the size comparasions. GNU/Linux, release templates (this time dev_build is turned off as well. 🙃). It saves almost 3 MB:

Default No Adv GUI
78.0 MB 75.2 MB

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

@YeldhamDev YeldhamDev added this to the 4.x milestone Feb 25, 2025
@YeldhamDev YeldhamDev requested review from a team as code owners February 25, 2025 18:39
@YeldhamDev YeldhamDev force-pushed the too_advanced_for_me branch 3 times, most recently from b13a13d to 4cac2ec Compare February 25, 2025 20:14
@YeldhamDev YeldhamDev force-pushed the too_advanced_for_me branch 2 times, most recently from 98e89a5 to 85bf5f0 Compare March 9, 2025 19:24
@YeldhamDev YeldhamDev force-pushed the too_advanced_for_me branch 2 times, most recently from c80c663 to dbdac13 Compare March 9, 2025 23:27
@Calinou
Copy link
Member

Calinou commented Mar 10, 2025

This makes sense on the surface. I'm hoping it doesn't break GDExtension compatibility though.

@YeldhamDev YeldhamDev force-pushed the too_advanced_for_me branch from dbdac13 to a0c8d85 Compare March 11, 2025 03:01
@YeldhamDev YeldhamDev force-pushed the too_advanced_for_me branch 4 times, most recently from d5fd991 to c55b1cb Compare March 21, 2025 19:27
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.

Tested locally, it works as expected.

Binary size for a stripped Linux x86_64 release export template with LTO (scons platform=linuxbsd target=template_release optimize=speed lto=full):

  • master: 72,031,960 bytes
  • This PR: 72,036,056 bytes (+ 5 KB)
  • master with disable_advanced_gui=yes: 69,324,408 bytes
  • This PR with disable_advanced_gui=yes: 69,099,096 bytes (-225 KB)

@Repiteo Repiteo modified the milestones: 4.x, 4.5 Mar 22, 2025
@akien-mga akien-mga self-requested a review March 25, 2025 21:49
@YeldhamDev YeldhamDev force-pushed the too_advanced_for_me branch 3 times, most recently from 6ba0ab1 to 23a4954 Compare March 29, 2025 08:35
@YeldhamDev YeldhamDev force-pushed the too_advanced_for_me branch from 23a4954 to 915e960 Compare March 30, 2025 18:19
@akien-mga
Copy link
Member

akien-mga commented Apr 9, 2025

I tested this and I don't think the gains are worth the awkward advanced/ subfolder and breakage. Most of this code already gets stripped by the linker due to it not being registered for use (using the same principle as engine build profiles), so this doesn't do much (aside from possibly decreasing compilation time a bit, as we avoid compiling objects just to discard them at link time).

Here's my test results for this PR and master (Linux x86_64 template_release with production=yes):

Branch disable_advanced_gui=no disable_advanced_gui=yes Difference
master (4248411) 70.25 MiB 67.50 MiB -2.75 MiB
This PR (915e960) 70.12 MiB 67.18 MiB -2.94 MiB

So the changes in this PR only strip an extra 195 KiB, which is IMO not worth breaking all PRs and well-used include paths for.

In fact, this option predates the build profile feature, and arguably isn't particularly well designed nowadays as it just removes an arbitrary set of control nodes, some of which may be needed for games which aren't GUI heavy (e.g. RichTextLabel). Build profiles (once made to reliably detect what a project uses) should be much finer and more useful.

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.

5 participants