godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.14k stars 93 forks source link

Add AudioStreamPlayer "play_start" signal #3977

Open QueenOfSquiggles opened 2 years ago

QueenOfSquiggles commented 2 years ago

Describe the project you are working on

An addon for generating spatial, and character dialogue subtitles using the built-in AudioStreamPlayer and the 2D/3D counterparts. This addon would then be used in my horror games which involve listening for the movements of a creature that is actively hunting the player similar to "Alien: Isolation". This addon is being developed under MIT license so it can be used to improve accessibility in Godot games

Describe the problem or limitation you are having in your project

There is a signal on _AudioStreamPlayer_s for "finished" but not "started" (for when the audio begins to play). Because of this, I need to use the process function to check for every audio stream player in the game to see if "playing" is true and the cached value from last frame is false to determine when to add the subtitle to the screen.

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

This signal would drastically reduce the processing load for the subtitle system especially for games that use a large number of sounds that have subtitles attached.

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

Ideally this would be added to the AudioStreamPlayer as well as AudioStreamPlayer2D and AudioStreamPlayer3D

#...
signal started
signal paused
#...

func play():
    ...
    emit_signal("started")

I wrote this in GdScript but ideally it would be part of the engine, not an extended script

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

This enhancement can be reproduced through my already mentioned work-around, however, every single AudioStreamPlayer would be running the process function every frame. The other work-around is to make an extended script, but that would break the intention of the addon, which is to integrate with any custom AudioStreamPlayers which may be introduced with other addons, such as Godot-SFXR, an addon which extends AudioStreamPlayer to produce new and interesting playback options.

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

This is a feature which would improve the ability for developers to add subtitle systems to their games. Subtitles are a very basic, crucial element for game accessibility. Without subtitles, many games made with Godot are unplayable for anyone who is hard of hearing or deaf. This small change would open up worlds of possibility for accessibility features in games, not just limited to my personal subtitle addon project.

Mickeon commented 2 years ago

I at first thought this was a generally needless addition to most games, but the accessibility argument is kind of compelling.

Because of this, I need to use the process function to check for every audio stream player in the game to see if "playing" is true

Do not be afraid to extend and attach a script for every AudioStreamPlayer that needs subtitles to be shown:

signal started

func play():
    .play()
    emit_signal("started")

Would work just fine for this purpose, but it could be some extra work to set up.

Mickeon commented 2 years ago

For the subtitles example specifically, warping your thinking a bit may also solve the problem. For example, the "Subtitle Manager" being an Autoload, thus a globally accessible reference that the AudioStreamPlayers with custom scripts may directly access, instead.

Adding this new signal would probably warrant adding "paused", "resumed" as well. Although, I find sounds to be additive to the experience. Usually, something triggers sounds to play, and they're not responsible for much game logic... Except, and notably, subtitles.

QueenOfSquiggles commented 2 years ago

@Mickeon I took your recommendations into account for my addon. I was already using the singleton pattern for managing subtitles. I do like being able to have event driven AudioStreamPlayer nodes. I still get the feeling that this feature would be beneficial to more than my use cases, especially considering the addons that add extensions of the AudioStreamPlayer nodes. For now, my use cases are covered without any feature change.

TheDuriel commented 2 years ago

Do not be afraid to extend and attach a script for every AudioStreamPlayer that needs subtitles to be shown:

This is in fact 0 effort. Since you can easily make a class with a class_name that you then use instead of audiostreamplayers in your project, adding this functionality to all of them.

This only takes a few seconds.

QueenOfSquiggles commented 2 years ago

@TheDuriel The issue with this approach is I am trying to make an addon for handling subtitles. I wanted to make it easy for users of my addon that may have already completed a large portion of their game already, but wanted to add subtitles to add accessibility. My original solution was to create extensions of the core AudioStreamPlayer nodes, but that would require that someone using my addon exclusively use my nodes instead of what works best for their game. I did follow @Mickeon 's suggestion which solved the problem for any AudioStreamPlayers being used in a game that don't otherwise need a script.

