Closed SolaceEllery closed 1 year ago
The Audio namespace additions mostly seem good for what they are. Aside from giving it a little test, a few little things that come to mind that should probably be worth adjusting:
MusicReset
doesn't seem like a good name to me. It prompts the question of what about the music it's resetting. Better names would be MusicRestart
or MusicRewind
. I'm leaning to thinking MusicRestart
may be most clear.MusicChannelGet
is an awkward name. It prompts the question of what about a music channel is it getting, and maybe even implies getting some sort of object representing a channel, which doesn't fit with what it actually does. A better name would be something like MusicGetNumChannels
or MusicGetChannelCount
LuaProxy::Audio::*
names match the name you've given the Lua function. There's no good reason for it to differ.I was thinking about it some more, and actually I don't think I'm happy with the "Channel" naming after all.
I originally didn't like "track" because it potentially conflicts with some future goals of handling multiple music tracks at once in a different way... but "channel" is bad too
Track may be best after all though... and I see Mixer-X has used "stream" to represent the more general way of having multiple music tracks at once, which I suppose is good enough...
So I think, I'd propose the following final renaming:
MusicRestart
-> MusicRewind
MusicGetChannelCount
-> MusicGetTrackCount
MusicChannelMute
-> MusicTrackMute
MusicChannelUnmute
-> MusicTrackUnmute
There is one concern I do still have with "Track" though... Mixer-X seems to be deeply conflicted about what "Track" means in it's design.
In Mix_MusicInterface
there is both the pair GetTracksCount
/SetTrackMute
, and also GetNumTracks
/StartTrack
. The former of which refers to tracks in one music file that can play concurrently, whereas the latter appears to refer to separate songs in one music file. Very different concepts, and using "track" for both seems like a problem.
It looks to me like it's mostly MIDI and GME codecs that implement GetNumTracks
/StartTrack
... but most of the ones that implement that also implement GetTracksCount
/SetTrackMute
too, making it doubly confusing.
The Mixer-X wiki documentation also uses "track" to refer to both things too.
The the current+proposed Lua APIs only provide an interface to GetTracksCount
/SetTrackMute
, so for purposes of this pull request we don't have a direct conflict yet... but seeing this conflict directly inside Mixer-X gives me pause about settling on terminology. @Wohlstand: Is there a plan to resolve this conflicting terminology in Mixer-X?
If I may chime in with an anecdote: Unity uses the concept of an audio mixer with "Mixer Tracks" to enable multiple songs playing at the same time. It might make sense to apply a similar thought process when dealing with sound files here, default applying the main song to a Master Track and creating nest-able sub-tracks that can be modulated like on a mixer rack in a DAW. What I'm getting at is that these Mixer Tracks have a lot of functionality that goes beyond playing one audio source and muting/unmuting it. In the future we might want to support panning, pitch shifting, being sent to an arbitrary other mixer (rather than Master) or mixer track effects like echo via an audio effects API. With that in mind, while it may blow the scope on this PR, I wonder if a handful of new functions in the Audio namespace is the right approach, or if preparing an AudioMixer struct now to make future implementations of extra features on a MixerTrack level easier is the way to go.
@enjlectric The MixerTrack concept you refer to maps much better to Mixer-X's "Stream" concept, than to what the GetTracksCount
/SetTrackMute
APIs refer to. In fact the "Stream" system looks like it can implement 100% of what you're thinking about with "Mixer Tracks" at least from how the API looks.
I think implementing support for multiple Mixer-X streams (which we could very well call "Mixer Tracks" if we wanted), would absolutely be a worthwhile endeavor, but is a FAR larger endeavor than this pull request, and is kinda orthogonal to this pull request in a way. The nature of overlap is:
The GetTracksCount
/SetTrackMute
APIs in Mixer-X were mostly designed for the case of MIDI/Tracker audio codecs, which gave an easy way to turn different channels within one stream on and off, but not do much else with them. They're much more limited than what you're thinking about. I'm tempted to call them "instrument channels" maybe.
I could see an argument that the instrument channels and such that GetTracksCount
/SetTrackMute
ought to get split out into separate streams... but I don't think that plays nicely with how the codecs are implemented under-the-hood. You could instantiate multiple streams, and make each mute all but one instrument channel, as a hack to emulate that capability, though that would add some extra overhead. We could of course opt to ignore these instrument channels in the interest of only supporting the full-featured streams, but that's not without it's downsides.
I think I'm now leaning toward:
MusicRestart
-> MusicRewind
MusicGetChannelCount
-> MusicGetInstChannelCount
MusicChannelMute
-> MusicInstChannelMute
MusicChannelUnmute
-> MusicInstChannelUnmute
unless I hear a good reason otherwise.While it's maybe a touch awkward, i think it's fitting because of how it:
Today, I decided to make a pull request surrounding SDL Mixer X. I added a few additional Audio functions to enhance X2:
Time Functions
Audio.MusicRestart() - Restarts the currently-playing music to the very beginning.
Channel Functions
Audio.MusicGetChannelCount() - Gets the total amount of channels used on the currently playing song. Audio.MusicChannelMute(value) - Mutes a specific channel from the currently playing song. Index starts at 0, not 1. Audio.MusicChannelUnmute(value) - Unmutes a specific channel from the currently playing song. Index starts at 0, not 1.
Speed/Pitch Functions
Audio.MusicGetTempo() - Gets the tempo of the current song. If -1, the song is unsupported. Audio.MusicSetTempo(value) - Sets the tempo of the current song. Check Audio.MusicGetTempo() before setting to see if it's compatible or not. Audio.MusicGetPitch() - Gets the pitch of the current song. If -1, the song is unsupported. Audio.MusicSetPitch(value) - Sets the pitch of the current song. Check Audio.MusicGetPitch() before setting to see if it's compatible or not. Audio.MusicGetSpeed() - Gets the speed of the current song. If -1, the song is unsupported. Audio.MusicSetSpeed(value) - Sets the speed of the current song. Check Audio.MusicGetSpeed() before setting to see if it's compatible or not (OGGs are supported as well).
Should help out with a few things for episode creators:
Since SDL Mixer X was upgraded, it also includes OGG channel layering support for Yoshi beats:
Audio.MusicChange(0, "_OST/File Select.ogg|m1;c1;r11")
m1 = Enable multi-track support c1 = The number of channels there is (Max is 8) r11 = Number of tracks there is (11 means using 11 different mono channels i.e.)
There's also music FX support as well (Based off TheXTech), but I haven't written code for the Audio class to add support for it.
Hopefully this long description clears things up, and this PR gets accepted whenever it does!