Add option `modules_enabled_by_default` for shorthand disabling modules #41091

totlmstr · 2020-08-07T03:15:02Z

Godot has frankly a lot of modules, only complicated further when adding custom modules, so disabling each of them by hand can get rather cumbersome. This PR adds in a shorthand form for disabling all modules, including overriding their default settings (such as mono's default disabled). It's intended to be an advanced, experimental debug option.

~~Side note: I had to add in some ifdefs in cpp files because those files did not compile correctly with the new option.~~

For example, if I wanted to test out bullet exclusively without compiling others because I (e.g.) modified something in bullet, I would have to do:

scons -j6 tools=no module_bullet_enabled=yes module_arkit_enabled=no module_assimp_enabled=no module_basis_universal_enabled=no module_bmp_enabled=no  module_camera_enabled=no module_csg_enabled=no module_cvtt_enabled=no module_dds_enabled=no module_denoise_enabled=no module_enet_enabled=no module_etc_enabled=no module_freetype_enabled=no module_gdnative_enabled=no module_gdnavigation_enabled=no module_gdscript_enabled=no module_glslang_enabled=no module_gridmap_enabled=no module_hdr_enabled=no module_jpg_enabled=no module_jsonrpc_enabled=no module_lightmapper_rd_enabled=no module_mbedtls_enabled=no module_mobile_vr_enabled=no module_ogg_enabled=no module_opensimplex_enabled=no module_opus_enabled=no module_pvr_enabled=no module_regex_enabled=no module_squish_enabled=no module_stb_vorbis_enabled=no module_svg_enabled=no module_tga_enabled=no module_theora_enabled=no module_tinyexr_enabled=no module_upnp_enabled=no module_vhacd_enabled=no module_visual_script_enabled=no module_vorbis_enabled=no module_webm_enabled=no module_webp_enabled=no module_webrtc_enabled=no module_websocket_enabled=no module_xatlas_unwrap_enabled=no

This obviously gets even more complicated if I wanted to (e.g.) add in my custom module, enable it with bullet, and test it against that. In any case, the previous line with my new options becomes:

scons -j6 tools=no modules_enabled_by_default=no module_bullet_enabled=yes

Note: updated with module_bullet_enabled=yes instead of except_modules=bullet and modules_enabled_by_default=yes instead of disable_all_modules=yes and dev option.

Note that I added in dev as well, because one of the primary ways to effectively see if Godot works as is is to actually run the tests. Of course, in order to have the full effect of this PR to be realized, #40659 should be completed as well.

aaronfranke · 2020-08-07T03:31:27Z

What about if the syntax was disable_all_modules=yes module_bullet_enabled=yes? I think if a setting is not set then the default can be an empty string, and if a module is set to an empty string the logic can set it to the inverse of disable_all_modules.

totlmstr · 2020-08-07T03:51:54Z

What about if the syntax was disable_all_modules=yes module_bullet_enabled=yes? I think if a setting is not set then the default can be an empty string, and if a module is set to an empty string the logic can set it to the inverse of disable_all_modules.

@aaronfranke With that in mind, that would make my except_modules mostly useless, since most people will probably force their modules like that but not use or forget to use the new option. (I can just imagine this happening quite easily.)

Should be straightforward to implement that instead.

totlmstr · 2020-08-07T04:35:12Z

@aaronfranke Ended up being just a simple try-except block. The placement had to be pretty specific, because of the env_base update.

This should be ready for reviewing and/or requests for more edits.

SConstruct

main/main.cpp

Xrayez · 2020-08-07T10:13:09Z

I presume the use case of disabling modules is optimizing export templates for size?

Note that you can use custom.py and/or scons profile=modules_enabled.py option to make this easier (no need to use command-line for that currently). For a complete list of modules you can also look at .scons_env.json (only available once you attempt to build Godot) apart from --help which is easier to copy-paste and adapt into a Python script.

I confirm that with these changes, module_freetype_enabled=no works for compiling export templates (tools=no), but not for the editor: #28650. I think this deserve a separate PR as well, because it's a long standing issue. Besides there are already some modules which cannot be disabled as of now as seen in #39196. I'm also not sure whether it would actually work in release, yeah it does compile but what about run-time errors?

Xrayez · 2020-08-07T10:15:57Z

SConstruct

+            enabled = True
+    except:
+        if not env_base["disable_all_modules"]:
+            enabled = True


Without all those changes, modifying a single line seems to work too:

Suggested change

enabled = True

# Enable or disable modules by default.

enabled = not env_base["disable_all_modules"]

I originally thought that to be case, but (pointing out in your commit) mono's config.py will always override this setting with the code you have now, even if module_mono_enabled=true is set which leads to env_base being updated wrongly (notice that the configurations are being sent to env_base). On the same train of thought, I cannot do module_bullet_enabled=true for essentially the same reasons.

This led me to realize that I need to get the user input before that happens and skip over it.

Yeah that's an idea behind my commit, I'm not sure if a user would expect this overriding behavior. For your PR this logic makes sense I think.

Xrayez · 2020-08-07T10:31:34Z

This PR adds in a shorthand form for disabling all modules, including overriding their default settings (such as mono's default disabled)

Do you suggest that using disable_all_modules=no would enable all modules? I think that's misleading.

I think we may have to play an explicit game and call this option modules_enabled_by_default=no (default would be yes as now).

I believe the module's config should decide for itself whether it's enabled or disabled by default as well. What if a module is forced to be enabled by default because the engine depends on it? (But yeah in that case such a module shouldn't be a module in the first place...)

totlmstr · 2020-08-07T21:39:15Z

Note that you can use custom.py and/or scons profile=modules_enabled.py option to make this easier (no need to use command-line for that currently).

On this point, I should have been a bit clearer, but I only intend for this to be an alternative option to enabling and disabling the modules themselves. Replacing the custom.py option in general is really "fixing something that isn't broken".

Do you suggest that using disable_all_modules=no would enable all modules? I think that's misleading.

I think we may have to play an explicit game and call this option modules_enabled_by_default=no (default would be yes as now).

@Xrayez I have been personally concerned about the naming scheme of the option, but before I change the name of it (and resolving the other changes), I'll do some more changes on this PR first.

Xrayez · 2020-08-07T22:09:07Z

Yeah, sorry for the wave of review, I'm sorta suggesting possible workarounds as well, it may take months before something can be merged in Godot, (depending on how big your changes are and the scope of those changes), I'm just a contributor, and the project manager is currently on vacation... 🙂

totlmstr · 2020-08-08T00:03:08Z

I'm not sure if a user would expect this overriding behavior.

I can just imagine a user complaining why their option is not being overridden themselves, even though the option itself does say explicitly that it disables them. 🤣

With that in mind, that makes my option...nicer(?), so changing the option's name to something nicer(?) is actually the better option.

SConstruct

Calinou

Should be good to merge after applying the suggestion, rebasing then squashing.

This can be a good feature for contributors to make sure their newly added module doesn't accidentally depend on other modules.

SConstruct

aaronfranke · 2021-04-10T00:00:57Z

@totlmstr Any update? This would be nice to have, but this PR needs work.

totlmstr · 2021-04-10T21:13:12Z

@Calinou @aaronfranke Apologies for the delay on this. I haven't had much free time lately to be able to focus on this, so things should be much smoother now.

Did that rebase, and, at least from my perspective, the Freetype errors I have previously seen in the repo are not present now (even did a fresh pull just to be sure, since it has been a while). I simply just reverted my edits in those unneeded files, since there is no need for those macros anymore. If there are no more edits needed, this can be merged pretty easily.

aaronfranke

Works for me with both debug and release builds of export templates. I also tested production=yes builds which fail, but it also fails on master, so it's not a problem with this PR.

akien-mga · 2021-04-16T08:28:41Z

Thanks!

aaronfranke added enhancement topic:buildsystem labels Aug 7, 2020

totlmstr force-pushed the module-disable-edit branch from f7767f4 to 80586ed Compare August 7, 2020 03:39

totlmstr force-pushed the module-disable-edit branch from 80586ed to 0b80f12 Compare August 7, 2020 04:25

totlmstr changed the title ~~Add option disable_all_modules for shorthand disabling modules~~ Add option disable_all_modules for shorthand disabling modules Aug 7, 2020

totlmstr force-pushed the module-disable-edit branch 2 times, most recently from 93b59e1 to aa4b1cd Compare August 7, 2020 05:20

Xrayez reviewed Aug 7, 2020

View reviewed changes

SConstruct Outdated Show resolved Hide resolved

SConstruct Outdated Show resolved Hide resolved

main/main.cpp Outdated Show resolved Hide resolved

Xrayez reviewed Aug 7, 2020

View reviewed changes

totlmstr force-pushed the module-disable-edit branch 2 times, most recently from 39ba48a to 0c3d2c3 Compare August 7, 2020 20:39

totlmstr force-pushed the module-disable-edit branch from 0c3d2c3 to 3e9abd4 Compare August 7, 2020 23:45

totlmstr changed the title ~~Add option disable_all_modules for shorthand disabling modules~~ Add option "modules_enabled_by_default" for shorthand disabling modules Aug 8, 2020

totlmstr force-pushed the module-disable-edit branch 2 times, most recently from b61c329 to 2e0e2d1 Compare August 8, 2020 00:48

totlmstr mentioned this pull request Aug 8, 2020

Add missing #ifdef MODULE_SVG_ENABLED #41130

Merged

akien-mga reviewed Aug 18, 2020

View reviewed changes

SConstruct Outdated Show resolved Hide resolved

totlmstr force-pushed the module-disable-edit branch 2 times, most recently from c38081d to 00150b1 Compare August 23, 2020 18:59

totlmstr requested a review from akien-mga August 23, 2020 19:11

totlmstr mentioned this pull request Sep 7, 2020

CI: Add workflows to check if Godot compiles without modules enabled #41836

Closed

Calinou added this to the 4.0 milestone Jan 5, 2021

Calinou approved these changes Mar 21, 2021

View reviewed changes

SConstruct Outdated Show resolved Hide resolved

totlmstr force-pushed the module-disable-edit branch from 00150b1 to 63c0188 Compare April 10, 2021 21:06

totlmstr requested review from a team as code owners April 10, 2021 21:06

Add option modules_enabled_by_default

634ed3e

totlmstr force-pushed the module-disable-edit branch from 63c0188 to 634ed3e Compare April 10, 2021 21:17

aaronfranke reviewed Apr 11, 2021

View reviewed changes

aaronfranke approved these changes Apr 11, 2021

View reviewed changes

akien-mga approved these changes Apr 16, 2021

View reviewed changes

akien-mga merged commit ce618d1 into godotengine:master Apr 16, 2021

madmiraal mentioned this pull request Apr 17, 2021

Rename OS to Platform #47789

Closed

akien-mga mentioned this pull request Nov 12, 2021

[3.x] Add option modules_enabled_by_default and minimal CI build #53489

Merged

akien-mga changed the title ~~Add option "modules_enabled_by_default" for shorthand disabling modules~~ Add option modules_enabled_by_default for shorthand disabling modules Jan 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option `modules_enabled_by_default` for shorthand disabling modules #41091

Add option `modules_enabled_by_default` for shorthand disabling modules #41091

totlmstr commented Aug 7, 2020 •

edited

aaronfranke commented Aug 7, 2020

totlmstr commented Aug 7, 2020

totlmstr commented Aug 7, 2020 •

edited

Xrayez commented Aug 7, 2020 •

edited

Xrayez Aug 7, 2020 •

edited

totlmstr Aug 7, 2020 •

edited

Xrayez Aug 7, 2020

Xrayez commented Aug 7, 2020 •

edited

totlmstr commented Aug 7, 2020 •

edited

Xrayez commented Aug 7, 2020

totlmstr commented Aug 8, 2020

Calinou left a comment •

edited

aaronfranke commented Apr 10, 2021

totlmstr commented Apr 10, 2021

aaronfranke left a comment

akien-mga commented Apr 16, 2021

	enabled = True
	# Enable or disable modules by default.
	enabled = not env_base["disable_all_modules"]

Add option modules_enabled_by_default for shorthand disabling modules #41091

Add option modules_enabled_by_default for shorthand disabling modules #41091

Conversation

totlmstr commented Aug 7, 2020 • edited

aaronfranke commented Aug 7, 2020

totlmstr commented Aug 7, 2020

totlmstr commented Aug 7, 2020 • edited

Xrayez commented Aug 7, 2020 • edited

Xrayez Aug 7, 2020 • edited

Choose a reason for hiding this comment

totlmstr Aug 7, 2020 • edited

Choose a reason for hiding this comment

Xrayez Aug 7, 2020

Choose a reason for hiding this comment

Xrayez commented Aug 7, 2020 • edited

totlmstr commented Aug 7, 2020 • edited

Xrayez commented Aug 7, 2020

totlmstr commented Aug 8, 2020

Calinou left a comment • edited

Choose a reason for hiding this comment

aaronfranke commented Apr 10, 2021

totlmstr commented Apr 10, 2021

aaronfranke left a comment

Choose a reason for hiding this comment

akien-mga commented Apr 16, 2021

Add option `modules_enabled_by_default` for shorthand disabling modules #41091

Add option `modules_enabled_by_default` for shorthand disabling modules #41091

totlmstr commented Aug 7, 2020 •

edited

totlmstr commented Aug 7, 2020 •

edited

Xrayez commented Aug 7, 2020 •

edited

Xrayez Aug 7, 2020 •

edited

totlmstr Aug 7, 2020 •

edited

Xrayez commented Aug 7, 2020 •

edited

totlmstr commented Aug 7, 2020 •

edited

Calinou left a comment •

edited