search

godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.16k stars 97 forks source link

Replace the "Sample" from AudioStreamSample's name with something else #4532

Closed DeeJayLSP closed 2 years ago

DeeJayLSP commented 2 years ago

Describe the project you are working on

A game that uses lots of sound effects.

Describe the problem or limitation you are having in your project

When I first saw "AudioStreamSample" I thought it was a resource that imported any kind of audio file, while what it actually imported was just WAV format. In fact, I later found out AudioStreamSample isn't just for WAVs, but a resource that stores PCM data, allowing it to be used to store generated data or microphone recordings.

I think "Sample" might be misleading in some cases.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Simply rename the class to something else, like AudioStreamWAV or AudioStreamPCM. PCM would be more specific.

This was sort of discussed in a pull request, and ellenhp asked to do it as a feature proposal.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

All references to "AudioStreamSample" would change, simply.

There are already two pull requests ready for the options mentioned above:

AudioStreamWAV (there's already some discussion there)

AudioStreamPCM

Approve any of those, and this proposal is accepted.

If this enhancement will not be used often, can it be worked around with a few lines of script?

No

Is there a reason why this should be core and not an add-on in the asset library?

While this isn't exactly "core", you can't rename a class with an addon.

ellenhp commented 2 years ago

I support renaming to AudioStreamPCM for the reasons outlined here: https://github.com/godotengine/godot/pull/60953#issuecomment-1124350643

Additionally, I'd like to call attention to AudioStreamSample's save_as_wav method. That makes a lot of sense for a PCM resource, but not so much for a "wav" resource, in my opinion.

GeorgeS2019 commented 2 years ago

For background reference. wav or Waveform Audio File Format support many formats

Eventually, the choice needs to be consistent with the Godot vision, easy to adopt, low learning curve.

For beginner, it will be inherently default to be PCM (uncompressed).

For advanced users. there may be need to EVENTUALLY support LPCM (uncompressed) and ACM (Microsoft) (compressed) to show the maturity of the Godot community being inclusive to different main stream WAV audio formats

@aaronfranke I'm wondering if AudioStreamPCM would lead to confusion or a lack of discoverability. WAV files are typically used for short audio snippets/samples,

For suggestions above => AudioStreamWAV will immediately understood by most users coming from different programming backgrounds. Godot documentation on audio needs to reflect ease of use for beginners and awareness of other WAV formats reflecting advanced users.

GeorgeS2019 commented 2 years ago

Importing audio samples

WAV files use raw data or light compression (IMA-ADPCM). They are lightweight on the CPU to play back (hundreds of simultaneous voices in this format are fine). The downside is that they take up a lot of disk space.

WAV files come in different raw uncompressed and compressed formats. PCM is the common uncompressed raw format. The PCM format is lightweight on the CPU to play back (hundreds of simultaneous voices in this format are fine). The downside is that they take up a lot of disk space.

Audio compression is needed for saving disk space. The common ones are WAV with IMA-ADPCM compression, mp3 and Ogg Vorbis.

DeeJayLSP commented 2 years ago

Edited in response to the answer below

Remember: AudioStreamSample is not just used to load WAVs.

I know Godot should support compressed WAVs, but this proposal is meant to rename the class, not to add support for more formats.

Again, what the class does is storing raw PCM data (or IMA-ADPCM), not supporting WAVs with different data formats other than uncompressed PCM and 32bit IEEE float (I kinda took a look at the importer code to confirm it). My guess is that if the devs were to add support for compressed WAVs, they would probably add a conversion code to the importer, or more likely make a different class (let's suppose it would be named AudioStreamACM) to store it. ACM however is pretty deprecated at this point.

For now, my support goes to AudioStreamPCM, despite having started all this discussion with AudioStreamWAV, which I'm just keeping to see if there's significant community support.

ellenhp commented 2 years ago

We do support IMA-ADPCM right now.

DeeJayLSP commented 2 years ago

We do support IMA-ADPCM right now.

I guess this line of code needs an update? Nevermind I misread the code.

I see it stores as IMA-ADPCM, doesn't actually import it. What it does import, apart from PCM WAV, is 32bit IEEE float.

There may be a need to update that line, but it's unrelated to this proposal.

Should I add AudioStreamWaveform as a candidate? I think it's way more neutral than PCM and not as misleading as AudioStreamWAV.

ellenhp commented 2 years ago

Let's just wait to see what the community thinks, no need to go back and edit this or create a new PR. The idea of a proposal is to establish community consensus and let the conversation about a proposed change run its course. Then we look at it and consider the points brought up when we decide what to ultimately do.

DeeJayLSP commented 2 years ago

Let's just wait to see what the community thinks

Well, I barely see someone not supporting any renames. But since this proposal is way too recent, I think the community should be asked in a more accessible platform outside GitHub. Should I ask them on Twitter or Reddit?

ellenhp commented 2 years ago

Can we just wait for consensus to happen organically? There's no rush. These things take time because lots of contributors only check on the state of the project periodically.

DeeJayLSP commented 2 years ago

Sorry for my rush, it's because I know 4.0 is very close to a feature freeze which would likely delay my proposal to 5.0

ellenhp commented 2 years ago

No worries! I don't expect that we'll hit a feature freeze sooner than we get to a consensus here. I just want to make sure we pick the right name :)

DeeJayLSP commented 2 years ago

I will try to keep a list of pros and cons of each candidate name and update with feedback.

AudioStreamSample

Pros

AudioStreamWAV

Pros

AudioStreamPCM

Pros

AudioStreamWaveform

Pros

Suggestions are welcome

wareya commented 2 years ago

I believe AudioStreamPCM is a poor option. PCM generally refers to the type of audio data, not the format. OGG and MP3 and similar formats are ways of encoding PCM data; they are still PCM formats, they just don't store the PCM data directly. Examples of genuine non-PCM audio streams might be MIDI, synthesis sequences/streams, etc.

DeeJayLSP commented 2 years ago

Again, what the class does is storing raw PCM data (or IMA-ADPCM)

From lyuma, in the previous discussion:

If renaming is desired, I think a better name would be AudioStreamPCM, since that more closely matches the in-memory format.

AudioStreamOGGVorbis and AudioStreamMP3 are named as such, because they store their data in those respective formats in memory (Vorbis / MP3 streams) and perform streaming decompression during playback.

AudioStreamSample stores the PCM data (or IMA-ADPCM) directly. No such thing as a WAV format involved on storage. That's why AudioStreamPCM makes sense.

Still, my opinion leans towards AudioStreamWaveform to avoid much confusion.

wareya commented 2 years ago

Again, what the class does is storing raw PCM data (or IMA-ADPCM)

If renaming is desired, I think a better name would be AudioStreamPCM, since that more closely matches the in-memory format.

That sounds like leaking an abstraction out into the name to me. From my perspective, all of these types store PCM. Whether it's direct or not doesn't matter that much, especially since IMA-ADPCM data is not direct PCM data, but rather a way of encoding it (it just lacks frames like OGG and MP3 etc have).

Regardless, I'm not explicitly arguing in favor of WAV, just noting that using PCM itself in the name also has a similar pedantic downside.

No such thing as a WAV format involved on storage. Still, I'm leaning towards Waveform to avoid much confusion.

I have a suspicion that "Waveform" might make people imagine oscilloscopes or wavetable synths, and assume that it's for short PCM sources meant for fast looping. For example, googling "waveform" brings up as many images of sine/square/saw/triangle waves as it does of waveform analyzers.

What about AudioStreamWave? No "form". There are many things with the name "Wave", so it's not an explicit reference to the WAV format. That way it doesn't explicitly invoke the WAV format (and as such doesn't imply universal support for WAV features) and also doesn't invoke oscillators like "waveform" is sometimes associated with.

DeeJayLSP commented 2 years ago

I shall probably add AudioStreamWave as a candidate. Unironically, it was my first bet when starting this whole discussion.

In the meantime, I will wait for more opinions.

ellenhp commented 2 years ago

all of these types store PCM

I suppose. All of them also store sound "samples" though too! 😆 I would be fine with "Wav". Since the resource type can contain IMA-ADPCM data I'm not convinced that the arguments against "Wav" are as strong as I thought. We strip the wav header, but we do the same for ogg/vorbis header packets, and I don't want to bikeshed too much over the internal representation of the data. We actually unpack the entire ogg data structure technically for ogg/vorbis streams, but nobody knows that or has any reason to care unless they work on the engine.

I don't like the idea of calling it "Wave" with an "e", and I honestly am not sure why.

DeeJayLSP commented 2 years ago

Some points that have been brought into this discussion about the name is that it should:

I don't like the idea of calling it "Wave" with an "e", and I honestly am not sure why.

Probably because Wave have so many other meanings.

WAV is the short form of Waveform Audio File Format (which is probably why I suggested AudioStreamWaveform)

Also: If there's Audio in the name, I don't think people will relate the Waveform part as something outside of the subject.

wareya commented 2 years ago

WAV is the short form of Waveform Audio File Format (which is probably why I suggested AudioStreamWaveform)

WAVE is another valid formal name for it (sometimes RIFF WAVE if you want to be long about it); see the pdfs linked on this site. WAV is a back-formation from the 8.3-constrained file extension, like JPG and JPEG.

I think that leaning on the fact that Wave is both vague/broad and the noun-case version of one of its formal names might actually be a good way to compromise and avoid the downsides of the other options.

Also: If there's Audio in the name, I don't think people will relate the Waveform part as something outside of the subject.

The oscillator-related meaning of Waveform is audio-related, to be clear.

DeeJayLSP commented 2 years ago

At this point I don't want to overload Godot repo with PRs that won't be accepted, so for now I'll avoid opening new ones until a decision is taken.

reduz commented 2 years ago

Just to add to the discussion, AudioStreamSample has the following advantages:

IMO if its renamed, it would be good that it conveys this.

DeeJayLSP commented 2 years ago

I would like to introduce AudioStreamRaw to this discussion, despite probably being misleading as "Raw" in some formats may mean a format that needs to be converted

wareya commented 2 years ago

I think "Raw" is pretty good, but I contend that it might be confusing for something that supports IMA-ADPCM to be called "Raw". If I saw an audio storage type called "Raw", I would probably assume it stored completely uncompressed PCM.

This might not generalize to other people, though; for example, VOX ADPCM is supported in the list of encodings that you can tell Audacity to import when you use the "Import -> Raw Data" menu item.

DeeJayLSP commented 2 years ago

Candidates recap:

As Juan said, AudioStreamSample is a class that stores data that is very cheap to decode, resample and loop, while spending more memory. We should use a name that reflects that.

About WAV, Waveform and *Wave. Even if the class sort of behaves like a WAV by storing almost the same types of data, it's not a WAV.

*Sample is way too generic, as it could also mean other formats including compressed ones.

*PCM isn't that bad, but could be overshadowed by possibly other uncompressed formats being supported in the future.

Finally, *Raw kind of reflects well the data being stored, but could still be misleading.

To not oversimplify, I would like to suggest RawSample and RawData too.

I think the word Compression may be a bit confusing here.

To consider a data to be "compressed", it would have to be decompressed in order to be read, at real time or not.

I may be wrong, but I did some research on ADPCM formats (which, obviously, includes IMA-ADPCM) and found out what it does isn't quite compression, but lowering the amount of data while telling its readers to predict what's missing (this is how I interpreted it). That's why it could be classified as Raw.

ellenhp commented 2 years ago

I may be wrong, but I did some research on ADPCM formats (which, obviously, includes IMA-ADPCM) and found out what it does isn't quite compression, but lowering the amount of data while telling its readers to predict what's missing (this is how I interpreted it). That's why it could be classified as Raw.

I think you could say the same for any lossy compression?

This conversation is getting extremely difficult to follow because of all the branching conversations and repeated posts and I'd like to ask that we try to reduce the number of words we're considering please, and only comment with responses to previous suggestions (edit: unless you have something new you need to say or a very compelling new name idea). No need for "summary" comments, they just make things harder to follow.

Does anyone specifically disagree with calling it AudioStreamWav? Thumbs-up reacts can be used for support, no need to create a new comment in support, but please do reply if you disagree (include reason if you have one)

My reasoning is that we omit the wav header internally but that's just an implementation detail. 99% of the time when someone interacts with this resource type it will be because they imported a wav file. The only other time this resource is created is from AudioStreamRecord, but I'm not convinced that's a big deal for naming. I also would point out that the other two file-based audio resources are named after their file formats. I also would like to rename save_to_wav to save_to_file if we call it an AudioStreamWav to avoid duplicated information.

We're at 28 comments now on this proposal I want to start narrowing in on something we can agree on instead of adding even more ambiguity. Please, no more name ideas unless you are personally convinced that the new name idea is what we should use, and wish to argue in favor of it to convince everyone else. I don't like bikeshedding about renames. In my opinion anything that has strong community support would be better than AudioStreamSample, we don't need to be here for weeks picking the perfect name.

wareya commented 2 years ago

I may be wrong, but I did some research on ADPCM formats (which, obviously, includes IMA-ADPCM) and found out what it does isn't quite compression, but lowering the amount of data while telling its readers to predict what's missing (this is how I interpreted it). That's why it could be classified as Raw.

Pure ADPCM is compression, it's just lossless. (As opposed to IMA-ADPCM, which is generally lossy.) The "prediction" you're reading about DPCM doing is almost always just discrete differentiation and later integration, which is a lossless process.

Does anyone specifically disagree with calling it AudioStreamWav? Thumbs-up reacts can be used for support, no need to create a new comment in support, but please do reply if you disagree (include reason if you have one)

I disagree solely because AudioStreamWave would sidestep the "but it's not a .wav file" thing while also technically actually using the formal nickname for the source format. I also think Wave looks nicer than Wav.

DeeJayLSP commented 2 years ago

I would like to show my support to:

I also would like to show rejection to:

This will be my final opinion. With this comment I will slow down to avoid more pointless discussion until more people show opinions.

I advise everyone to speak out which ones you support/reject too.

Edit:

If I could rank them, it would be: Raw RawSample Wave PCM Waveform WAVE WAV Sample

YuriSizov commented 2 years ago

We've approved the rename to AudioStreamPCM (https://github.com/godotengine/godot/pull/60957) during the review meeting. You can focus on finalizing it! 🎉