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

Rework sound system for better music support #62896

Closed
wants to merge 1 commit into from

Conversation

reduz
Copy link
Member

@reduz reduz commented Jul 10, 2022

This PR reworks the Godot sound sytem to improve the support for doing things with music content.

  • Adds BPM/Bar/Beat Length to some types of AudioStream
  • Implements proper beat-based looping for music files (current looping support is poor).
  • Added an advanced importer for OGG and MP3 files that allows seting the above information with more ease.
  • Improves the visualization of sound resources, including showing sound playback position while in use.

As a bonus, ported and reworked the interactive music player from #32769.
Not finished but works as a test of the above features.

Some screens:

Importing OGG/MP3 files, advanced settings:

image

Interactive music player at work:
https://www.youtube.com/watch?v=ftYVhMpBk4Y

** DISCUSSION **

This PR has not gone through a proposal as I often do, for the most part because the original work by Daniel Matarov is much older than the proposal system.
So I would not merge as-is without more discussion. The points I would make would be:

  • I think the improvements to the audio workflow with music files definitely need to get merged. Those need to be core and are very useful for building more complex systems on top, be it as extension or included with the engine.
  • I am not convinced with the interactive music player. I think its a good proof of work but ultimately would merge this PR without it and write a proper proposal later on how I would expect it to work to get feedback.

Here is the test project I made to the interactive music player in case you want to play around with it, though:

intmusic.zip

@reduz reduz requested review from a team as code owners July 10, 2022 19:58
@reduz reduz marked this pull request as draft July 10, 2022 20:01
@Calinou Calinou added this to the 4.0 milestone Jul 10, 2022
@ellenhp
Copy link
Contributor

ellenhp commented Jul 10, 2022

Exciting! I can review this as soon as July 19th. I've been very busy lately with chores and a job search. I would like to review this but unfortunately I can't do any sooner.

@fire
Copy link
Member

fire commented Jul 10, 2022

I have invited @EIREXE to review the BPM code because EIREXE works on https://store.steampowered.com/app/1216230/Project_Heartbeat/ and EIREXE previously wasn't able to detect bpm except externally.

@DrCanvas
Copy link

Added an advanced importer for OGG and MP3 files that allows seting the above information with more ease.

Can we also add this feature for WAV in the future?

@IntangibleMatter
Copy link
Contributor

