Your Preferred Python Writing Style - Followup

Err, I feel dumb/embarrassed. For some reason I didn’t think of accessing the handle outside the class.

What do you mean by capabilities?

No worries. We’re working it out; it’s a good thing.

Is this requirement satisfied by what we currently have?

Well, I need a way to control all the sounds at once. So I would be able to call a method to change the volume of all the sound handles, or a way to stop every sound from playing, or to pause every sound at once.

I’m assuming that’s after though.

So yes, I’m happy with what we currently have!

Ok, build that, as best you can.

Hi. Sorry for the long delay, I’ve had a lot of work lately.

So here’s what I’ve come up with (.blend):
audio_manager.blend (404 KB)

The scripts should be self explanatory. The methods in Factory.py are there for temporary convenience.
I have an AudioManager script which is trying to imitate a static class. It contains a list of all factories. I append new factories through the Factory object itself.

I append the new factories in the play method in the Factory class. I’m starting to think I should do it in initialization because if I hit the play method again on the same factory, I’d append the same handle into the factory list. Then again, when will I ever hit the play again on the same factory? On the other hand, if I do it in the class’ init, then I can’t do clean up properly. I want to remove all unused factories from the factory list; the ones with a false handle status. In other words, the stopped handles.

I don’t know how to prevent repetition either. In AudioManager.py, I also want to have a pause and resume method but that means I’ll have to use the same for loop over.


for factory in all_factories:
      do whatever

Anyway to prevent this repetition?

Edit:

I just noticed that I’m adding the factory twice because I use the play method in the initialization. Maybe I should add the factory to the static list in init instead of the play method. Or maybe there’s another way to access all factories without making a static list. ?

There is really no need for AudioManager to be a “static class”, or anything like that. Initially, our design focused on nothing more than a simple dictionary containing our custom Factory instances -> We should find a way to preserve that conceptual simplicity:


class AudioManager(dict):

    def __init__(self, audio_file_paths):
        for p in audio_file_paths:
            self[name_from_path(p)] = Factory(p)

You should really put both Factory and AudioManager in the same module (audio.py sounds like a fitting name). You can then have an init/config script, which will run before anything else, to create the logic.audio object:


import audio # your module
logic.audio = audio.AudioManager(audio_file_paths)

General control methods can (and should) be added to the AudioManager. If you study the audaspace api docs, you’ll notice that there is a global stop method on the Device, along with a global volume control. So, for that, you don’t need to keep track of handles, and you definitelly don’t need to append Factory instances to some module defined list.

For pause, you should test if the device lock/unlock methods do the trick. If not, you’ll have to keep track of handles, on the Factory, because that’s the object responsible for all playing sounds of that type. If AudioManger wants to do something with the handles, it should get them from the Factory.

As for how the Factory is supposed to track handles: Well, self.handle represents the last generated instance of a given sound, so self.past_handles can hold earlier instances.

Of course, this means that you’ll have to clean-out self.past_handles from time to time, but you can do that without running a loop on every tick: The play method can be modified to check len(self.past_handles), and run the proper cleanup when the count is greater than some predefined number (10 sounds reasonable).

Oh, and don’t do this:


for h in self.past_handles:
    if not h.status:
        self.past_handles.remove(h)

I’m pretty sure that remove has to search the list for the given element. Also, when you remove something from anywhere but the end of the list, the elements that follow have to be moved down, to preserve order.

Try this:


self.past_handles = [h for h in self.past_handles if h.status]

It’s much cleaner, and possibly faster for our general execution patterns.

Sometimes I don’t want to stop all sounds from playing though. I also want a way of stopping all sounds from a specific group. For example, stop all sound effects, or stop all background music, or stop all sounds coming from an enemy AI (this would be a subgroup under the sound effect group).

So the reason I manually made a global factory list was to separate different sounds from each other. It was to give me control on grouping sounds. Is there a better way to group sounds?

Well, in that case, you should keep track of the handles, as previously described.

As for categories/groups: you can just create two dictionaries on AudioManager, called sounds and music. You could do something similar for any other group you want to specify, but keep it reasonable: The sounds generated by a given game object should be controlled on that object.

If I have a monster object that typically growls, and I want to make it mute, I get that object, and disable the machinery that actually plays that sound on the object.

Right. So, making a group for enemy AI isn’t really reasonable because I could just loop over the enemy objects in the object list and mute them off there.

I’m not understanding what you mean by tracking handles… Mostly the past handles list. How does this allow me to pause/resume groups of factory handles?

And how do I add factories for the dictionaries on AudioManager; what keys? Why shouldn’t I just use a list?

When you call play on a Factory, you create a handle that controls the newly generated sound instance. That handle is stored in the handle property, on our Factory object. You can call play multiple times, on the same Factory, to create new instances of the same sound. If you do that fast enough, you hear those instances overlap, as expected. However, since you’re only storing the most recently generated handle, you can only control the most recent instance of that given sound. Previous instances, which may still be playing, are completely beyond your control, because you didn’t keep their handles.

That’s the purpose of the past_handles list; You keep those handles, so that you can control previous instances of a given sound, which have yet to finish.

As for grouping: music is in the music dictionary, sounds are in the sounds dictionary, and that defines your groups.

You can iterate through one, or both, and do whatever needs to be done to the handles on each Factory.

And how do I add factories for the dictionaries on AudioManager; what keys? Why shouldn’t I just use a list?

In the same way we did before: name of the given sound maps to a Factory that can produce instances of that sound (controlled by the generated handles).

If we used a list, we wouldn’t have that mapping, and we wouldn’t know which Factory represents which sound file.

Does that make sense?

Ok, I was just confused. Yes that makes sense!

Two more things:
Instead of having the current handle and the past handles in the Factory class, why not just have one list of all the handles being used?

Also, isn’t there going to be a lot of repetition if I keep looping from the dictionaries to pause, stop or resume?

Thanks!

I reasoned that controlling a single handle would be the typical use case, and that therefore a main “handle” property would make sense in that context (and we wouldn’t have to write wrappers for control methods already on the handle, if we can just use it directly).

However, now that we have multiple handles, we typically want to control them in bulk (for a given Factory), so it’s probably better to have a single handles list, along with stop, resume, and pause on the Factory, to loop over the handles, and call the corresponding method.

So yea, that would be better - good thinking.

Also, isn’t there going to be a lot of repetition if I keep looping from the dictionaries to pause, stop or resume?

Well, you’re going to write proper abstractions, of course:


audio.music["track"].play()
audio.music["other_track"].play()

# Pause all instances of "track":
audio.music["track"].pause()

# Pause all music:
audio.music.pause()

Same structure for sound.

I don’t know how to make the abstractions.

I’m guessing I’d need something like this?



class FactoryDict(dict):
     def stop(self):
          for f in self.vaules():
               f.stop()


So in AudioManager:



class AudioManager:
     def __init__(self):
          self.music = FactoryDict()


And in the Factory class Id need:



def stop():
     for h in self.handles:
          h.stop()


so that I could do:



AudioManager.music.stop()


Is this what you mean?

Yes, basically that’s it, but I would use the following semantics:

# audio.py

class Factory:

    def __init__(self, path_to_audio):
        self.aud_factory = aud.Factory(path_to_audio)

        self.handles = []

        self.play()
        self.stop()
    
    def play(self):
        device = aud.device()
        handle = device.play(self.aud_factory)
        self.handles.append(handle)

        # Periodic clean-up algorithm:
        l = len(self.handles)
        if l and l % 10 == 0:
            self.handles = [h for h in self.handles if h.status]

        return handle

    def stop(self):
        for h in self.handles:
            h.stop()

    def pause(self):
        for h in self.handles:
            h.pause()
   
    def resume(self):
        for h in self.handles:
            h.resume()


class Group(dict):

    def __init__(self, file_paths_audio):
        for p in file_paths_audio:
            self[name_from_path(p)] = Factory(p)
    
    def stop(self):
        for f in self.values(): 
            f.stop()

    def pause(self):
        for f in self.values():
            f.pause()

    def resume(self):
        for f in self.values():
            f.resume()


class Manager:

    def __init__(self, file_paths_music, file_paths_sounds):
        self.music = Group(file_paths_music)
        self.sounds = Group(file_paths_sounds)

And then you can initialize it like this:


# init.py

from bge import logic
import audio

def main():
    logic.audio = audio.Manager(file_paths_music, file_paths_sounds)

