I don't know if it makes sense to add even 3.5k to the export template to support one game. Couldn't you transcode these audio files or maintain a separate export template? 3.5k doesn't seem like much but for web games where it's common to strip down the export template via a custom build, every kb counts.

I don't know if it makes sense to add even 3.5k to the export template to support one game. Couldn't you transcode these audio files or maintain a separate export template? 3.5k doesn't seem like much but for web games where it's common to strip down the export template via a custom build, every kb counts.

The difference in HTML5 export templates is likely smaller (since these use optimize=size by default). It may be worth creating release HTML5 builds with and without this change to check if the difference is significant.

We're talking about a feature that likely only one person will ever use, though. And it seems like @Ithamar is more than capable of making their own export templates. I know 3.5k is practically nothing, but I still don't think this is a valuable feature for the mainline engine to have, and I think guidance is still to refuse-by-default to keep the engine small, right? https://godotengine.org/article/will-your-contribution-be-merged-heres-how-tell/

@ellenhp would a build-time option be acceptable? Or are you suggesting I maintain a fork?

I'll stick my nose in here to say that, were the mp2/mp1 branch be too big in html builds and considering that these formats are completely unused outside of remakes, a build flag would be perfect IMO, since it's basically a three line patch.

A build time option would be fine by me! I'm not opposed to this feature existing especially considering how little code maintenance burden it is for us (it's all in minimp3) but I just think that when someone goes to create a stripped-down web export for their game by removing every node that they don't use, that web export shouldn't have to know how to decode MP2. :)

@ellenhp thinking about it, are there some benchmarks about template size in relation to available features? I think that there might be lots of similar cases of "mostly unused thing that could be hidden behind a switch" that we could get rid of.

While this is a bit OT, #50148 might come useful in this regard, but as I said we should probably move into another thread if there exists one.

Sorry for the "redo everything" review, at least I suggested a lot of changes, so you should be able to just apply them if you agree.

BTW Don't forget to squash everything once you're done!

Sorry for the "redo everything" review, at least I suggested a lot of changes, so you should be able to just apply them if you agree.

No problem, its not like it is a lot of code, so even if I do it manually it is not a lot of work ;)

BTW Don't forget to squash everything once you're done!

Ah, Github does not do that automagically? I'm more used to Gitlab which has an PR option to do this at merge time.

@Ithamar

Ah, Github does not do that automagically?

Nope.

I'm more used to Gitlab which has an PR option to do this at merge time.

I doubt that most of us use this platform out of favour :P

I'm more used to Gitlab which has an PR option to do this at merge time.

I doubt that most of us use this platform out of favour :P

Actually, turns out Github has the option too, it might be an idea to simply enforce this for PRs if it is the expected behaviour anyway......

We don't use this option on the main repo, because it doesn't create merge commits.

@ellenhp thinking about it, are there some benchmarks about template size in relation to available features? I think that there might be lots of similar cases of "mostly unused thing that could be hidden behind a switch" that we could get rid of.

I made https://github.com/Calinou/godot-size-benchmarks a few years ago, but I also had another script that invidiaully compiled Godot with each module disabled and found that optimize=size alone had a greater impact on binary size compared to disabling all modules.

So I updated this PR to current master again, is there anything else I can do to help get this merged?

This looks fine overall as an option, but there's a problem with the implementation, as it's not self-contained. Modules are meant to be self-contained so the module-specific config can't modify SConstruct.

Thankfully there's a way to register new options from the module itself, I just tested it locally so I'll push an update to your PR directly with this change.

This looks fine overall as an option, but there's a problem with the implementation, as it's not self-contained. Modules are meant to be self-contained so the module-specific config can't modify SConstruct.

Ah, I was not aware of that.

Thankfully there's a way to register new options from the module itself, I just tested it locally so I'll push an update to your PR directly with this change.

Awesome, I'll take a look at your changes to see how it is done in case I need something similar in the future.

Thanks very much!

Seems like I don't have permission to push changes to the PR branch, so I pushed changes in my fork: akien-mga@e75da97

This is making use of get_opts in config.py to add a module-specific option.

If you can push this commit to your fork's branch to update this PR, it should be good to go.

Thanks! And congrats for your first merged Godot contribution 🎉

Thanks! And congrats for your first merged Godot contribution 🎉

Thank you for helping getting it over the line!

Also, is there a contributors file that I need to send a PR on, as this actually my 3rd (merged) contribution (see #71394 and #65717) and every time I get congratulated for my first merged issue (not complaining, just wondering why 😛 )

This means that your commits are not linked to your GitHub account. See: Why are my commits linked to the wrong user? for more info.

Despite this third merge, you're still shown like this: