-
-
Notifications
You must be signed in to change notification settings - Fork 21.6k
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
Add option modules_enabled_by_default
for shorthand disabling modules
#41091
Conversation
What about if the syntax was |
f7767f4
to
80586ed
Compare
@aaronfranke With that in mind, that would make my Should be straightforward to implement that instead. |
80586ed
to
0b80f12
Compare
@aaronfranke Ended up being just a simple try-except block. The placement had to be pretty specific, because of the This should be ready for reviewing and/or requests for more edits. |
disable_all_modules
for shorthand disabling modules93b59e1
to
aa4b1cd
Compare
I presume the use case of disabling modules is optimizing export templates for size? Note that you can use I confirm that with these changes, |
SConstruct
Outdated
enabled = True | ||
except: | ||
if not env_base["disable_all_modules"]: | ||
enabled = True |
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.
Without all those changes, modifying a single line seems to work too:
enabled = True | |
# Enable or disable modules by default. | |
enabled = not env_base["disable_all_modules"] |
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.
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.
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.
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.
Do you suggest that using I think we may have to play an explicit game and call this option 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...) |
39ba48a
to
0c3d2c3
Compare
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
@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. |
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... 🙂 |
0c3d2c3
to
3e9abd4
Compare
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. |
b61c329
to
2e0e2d1
Compare
c38081d
to
00150b1
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.
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.
@totlmstr Any update? This would be nice to have, but this PR needs work. |
00150b1
to
63c0188
Compare
@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. |
63c0188
to
634ed3e
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 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.
Thanks! |
modules_enabled_by_default
for shorthand disabling modules
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 inbullet
, I would have to do: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:Note: updated with
module_bullet_enabled=yes
instead ofexcept_modules=bullet
andmodules_enabled_by_default=yes
instead ofdisable_all_modules=yes
anddev
option.Note that I added indev
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.