PS: Excuse the poor indentation - BA seems to format every tab down to something like 2 spaces. Hopefully you won’t have too much trouble reading the code.

Alright, that’s what I thought.

Here’s what I have for audios.py:


import aud,os, random
from bge import logic
import PATHS


class Factory():
	def __init__(self, path_to_audio):

		self.aud_factory = aud.Factory(path_to_audio)
		self.handles = [] 
		self.play()
		self.stop()
		
		
		
	def play(self):
		device = aud.device()
		handle = device.play(self.aud_factory)
		self.handles.append(handle)
		
		l = len(self.handles)
		if l and l %10 == 0:
			self.handles = [handle for handle in self.handles if handle.status]
		
	def stop(self):
		for handle in self.handles:
			handle.stop()
			
	def pause(self):
		for handle in self.handles:
			handle.pause()
	
	def resume(self):
		for handle in self.handles:
			handle.resume()
			
			



class Group(dict):
	def __init__(self, file_paths_audio):
		for file in os.listdir(file_paths_audio):
			self[file] = Factory(file_paths_audio+file)
	
	def stop(self):
		for factory in self.values():
			factory.stop()
	
	def pause(self):
		for factory in self.values():
			factory.pause()
			
	def resume(self):
		for factory in self.values():
			factory.resume()
			
	

class Manager:
	def __init__(self, music_file_path, sound_file_path):
		self.music = Group(music_file_path)
		self.sound = Group(sound_file_path)
	

#TESTING
	
def init(cont):
	logic.Manager = Manager(PATHS.MUSIC,PATHS.SOUNDS)
	
def main(cont):
	logic.Manager.music['rain.ogg'].play()
	logic.Manager.music.stop()



I’m wondering, for the self.handles length check, why are you doing:


if l and l%10 == 0:

instead of


if l > 10:

Why check for a non-zero length and a zero remainder by dividing by ten if you can just check if the length is greater than ten?

Imagine a rapid fire scenario, where even after the 10th handle is added, all the sounds are still actively playing, and there is nothing to remove. You don’t want to run the cleanup routine on every subsequent play from that point on. Instead, you want to wait until you have 10 more, by which time the previous 10 will probably become inactive, and ready for cleanup.

Other notes:

The module name should be audio, not audios.

The instance should also be audio, not Manager (even in testing).


# This:
[h for h in self.handles]

# Is no less expressive than this:
[handle for handle in self.handles]

# But it is easier to read and shorter to type ...

Don’t use the full filename as the key. Ditch the extension, so that you don’t have to type it over and over again. Just use the name of the song.

Also, don’t forget that you can alias (make shorthand variables) when convenient:


m = logic.audio.music

m["rain"].play()
m["shine"].play()

Ok, so now, if this covers all your previous requirements, we can move on to animation/frame based sounds:

I don’t think you should add those directly to the audio system, because I think it belongs on the object. You should have a kind of sound component that will simply play the right sounds on the right frames.

Any thoughts on how to do that? Give it a try, and show me what you come up with.

So I finally got around to trying this, sorry for taking so long.

I decided to use iterators! Like you showed me in that Sprycle video.


from bge import logic, types
import random as rand
import itertools as it


class Frames:
	def __init__(self,frames,loop=True):
		if loop:
			self.frames = it.cycle(frames)
		else:
			self.frames = it.chain(frames+['end'])
		
		self.frame_list = frames
		self.current = None
		self.last = None
		
		self.next()
		
	def next(self):
		self.last = self.current
		self.current = next(self.frames)
		
		

class FrameAudio:
	def __init__(self,frames,factories,layer):
		self.factories = factories
		self.frames = frames #iterable
		self.layer = layer
		
	def __eq__(self,other):
		if isinstance(other, FrameAudio):
			return self.layer == other.layer and \
		self.frames.frame_list == other.frames.frame_list and \
		self.factories == other.factories
		return False
		