I think something that, if it hasn't been added yet, must be added is signals that let you know about the following events:

  • Beginning of Bar
  • Beginning of Beat
  • Half Beat (although this may not be necessary because of Quarter Beats. This would generally mean eighth notes)
  • Thirds of Beats (in case you're doing a rhythm game which requires triplets)
  • Quarter Beats (What most rhythm games use. Ex: Friday Night Funkin'. This would generally mean sixteenth notes)

@pibolib
Copy link

pibolib commented Jul 11, 2022

I think that additional information that defines time signature and tempo changes would be very important as well.
Perhaps, piggybacking off of what @IntangibleMatter said, maybe a way to define a number of steps per beat (and changes in that) would be good to subdivide a beat for signals. It would accomplish the job of the half beat, thirds, and quarter signals, and could be dynamically changed to accommodate for different complexities.

@reduz
Copy link
Member Author

reduz commented Jul 11, 2022

@IntangibleMatter @pibolib Signals for these things are difficult because the audio thread/playback is not necesarily in sync with the game thread. At much I think what can be done is querying where the playback currently is in amounts of beats/bars so you check every frame.

@ellenhp
Copy link
Contributor

ellenhp commented Jul 11, 2022

In addition to the above, I don't know if there's a real-time friendly way to emit a signal. We could add one with a one-off use of SafeList, but that would be hacky. I think it would be a big convenience to add these signals, I just don't know how to make it work in a way that would respect the latency of the audio mixing system and driver.

@IntangibleMatter
Copy link
Contributor

@reduz It occurs to me that a solution for this, albeit one that would require more than a bit more effort, would be to add a _music_sync_process(beat: int) function to be called. This would be one that is called on every 'beat', or whatever other denomination we could decide on, and include info about what the current beat is. I fear this may interfere with _process() and the main thread in a way, but in my opinion it would be worth it to make it so that you could just perfectly sync the music and gameplay. Make Godot an engine that makes rhythm games that much easier. Might even encourage people who aren't doing rhythm games to add rhythm-based features (ex. The beat blocks in Celeste.)

This might make the engine 3x more complicated, but I'm always going to advocate for first-class on the beat support.

@lapspider45
Copy link

This looks like a big improvement. Having a single tempo/time signature per track is limiting though. Support for tempo changes would be amazing, even better if you were able to import the tempo map from a .mid file.

@reduz
Copy link
Member Author

reduz commented Jul 11, 2022

@lapspider45 If you see the video above, it does tempo changes without much issue.

@Ansraer
Copy link
Contributor

Ansraer commented Jul 11, 2022

So I haven't really done anything audio-related so far (I am pretty much tone deaf) but something I noticed immediately while watching the WIP video is that it looks like only one clip can be played at once by the AudioStreamPlayer.
Would support for playing multiple clips at the same time be part of the interactive music player you want to write a proposal for?

@pibolib
Copy link

pibolib commented Jul 11, 2022

This addon implements signals for beats and bars, it may be useful for reference in a potential signal implementation for this system. I can understand why subdivisions of beats may be difficult to pull off given how Godot works currently though. If signals can only be sent out once per frame, then even small subdivisions at a moderate BPM would fall out of sync with the music.

I know that some rhythm games tackle the problem of only being to act at the display rate by increasing the target frame rate to something several times the display frame rate (for input accuracy), but if I remember correctly this is not something that was being considered for implementation in Godot.

@lapspider45
Copy link

@lapspider45 If you see the video above, it does tempo changes without much issue.

@reduz does this mean one clip/file can have built in tempo changes (say you make the chorus 3 BPM faster, or even record without a metronome and later make an accurate tempo map with a precise BPM change every few bars), or that tempo changes are possible using multiple clips?
The former would be much more flexible and simpler especially when working with very tempo-variant material, but glancing at the code and the video I only see indication to the latter.
(Tempo map support isn't trivial to implement, so I can see this maybe needs its own proposal. But it would be very useful to have.)

@fire-forge
Copy link
Contributor

I don't work with sound very much, but these new features seem really cool!

This also closes #62284

@IntangibleMatter
Copy link
Contributor

I think something to look at in terms of features to make the music system even stronger would be the music features here. I know it's for GMS2, but it allowed strong dynamic music for games like Chicory: a Colourful Tale and Wandersong.

Something specific that occurs to me is the ability to define lengths of transitions. As an example, Chicory has a piece of music with about 7 pieces in it. Loops smoothly transition into oneshot parts, mostly by defining "intro" and "outro" lengths in beats.

I think what would make smooth, more complex transitions easier is this:

  • defining transition/cross fade length
  • the ability to define the length of in/out transitions for oneshot segments

This PR reworks the Godot sound sytem to improve the support for doing things with music content.
* Adds BPM/Bar/Beat Length to some types of AudioStream
* Implements proper beat-based looping for music files (current looping support is poor).
* Added an advanced importer for OGG and MP3 files that allows seting the above information with more ease.
* Improves the visualization of sound resources, including showing sound playback position while in use.

As a bonus, ported and reworked the interactive music player from godotengine#32769.
Not finished but works as a test of the above features.

Some screens:

Importing OGG/MP3 files, advanced settings:

Interactive music player at work:
https://www.youtube.com/watch?v=ftYVhMpBk4Y

** DISCUSSION **

This PR has not gone through a proposal as I often do, for the most part because the original work by Daniel Matarov is much older than the proposal system.
So I would not merge as-is without more discussion. The points I would make would be:

* I think the improvements to the audio workflow with music files definitely need to get merged. Those need to be core and are very useful for any more complex system to use on top.
* I am not convinced with the interactive music player. I think its a good proof of work but ultimately would merge this PR without it and write a proper proposal later on how I would expect it to work to get feedback.

Here is the test project I made to the interactive music player in case you want to play around with it, though:
reduz added a commit to reduz/godot that referenced this pull request Jul 23, 2022
Based on godotengine#62896, only implements the BPM support part.

* Implements BPM support in the AudioStreamOGG/MP3 importers.
* Can select BPM/Bar Size and total beats in a song file, as well as edit looping points.
* Looping is now BPM aware
* Added a special importer UI for configuring this.
* Added a special preview showing the audio waveform as well as the playback position in the resource picker.
* Renamed `AudioStream::instance` to `instantiate` for correctness.
@reduz
Copy link
Member Author

reduz commented Aug 8, 2022

I will do another PR with just the music support and close this one eventually

@Calinou
Copy link
Member

Calinou commented Aug 16, 2022

Superseded by #64488.

@Calinou Calinou closed this Aug 16, 2022
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.