The signal seems like something that should logically be there. I was surprised that it wasn't.There is a signal for when an audio finishes, but not for when it starts. The observer pattern is intended to allow for decoupling of dependencies, which for most projects is ideal. Moreover,

I feel that this feature itself would take minimal effort. Probably a good "first time contributor" kind of feature. It would also make it incredibly easy for users to make a subtitling system for their own games, with more customized features than with my addon. In terms of accessibility, I think we should be making it as easy as possible for game developers to add accessibility to their game(s). This feature would be a nudge in the right direction

Calinou commented 2 years ago

In terms of accessibility, I think we should be making it as easy as possible for game developers to add accessibility to their game(s). This feature would be a nudge in the right direction

If you want to make it easy for developers to add subtitles, having a complete plugin for doing so is likely a better idea. There's a lot of effort that goes into creating an useful subtitle system, and only doing a small part of the work (audio signals) will not automatically result in a good subtitles implementation from the developer's part. See also https://github.com/godotengine/godot-proposals/issues/983.

Mickeon commented 2 years ago

It's true that other Objects having a "started" signal of sorts are more likely to be involved with gameplay logic, but a new signal would help decoupling audio-related logic from the rest of the game. Subtitles could be one application, but aesthetic or acoustic effects, such as generating a wave, or dynamically filtering and redirecting audio, regardless of why the audio is being played could be others, as well.

Although, to be fair, adding the signal manually isn't inherently difficult by itself. In fact, just extending a script for it may allow (However possibly annoying it could be to attach everywhere in the middle of a project's development) more control for the features described above by incorporating them within the script (although they may not be able to coexist).

I also believe, however, that this would be a minor good step towards accessibility. The easier it is to implement features like subtitles, the more likely developers are going to make use of them.

TheDuriel commented 2 years ago

To look at this a different way: Why would, in any case, the AudioStreamPlayer be responsible for triggering Subtitles?

Would there not be a Dialogue, Conversation, or other system, responsible for selecting the correct voice line based on the language to play. And would this system not also forward to your subtitle display the corresponding subtitles?

Reacting to a sound being played, then matching that to a subtitle key just, appears backwards to me.

If you are calling .play(), then you are doing so from whatever place you could also handle any reactionary stuff from.

func play_voice_line(voice line):
    audiosystem.play_sound(voice_line.sound)
    subtitlesystem.show_subs(voice_line.text)
Mickeon commented 2 years ago

To look at this a different way: Why would, in any case, the AudioStreamPlayer be responsible for triggering Subtitles?

It would be good, in fact, to have an entirely independent system, external of the AudioStreamPlayer, manage subtitles in a simple and easily integrable way.

And it seems that, upon taking a further look, this is how the addon OP is working on aims to be like. With the additional, albeit minor caveat that it currently requires an additional Script just to connect the "started" signal, else it would require checking the interested AudioStreamPlayers' "playing" flag every frame. However...

... that would break the intention of the addon, which is to integrate with any custom AudioStreamPlayers which may be introduced with other addons, such as Godot-SFXR, an addon which extends AudioStreamPlayer to produce new and interesting playback options.

@QueenOfSquiggles One way to possibly mitigate this is by instructing your addon's users to rewrite their other Audio addons' classes to extend your own custom Script with the "started" signal.

winston-yallow commented 1 year ago

I'd be willing to have a look at this and maybe implement it.

I'm also making a plugin for subtitles, and would like people to be able to just use normal stream players. I want the setup for the user to be as easy as possible, so that hopefully more people add accessibility options like subtitles to their games. Custom subclasses of AudioStreamPlayer would kinda work, but this quickly becomes a problem when users have other plugins with their own subclasses of AudioStreamPlayer. Those then won't work with the subtitles plugin. Therefore a built in signal would be very useful to just support every kind of stream player.

I think adding these signals doesn't require many code changes, and the cost of maintaining this feature should also be pretty low. So I think it's warranted to add these signals to core. It's a small change, but useful.

Edit: after some discussing on rocket chat, I will not work on this. Other approaches are better suited to solve the issue.