-
Notifications
You must be signed in to change notification settings - Fork 458
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
Added FlxSoundGroup for managing volume of music/sfx/speech etc separately, closes #362 #1316
Conversation
By default, sounds don't belong to a group it seems... What do you think about flixel setting up a Overall, this seems like a very flexible yet lightweight feature. Not sure about the string-identifiers, though... |
I'm not sure about the string identifiers, either. It seems that something like it might be necessary in order to avoid users having to keep global references to their groups, though. For defaults, I'm wondering about having a defaultGroup and defaultMusicGroup that can be changed? I'm not super keen on the existing FlxG.sound.music, mind. It doesn't seem to be useful for anything but the most basic case where music is always mutually exclusive. In my current project, music transitions, and certain events trigger a short musical piece over the top of the background music, and these would certainly be classed as music when determining their volume. I'm wondering if maybe the single FlxG.sound.music could be replaced with a music group if FlxSoundGroup is improved a bit more? |
"Music group" sounds weird. But Music/Sounds groups are useful for:
You can do all that without the groups, but it will make life a bit more simple. |
for (sprite in _sprites) | ||
{ | ||
if (sprite != null && sprite.exists && sprite.visible) | ||
if (sprite != null && sprite.exists && sprite.visible && sprite.isOnScreen(Camera)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you changing this logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the old way was inefficient. My way produces the exact same result, except it breaks out of the loop as soon as it is clear the result will be true. In the old way, even after the result can be nothing but true, it still loops through every other sprite in the sprite group.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Besides, it's more reabable. :)
I don't see any logic in the sound group to handle pre-loading of sound assets. This seems like one of the main reasons for using sound groups. |
Should we wait for pre-load sound functionality or merge this in and add it later @Gama11? |
I have an old snippet from the forum by @Hasufel, it's pretty straight forward: //preloading
var soundBank:Array<Sound> = new Array<Sound>();
var soundList = ["sound1.ogg", "sound2.ogg"];
for (i in 0...soundList.length) soundBank.push(Assets.getSound(soundList[i]));
//while in game
soundBank[n].play(); //n being the sound u need |
This pull request adds a new class FlxSoundGroup, so pre-loading sounds through that class should be even easier. |
Added ability to set default groups. |
@gamedevsam @sruloart I'm not sure how sound groups would be useful for preloading sounds. There already are |
@@ -490,10 +495,12 @@ class FlxSound extends FlxBasic | |||
/** | |||
* Call after adjusting the volume to update the sound channel's settings. | |||
*/ | |||
private function updateTransform():Void | |||
public function updateTransform():Void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should use @:allow()
here instead of making the function public.
@Gama11 I'm not saying this right here is the complete answer, I was just describing what music/sound groups are good for :)
Does that make sense? |
I see what gama is saying... loading (caching) and playing sounds is mostly handled through one static location, the SoundFrontend, like so:
You can't really use FlxSoundGroups to manage caching of sounds, since you need an FlxSound to add to the group, and calling
|
*/ | ||
public function getGroup(name:String):FlxSoundGroup | ||
{ | ||
return groups.get(name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The more I think about this change the more I want to automate this functionality. Something like:
public function getGroup(name:String):FlxSoundGroup
{
if(name != "")
{
if(!groups.exists(name))
groups.set(name, new FlxSoundGroup());
return groups.get(name);
}
That would eliminate the need for users to spawn sound groups manually and would simplify their usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, was wondering about this. Probably would be a good addition. :)
We can discuss the all sounds/assets management deal later, I for one welcome the new FlxSoundGroup in open arms :) |
@Gama11: you can merge this if you want, I'm not a big fan of how it's implemented, but I don't have a better suggestion atm. |
I'm unsure as well. |
The only alternative I can think of would require a complete overhaul of the front ends stuff to make it easier to extend. I'm not particularly keen to do that myself at the moment. If you main reservation is the use of the string identifiers, then perhaps consider that it is pretty much the same as what we have for sprite animations . . . |
Any opposition to this change? It's been lingering for a while, seems like we should merge it and improve it as necessary. |
I snuck another small modification in here, too. Just making a for loop more efficient.
FlxSound
now has a group. You can set a sound's group either by adding the sound to the group, setting the group of the sound, or by passing the name of a group toplay()
,load()
orplayMusic()
.Presently groups must be added to the
FlxSoundFrontend
by callingaddGroup
.Example use: