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

Improve AudioStream looping #86517

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Improve AudioStream looping #86517

wants to merge 1 commit into from
+75 −8

Conversation

reduz
Copy link
Member

@reduz reduz commented Dec 26, 2023 •
edited
Loading

  • AudioStream (OGG/MP3) import dialog now allows to optionally set loop in beats.
  • Loop points other than zero now properly fade-in to avoid clicks.
  • Stream import dialog now indicates the beat where loop occurs.
  • Fixed the preview generation in the importer dialog to not show the looped part.
  • Cleaned up the import dialog a bit.

Screenshot:

image

@reduz reduz requested review from a team as code owners December 26, 2023 09:48
@reduz
Improve AudioStream looping
89b20b8 
* AudioStream (OGG/MP3) import dialog now allows to optionally set loop in beats.
* Loop points other than zero now properly fade-in to avoid clicks. Keep in mind if you have strong transients in the loop they may be smoothed due to the fade, so consider eventually separating in two clips and use upcoming AudioStreamPlaybackPlaylist or AudioStreamPlaybackInteractive.
* Stream import dialog now indicates the beat where loop occurs.
* Cleaned up the import dialog a bit.
@reduz reduz mentioned this pull request Dec 26, 2023
Merged
@AThousandShips AThousandShips added this to the 4.3 milestone Dec 26, 2023
@MJacred
Copy link
Contributor

MJacred commented Dec 27, 2023

@reduz: please make the PR title a little more explicit, e.g. "Improve AudioStream looping when using BPM logic", as looping in seconds stays unaffected

@reduz
Copy link
Member Author

reduz commented Dec 27, 2023 •
edited
Loading

@MJacred It also fixes the clicking sound of regular loops when position is not zero.

@ellenhp
Copy link
Contributor

ellenhp commented Dec 27, 2023

(on mobile so I can't review fully) Does this also fade in if you call play(nonzero)? If so we can remove the fade in logic in the audio server and fix the open bug about beginnings of sounds being cut off.

@MJacred
Copy link
Contributor

MJacred commented Dec 27, 2023

@reduz: loop_fade_remaining is only set to a value < FADE_SIZE in an if-clause, which depends on vorbis_stream->get_bpm() > 0 and beat_length_frames >= 0...
There is no fading in ogg/mp3 on looping if BPM is not used

@MJacred
Copy link
Contributor

MJacred commented Dec 27, 2023 •
edited
Loading

@ellenhp: on play(any_value), a new AudioStreamPlayback is instantiated (so the loop fading is reset). At least, there is no loop fading in ogg vorbis and mp3. The audio server only ever does a fade out. So afaik no fading here

@ellenhp
Copy link
Contributor

ellenhp commented Dec 27, 2023 •
edited
Loading

Unless someone removed that logic since #71780 it's definitely still in there. Imo we should move that fade to be triggered by a seek. I'm not sure if it's been suggested but I want to preemptively say I strongly disagree with moving fade-out logic into the playbacks because that'll introduce pops when an audio stream player is removed from the tree without being stopped. Fade-in and loop fades (both triggered by a seek operation) can be moved to playbacks without introducing bugs I think.

@MJacred
Copy link
Contributor

MJacred commented Dec 27, 2023

Ok, you're right. I relied on variable naming. So if there's just some non-descriptive (for me at least) math going on, I missed it.

@ellenhp
Copy link
Contributor

ellenhp commented Dec 27, 2023

Yeah it's not documented well which is on me.

@MJacred
Copy link
Contributor

MJacred commented Dec 28, 2023 •
edited
Loading

There's no developer who hasn't neglected documentation before. But you can dump your knowledge here godotengine/godot-docs#7896 (or just reference valuable PRs/proposals/issues, where I can read up) and we'll have something that sticks around in one place :D (PR hijacking over and out)

@reduz reduz mentioned this pull request Feb 6, 2024
Closed
@akien-mga
Copy link
Member

The docs need to be added for the new members:

 diff --git a/modules/minimp3/doc_classes/ResourceImporterMP3.xml b/modules/minimp3/doc_classes/ResourceImporterMP3.xml
index a84c51c..e46285b 100644
--- a/modules/minimp3/doc_classes/ResourceImporterMP3.xml
+++ b/modules/minimp3/doc_classes/ResourceImporterMP3.xml
@@ -33,5 +33,7 @@
 			Only has an effect if [member loop] is [code]true[/code].
 			A more convenient editor for [member loop_offset] is provided in the [b]Advanced Import Settings[/b] dialog, as it lets you preview your changes without having to reimport the audio.
 		</member>
+		<member name="loop_offset_type" type="int" setter="" getter="" default="0">
+		</member>
 	</members>
 </class>
diff --git a/modules/vorbis/doc_classes/ResourceImporterOggVorbis.xml b/modules/vorbis/doc_classes/ResourceImporterOggVorbis.xml
index dd6c181..8f225e8 100644
--- a/modules/vorbis/doc_classes/ResourceImporterOggVorbis.xml
+++ b/modules/vorbis/doc_classes/ResourceImporterOggVorbis.xml
@@ -49,5 +49,7 @@
 			Only has an effect if [member loop] is [code]true[/code].
 			A more convenient editor for [member loop_offset] is provided in the [b]Advanced Import Settings[/b] dialog, as it lets you preview your changes without having to reimport the audio.
 		</member>
+		<member name="loop_offset_type" type="int" setter="" getter="" default="0">
+		</member>
 	</members>
 </class>

(and the descriptions written)

akien-mga
akien-mga approved these changes Feb 9, 2024
View reviewed changes
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Code and rationale seem fine to me. Didn't test.

editor/import/audio_stream_import_settings.cpp
loop_offset->set_tooltip_text(TTR("Loop offset (from beginning). Note that if BPM is set, this setting will be ignored."));
loop_offset->connect("value_changed", callable_mp(this, &AudioStreamImportSettingsDialog::_settings_changed).unbind(1));
loop_hb->add_child(loop_offset);
loop_offset_type = memnew(OptionButton);
loop_offset_type->add_item(TTR("Sec"));
Copy link
Member

Choose a reason for hiding this comment

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

We don't seem to width constrained here so I would avoid abbreviations, which can be hard to translate.

Suggested change
loop_offset_type->add_item(TTR("Sec"));
loop_offset_type->add_item(TTR("Seconds"));

@reduz
Copy link
Member Author

reduz commented Feb 12, 2024

Will do the changes suggested ASAP.

@akien-mga akien-mga modified the milestones: 4.3, 4.4 May 28, 2024
@akien-mga
Copy link
Member

Postponing to 4.4 as we're now in feature freeze for 4.3, and this still needs some more work.

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

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

Successfully merging this pull request may close these issues.
5 participants
@reduz @MJacred @ellenhp @akien-mga @AThousandShips