Skip to content
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.

Sign up for GitHub

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

Jump to bottom

WAV stream/importer: Improve compression/loop names and descriptions #96174

Merged
merged 1 commit into from
Aug 30, 2024
Merged

WAV stream/importer: Improve compression/loop names and descriptions #96174

merged 1 commit into from
Aug 30, 2024

Conversation

DeeJayLSP
Copy link
Contributor

@DeeJayLSP DeeJayLSP commented Aug 27, 2024 •
edited
Loading

  • "Disabled" compression mode renamed to "PCM (Uncompressed)". It's the nomenclature used in other engines. Edit: added (Uncompressed) for clarity.
  • All mentions of "IMA ADPCM" such as "RAM(Ima-ADPCM)" and "IMA-ADPCM" renamed to the former (except code comments).
  • "QOA (Quite OK Audio)" reduced to "Quite OK Audio".
  • More details to compression modes have been added.
  • Added Detect From WAV on description.
  • AudioStreamWAV descriptions adjusted to remove info relevant to the importer.

Before:

image

After:

image

@DeeJayLSP DeeJayLSP requested review from a team as code owners August 27, 2024 16:06
Mickeon
Mickeon reviewed Aug 27, 2024
View reviewed changes
doc/classes/ResourceImporterWAV.xml Outdated
[b]Ping-Pong:[/b] Play audio forward until it's done playing, then play it backward and repeat. This is similar to mirrored texture repeat, but for audio.
[b]Backward:[/b] Play audio in reverse and loop back to the end when done playing.
Controls how audio should loop.
- [b]Detect From WAV:[/b] Uses loop information from the WAV metadata.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you did your homework

@AThousandShips AThousandShips added this to the 4.x milestone Aug 27, 2024
Mickeon
Mickeon reviewed Aug 27, 2024
View reviewed changes
doc/classes/AudioStreamWAV.xml Show resolved Hide resolved
doc/classes/ResourceImporterWAV.xml Outdated Show resolved Hide resolved
doc/classes/ResourceImporterWAV.xml Outdated Show resolved Hide resolved
doc/classes/ResourceImporterWAV.xml Outdated
- [b]Detect From WAV:[/b] Uses loop information from the WAV metadata.
- [b]Disabled:[/b] Don't loop audio, even if metadata indicates the file playback should loop.
- [b]Forward:[/b] Standard audio looping. Plays the audio forward from the begin point to the end point, then loops back.
- [b]Ping-Pong:[/b] Plays the audio forward until the end point, then backwards to the begin point, repeating this cycle.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Goodness me I hated that comparison with texture repeat. This is not criticism to this PR.

Small note, in some languages "cycle" and "loop" would be translated to the same thing. Maybe saying "then it repeats" would be more appropriate.

@DeeJayLSP DeeJayLSP force-pushed the wav-docs branch 2 times, most recently from d01e3b2 to 78c7f3a Compare August 27, 2024 20:51
@DeeJayLSP
Copy link
Contributor Author

Small correction on loop forward description: it always starts at the absolute beginning of the file (position 0), plays all the way to loop_end then seeks back to loop_begin

@DeeJayLSP
Copy link
Contributor Author

Forgot a few names in AudioStreamWAV properties, done now.

akien-mga
akien-mga reviewed Aug 30, 2024
View reviewed changes
editor/import/resource_importer_wav.cpp Outdated
@@ -90,7 +90,7 @@ void ResourceImporterWAV::get_import_options(const String &p_path, List<ImportOp
r_options->push_back(ImportOption(PropertyInfo(Variant::INT, "edit/loop_mode", PROPERTY_HINT_ENUM, "Detect From WAV,Disabled,Forward,Ping-Pong,Backward", PROPERTY_USAGE_DEFAULT | PROPERTY_USAGE_UPDATE_ALL_IF_MODIFIED), 0));
r_options->push_back(ImportOption(PropertyInfo(Variant::INT, "edit/loop_begin"), 0));
r_options->push_back(ImportOption(PropertyInfo(Variant::INT, "edit/loop_end"), -1));
r_options->push_back(ImportOption(PropertyInfo(Variant::INT, "compress/mode", PROPERTY_HINT_ENUM, "Disabled,RAM (Ima-ADPCM),QOA (Quite OK Audio)"), 0));
r_options->push_back(ImportOption(PropertyInfo(Variant::INT, "compress/mode", PROPERTY_HINT_ENUM, "PCM,IMA ADPCM,Quite OK Audio"), 0));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the many users who aren't particularly familiar with WAV format compression, I think it would still be worth to make it clear which of those is uncompressed (which users may understand intuitively to be bigger but faster on CPU).

Suggested change
r_options->push_back(ImportOption(PropertyInfo(Variant::INT, "compress/mode", PROPERTY_HINT_ENUM, "PCM,IMA ADPCM,Quite OK Audio"), 0));
r_options->push_back(ImportOption(PropertyInfo(Variant::INT, "compress/mode", PROPERTY_HINT_ENUM, "PCM (Uncompressed),IMA ADPCM,Quite OK Audio"), 0));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@akien-mga akien-mga modified the milestones: 4.x, 4.4 Aug 30, 2024
@akien-mga akien-mga requested a review from Calinou August 30, 2024 07:19
@DeeJayLSP DeeJayLSP requested a review from a team as a code owner August 30, 2024 19:28
@akien-mga akien-mga merged commit 77bc419 into godotengine:master Aug 30, 2024
19 checks passed
@akien-mga
Copy link
Member

Thanks!

@DeeJayLSP DeeJayLSP deleted the wav-docs branch August 30, 2024 23:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akien-mga akien-mga akien-mga approved these changes

@Mickeon Mickeon Mickeon approved these changes

@Calinou Calinou Awaiting requested review from Calinou

Assignees
No one assigned
Labels
documentation enhancement topic:audio topic:editor usability
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
4 participants
@DeeJayLSP @akien-mga @Mickeon @AThousandShips