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.

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

Add loop_mode to AudioStreamPlayer & AudioStreamPlayback #65797

Closed
wants to merge 1 commit into from

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Sep 14, 2022

Implements godotengine/godot-proposals#3120, (minus actually stripping the import)

This PR implemets loop_mode for AudioStreamPlayer, AudioStreamPlayer2D, AudioStreamPlayer3D and AudioStreamPlayback.

This allows changing the looping mode of the AudioStream, regardless of how it was imported, even as the stream is playing, which leads to pretty interesting results! Imagine playing a sound, keep it looping continuously, until a key is released.

AudioStreamPlayer.loop.mode.mp4

By default, for the AudioStreamPlayer's, this property is set to "Detect", which lets any generated AudioStreamPlayback automatically figure out the loop_mode, based on their respective AudioStream Resource.

image

Note is that, the streams may vary a lot (MP3, Ogg, WAV...). Their implementation has been mostly unchanged in this PR, so it is not guaranteed that the given loop_mode is supported, if at all, in fact. Whenever this is the case, a warning configuration is issued. When actually playing the AudioStream with AudioStreamPlayback, the variable is internally reassigned to its closest equivalent and a warning is sent to the console (this has been configured manually).

Configuration Warning Console Warning
image image image

The way AudioStreams are imported into the editor is left unchanged. This feature is all on top of it.

What I would like to do:

  • Fix AudioStreamPlaybackWAV not resuming forward as expected after switching from Ping Pong/Backward to Forward (as if stuck looping backwards).
  • Hide unsupported Loop Modes from selection when the AudioStream of AudioStreamPlayer does not support them (I believe validate_property() allows you to do this). PROPERTY_TYPE_ENUM does not support skipping options (adding that to my list, I suppose). The closest thing is a reminder next to each option.
  • Move LoopMode::DETECT to the top of the enum.
  • Manage detecting unsupported loop modes outside of AudioStreamPlayback, and into AudioStreamPlayer (Have AudioStream return a list of supported modes).
  • Clean-up this code considerably.
  • Add and update the documentation.

Optional ideas:

  • Default most AudioStreamPlaybacks created with AudioStream.instantiate_playback() to LoopMode.DETECT.
  • Expose virtual _set_loop_mode() and _get_unsupported_loop_modes() methods to GDScript.
  • Allow overriding loop_begin and loop_end (if supported).
  • Remove (now) unnecessary enum from AudioStreamWAV (the only AudioStream that supports all modes).
  • Move the LoopMode enum to AudioStreamPlayer itself(?).

Feedback is absolutely appreciated.
I am particularly concerned about the naming. Perhaps it should be called loop_mode_override, or something along those lines in the AudioStreamPlayers. Or playback_mode (renaming the constants accordingly, too) because that's technically what it is, actually? I was getting confused a bit while working on it,

@Calinou Calinou added this to the 4.0 milestone Sep 14, 2022
@Mickeon Mickeon force-pushed the try-audio-playback-loop branch 2 times, most recently from 0753077 to 5b2af6d Compare September 15, 2022 10:15
@Mickeon
Copy link
Contributor Author

Mickeon commented Sep 15, 2022

Previews are outdated. The property has been shifted higher up and its group has been removed (no point having it if overriding loop_begin and loop_end isn't supported). However, it's undeniable that all of these properties require some form of grouping around.
image

@Mickeon
Copy link
Contributor Author

Mickeon commented Sep 15, 2022

The PR is now functional enough to be reviewed!

@Mickeon Mickeon marked this pull request as ready for review September 15, 2022 12:05
@Mickeon Mickeon requested review from a team as code owners September 15, 2022 12:05
@Mickeon Mickeon force-pushed the try-audio-playback-loop branch 4 times, most recently from b90a6c1 to 394a946 Compare September 15, 2022 16:27
This PR implemets `loop_mode` for  **AudioStreamPlayer**, **AudioStreamPlayer2D**, **AudioStreamPlayer3D** and  **AudioStreamPlayback**.
This allows changing the looping mode of the **AudioStream**, regardless of how it was imported, even as the stream is playing.

By default, for the **AudioStreamPlayer**'s, this property is set to "_Detect_", which lets any generated **AudioStreamPlayback** automatically figure out the `loop_mode`, based on their respective **AudioStream** Resource.
@akien-mga akien-mga modified the milestones: 4.0, 4.x Sep 20, 2022
@akien-mga
Copy link
Member

We discussed it briefly in a PR review meeting, there was no clear consensus in favor of exposing this this way. This should be rediscussed for a future release together with the interactive music enhancements that could also include easier configuration of sample looping.

@fguillen
Copy link

fguillen commented Jun 1, 2023

Wondering if there has been any update on this.

@Calinou
Copy link
Member

Calinou commented Jun 2, 2023

Wondering if there has been any update on this.

The above comment is still relevant. Akien is referring to #64488 which hasn't been merged yet (and won't be for 4.1, as it requires further work).

@Mupli
Copy link

Mupli commented Sep 30, 2023

any news on Loops?

@Mickeon
Copy link
Contributor Author

Mickeon commented Sep 30, 2023

Nothing has been recently talked about on this regard. Judging by the last PR review meeting, this is not the maintainers want this feature to be accessed.

@LEEROY-3
Copy link

LEEROY-3 commented Nov 7, 2023

I apologize, but what exactly is being discussed by the team for so long? It has been over a year since this PR is functional and it is a pretty simplistic feature. It's unfortunate that this engine still doesn't allow easily overriding loop mode via player.

I would greatly appreciate if you shared some of the concerns the team had with this particular PR.

@akien-mga
Copy link
Member

Superseded by #86473.

@akien-mga akien-mga closed this Jan 16, 2024
@AThousandShips AThousandShips removed this from the 4.x milestone Jan 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants