Add build option to enable MP1 and MP2 support in minimp3 #72729

Ithamar · 2023-02-04T19:22:46Z

This adds only 3.5k to the template size (Win/64bits).

ellenhp · 2023-02-04T19:26:38Z

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.

Calinou · 2023-02-04T19:30:08Z

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.

ellenhp · 2023-02-04T19:53:45Z

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/

Ithamar · 2023-02-04T20:03:39Z

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

Riteo · 2023-02-04T20:08:35Z

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.

ellenhp · 2023-02-04T20:09:48Z

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. :)

Riteo · 2023-02-04T20:13:25Z

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

SConstruct

modules/minimp3/audio_stream_mp3.cpp

modules/minimp3/resource_importer_mp3.cpp

Riteo · 2023-02-04T21:47:41Z

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!

Ithamar · 2023-02-04T22:04:29Z

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.

Riteo · 2023-02-04T22:32:24Z

@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

Ithamar · 2023-02-05T10:17:42Z

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

YuriSizov · 2023-02-05T10:35:36Z

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

Calinou · 2023-02-07T13:49:41Z

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

Riteo

Other than a teeny tiny issue (which is also my fault), this looks like a great lil' patch!

SConstruct

Ithamar · 2023-09-30T18:08:11Z

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

akien-mga · 2023-09-30T18:39:43Z

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.

Ithamar · 2023-09-30T18:51:17Z

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!

akien-mga · 2023-09-30T18:52:00Z

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.

SConstruct

Enabling this adds 3.5k to the template size (Win/64bits). Co-authored-by: Riteo <riteo@posteo.net> Co-authored-by: Rémi Verschelde <rverschelde@gmail.com>

akien-mga · 2023-09-30T19:53:11Z

SConstruct

@@ -375,6 +375,8 @@ for name, path in modules_detected.items():
    else:
        enabled = False

+    opts.Add(BoolVariable("module_" + name + "_enabled", "Enable module '%s'" % (name,), enabled))


For context, I moved this so that the module_<name>_enabled option would come before the options specific to that module in scons --help. It shouldn't impact any of the logic otherwise (unless some custom modules rely on this being undefined in get_opts, but I doubt it :)).

akien-mga · 2023-10-02T11:24:03Z

Thanks! And congrats for your first merged Godot contribution 🎉

Ithamar · 2023-10-02T12:57:03Z

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 😛 )

akien-mga · 2023-10-02T13:06:01Z

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:

Ithamar requested a review from a team as a code owner February 4, 2023 19:22

Calinou added enhancement topic:audio labels Feb 4, 2023

Calinou added this to the 4.x milestone Feb 4, 2023

Ithamar requested a review from a team as a code owner February 4, 2023 20:49

Riteo requested changes Feb 4, 2023

View reviewed changes

SConstruct Outdated Show resolved Hide resolved

SConstruct Outdated Show resolved Hide resolved

modules/minimp3/audio_stream_mp3.cpp Outdated Show resolved Hide resolved

modules/minimp3/resource_importer_mp3.cpp Outdated Show resolved Hide resolved

Ithamar force-pushed the feat-mp1-mp2 branch from 5fdc35b to 5b0b27d Compare February 5, 2023 09:48

Ithamar requested review from a team as code owners February 5, 2023 09:48

Ithamar force-pushed the feat-mp1-mp2 branch from 5b0b27d to 164c2af Compare February 5, 2023 10:00

Riteo reviewed Feb 7, 2023

View reviewed changes

SConstruct Outdated Show resolved Hide resolved

Ithamar force-pushed the feat-mp1-mp2 branch from 7d662ee to 64afd35 Compare February 7, 2023 19:11

Riteo approved these changes Feb 7, 2023

View reviewed changes

Calinou mentioned this pull request Feb 9, 2023

Allow enabling tinyexr module compilation in export templates #73003

Draft

Ithamar force-pushed the feat-mp1-mp2 branch from 18faeb9 to 728cb09 Compare September 30, 2023 16:59

akien-mga modified the milestones: 4.x, 4.2 Sep 30, 2023

Ithamar force-pushed the feat-mp1-mp2 branch 3 times, most recently from a92d84f to 8719f7f Compare September 30, 2023 19:27

akien-mga reviewed Sep 30, 2023

View reviewed changes

SConstruct Outdated Show resolved Hide resolved

akien-mga approved these changes Sep 30, 2023

View reviewed changes

Add build option for MP1/MP2 audio format support.

36ff059

Enabling this adds 3.5k to the template size (Win/64bits). Co-authored-by: Riteo <riteo@posteo.net> Co-authored-by: Rémi Verschelde <rverschelde@gmail.com>

Ithamar force-pushed the feat-mp1-mp2 branch from 8719f7f to 36ff059 Compare September 30, 2023 19:47

akien-mga reviewed Sep 30, 2023

View reviewed changes

akien-mga merged commit ce236a6 into godotengine:master Oct 2, 2023
15 checks passed

akien-mga changed the title ~~Enable MP1 and MP2 support in minimp3 and editor~~ Add build option to enable MP1 and MP2 support in minimp3 Oct 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add build option to enable MP1 and MP2 support in minimp3 #72729

Add build option to enable MP1 and MP2 support in minimp3 #72729

Ithamar commented Feb 4, 2023

ellenhp commented Feb 4, 2023

Calinou commented Feb 4, 2023 •

edited

Loading

ellenhp commented Feb 4, 2023

Ithamar commented Feb 4, 2023

Riteo commented Feb 4, 2023

ellenhp commented Feb 4, 2023

Riteo commented Feb 4, 2023

Riteo commented Feb 4, 2023

Ithamar commented Feb 4, 2023

Riteo commented Feb 4, 2023

Ithamar commented Feb 5, 2023

YuriSizov commented Feb 5, 2023

Calinou commented Feb 7, 2023 •

edited

Loading

Riteo left a comment

Ithamar commented Sep 30, 2023

akien-mga commented Sep 30, 2023

Ithamar commented Sep 30, 2023

akien-mga commented Sep 30, 2023

akien-mga Sep 30, 2023

akien-mga commented Oct 2, 2023

Ithamar commented Oct 2, 2023

akien-mga commented Oct 2, 2023 •

edited

Loading

Add build option to enable MP1 and MP2 support in minimp3 #72729

Add build option to enable MP1 and MP2 support in minimp3 #72729

Conversation

Ithamar commented Feb 4, 2023

ellenhp commented Feb 4, 2023

Calinou commented Feb 4, 2023 • edited Loading

ellenhp commented Feb 4, 2023

Ithamar commented Feb 4, 2023

Riteo commented Feb 4, 2023

ellenhp commented Feb 4, 2023

Riteo commented Feb 4, 2023

Riteo commented Feb 4, 2023

Ithamar commented Feb 4, 2023

Riteo commented Feb 4, 2023

Ithamar commented Feb 5, 2023

YuriSizov commented Feb 5, 2023

Calinou commented Feb 7, 2023 • edited Loading

Riteo left a comment

Choose a reason for hiding this comment

Ithamar commented Sep 30, 2023

akien-mga commented Sep 30, 2023

Ithamar commented Sep 30, 2023

akien-mga commented Sep 30, 2023

akien-mga Sep 30, 2023

Choose a reason for hiding this comment

akien-mga commented Oct 2, 2023

Ithamar commented Oct 2, 2023

akien-mga commented Oct 2, 2023 • edited Loading

Calinou commented Feb 4, 2023 •

edited

Loading

Calinou commented Feb 7, 2023 •

edited

Loading

akien-mga commented Oct 2, 2023 •

edited

Loading