class Entity(types.KX_GameObject):
	def __init__(self,own):
		self.frame_audio_dict = {} #Frame Audio
		
		self.armature = self.parent
	
	
	def play_on_frame(self, name, frame_audio):
		
		if not name in self.frame_audio_dict.keys():
			self.frame_audio_dict[name] = frame_audio
		
	
	def manage_audio(self):
		
		print(self.frame_audio_dict)
		for key in self.frame_audio_dict:
			if self.frame_audio_dict[key].frames.current != 'end':
				self.frame_audio_dict[key] = self.frame_audio_dict[key]
			else:
				del self.frame_audio_dict[key]
		
		for fa in self.frame_audio_dict.values():
			current_frame = self.armature.getActionFrame(fa.layer)
			if not isinstance(fa.frames.current,str):
				if fa.frames.current > current_frame-0.5 and fa.frames.current < current_frame+0.5:
					logic.Manager.sound[rand.choice(fa.factories)].play()
					fa.frames.next()
		
		
	
	def main(self):
		self.manage_audio()
		
		
		#Testing purposes...
		if not 'a' in self:
			walk = FrameAudio(Frames([10,20]),['foot1.ogg','foot2.ogg'],0)
			self.play_on_frame("walk",walk)
			self['a'] = True
		

def main(cont):
	player = cont.owner
	if not 'init' in cont.owner:
		player = Entity(cont.owner)
		player['init'] = True
	
	player.main()


This setup still has a problem… I can’t loop one frame for one sound. For example, if the list of frames had only one element the ‘next’ method wouldn’t change the iterator (x = [10], next(x) = 10, next(x) = 10, etc). As you can imagine, the same sound would play multiple times on the same frame. Any way to fix this so it only plays once per animation cycle?

Please tell me what you think.
It would be great to continue getting criticism from you.

Thanks.

Well, I don’t think this is the right context for iterators, because (if I understood correctly) the original goal was to make a system that can play a specific sound on a specific frame, so it’s fundamentally about mapping, not about managing some indefinite sequence.

With that in mind, the code you posted seems needlessly complicated, because you don’t really need anything more than a dictionary:


# when frame changes
if frame in frame_sound:
    frame_sound[frame].play()

Using strings, deleting keys, and those additional classes … It’s unnecessary.

Also, whenever you find yourself typing something absurd, like: “self.frame_audio_dict[key] = self.frame_audio_dict[key]” -> You need to rethink your approach, because you’re literally writing code that does nothing.

So, try again, with all that in mind.

Let me just clear up what I’m trying to do (I’ll be redoing the code anyways: variable = variable … face palm)

I don’t know if I’m understanding what you mean, but using one dictionary suggests that only one sound is played on the specified frame. What if I wanted to play more than just one sound on the same frame? Would it be better to fuse the two sounds outside of the bge? What if two sounds happen to be playing on the same frame, and wasn’t expected?

Maybe the dictionary can have more than one sound per key?



frame_sound[frame] = [[step1,step2],[slash1,slash2]]
#Where one random sound is picked from each list of factories.

and then punch in your code in the sound component of the object.

Is that a good solution?

But what if I only wanted the sound to play on specific frames in a certain state? Or what if I wanted to play a sound once on a few frames, through one cycle of an animation? How would I stop sounds from playing outside their states?

Well, if they really need to play on the exact same frame, then they should be one sound, representing one event … I find it really difficult to imagine a scenario where multiple sounds need to play on one specific frame, for any one animation.

I mean, things can play pretty close together, like “step” on frame 20, and “slash” on frame 21, which would basically give you the same effect.

What if two sounds happen to be playing on the same frame, and wasn’t expected?

Expected by what/who?

Whatever plays, plays - Expectations don’t really factor into the process.

… Or maybe I misunderstood?

Maybe the dictionary can have more than one sound per key?

It could, but that would also make the structure more complicated, which leads to trouble.

The base case is one sound per frame, and you should implement that first. For things like randomization, you can actually have functions:


fn_step = lambda: random.choice([step1, step2])
frame_soundfn = {20: fn_step, ...}

if frame in frame_soundfn:
    sound = frame_soundfn[frame]()
    sound.play()

However, you should seriously ask yourself if it’s even necessary? Generally, people don’t even notice the randomization.

If the benefits are not significant, it’s usually better to stick with a simpler implementation.

But what if I only wanted the sound to play on specific frames in a certain state?

You would have a corresponding state that plays those sounds.

Or what if I wanted to play a sound once on a few frames, through one cycle of an animation? How would I stop sounds from playing outside their states?

The component that controls sound playback, on the relevant object, would essentially be a state machine, with states that correspond to other logic states.