The code will be attached to the bottom of the post. I think it’s important for the context to be first, so even a dropdown would not suffice in having the context be first.
I’m currently creating a music module for my game that’s intended to completely manage the playing and loading of music. I noticed that sounds (especially music) consume a large amount of client memory so I started changing the music system to allow developers to control the loading and unloading of music (presence in DataModel) as well as its playback.
The point of this Code Review thread is that I am greatly unsatisfied with the API and have concerns that there’s memory leaks. The API can be grazed over at a cursory glance while my memory leak concerns stem from how I handle tweens. Soundtracks aren’t treated as objects so developers have to constantly pass names when they want to interact with a track which is… somewhat ugly.
I’ve thought about moving my system to an object-oriented approach where tracks would be managed as an object. Methods such as playing and pausing would exist on the object and then the main player’s version of those functions would check which song the developer wants to interact with before calling the relevant function on the track object.
Would appreciate any replies about if my code leaks memory or if there’s a better way to approach this music player. Below is the exact code I currently have including framework calls (irrelevant to know though) and unfinished documentation (assuming I change how I do this).
Music.lua
--- Controls music loading and playback from the game's data.
-- Memory conscious; unloaded tracks are removed from the DataModel.
-- @author colbert2677
local ContentProvider = game:GetService("ContentProvider")
local SoundService = game:GetService("SoundService")
local TweenService = game:GetService("TweenService")
local MUSIC_SOUND_GROUP = SoundService:WaitForChild("BGM")
local DEFAULT_TWEEN_INFO = TweenInfo.new(1, Enum.EasingStyle.Linear, Enum.EasingDirection.Out)
local MAX_LOADED_TRACKS = 6
local Soundtrack
local Promise
local Butler = {
LoadedTracks = table.create(MAX_LOADED_TRACKS),
}
--- Get a track from the loaded tracks table by name
-- Cases of nil returns need to be handled in each method
-- @param trackName name of the track to fetch
-- @return table of track data or nil
function Butler:_GetLoadedTrack(trackName: string)
local trackData = self.LoadedTracks[trackName]
if not trackData then
local issueMessage = string.format("Attempted to interact with track \"%s\" (not a loaded track)")
self:DebugLog(issueMessage, "warning")
return nil
end
return trackData
end
--- Force cancel the active tween given a track data table
-- @param trackData a table of track data
function Butler:_CancelTweenIn(trackData: table)
local tween = trackData.Tween
if not tween then return end
tween:Cancel() -- Stop tween
tween:Destroy() -- Disconnect connections
trackData.Tween = nil
end
--- Get a list and number of tracks loaded by the Butler
-- @return an array of the names of loaded tracks
function Butler:GetLoadedTracks()
local trackNameTable = table.create(MAX_LOADED_TRACKS)
for trackName, _ in pairs(self.LoadedTracks) do
table.insert(trackNameTable, trackName)
end
return trackNameTable
end
--- Identical to GetLoadedTracks but scoped to playing tracks
-- @see GetLoadedTracks
function Butler:GetPlayingTracks()
local trackNameTable = table.create(MAX_LOADED_TRACKS)
for trackName, trackData in pairs(self.LoadedTracks) do
if trackData.Sound.IsPlaying == true then
table.insert(trackNameTable, trackName)
end
end
return trackNameTable
end
--- Load a track with an id with optional modifiers
-- Requires a named identifier
-- @param trackName named identifier
-- @param trackId assetid for the track
-- @param trackSpeed PlaybackSpeed
-- @param trackVolume Volume
function Butler:LoadTrackById(trackName: string, trackId: number, trackSpeed: number, trackVolume: number)
if #self:GetPlayingTracks() >= MAX_LOADED_TRACKS then
local issueMessage = string.format("Cannot exceed %d loaded tracks (soft rejected id=%d)", MAX_LOADED_TRACKS, trackId)
self:DebugLog(issueMessage, "warning")
return
elseif self.LoadedTracks[trackName] then
local issueMessage = string.format("Track already loaded (soft rejected name=%s)", trackName)
self:DebugLog(issueMessage, "warning")
return
end
local sound = Instance.new("Sound")
sound.Name = trackName
sound.Looped = true
sound.SoundId = "rbxassetid://" .. tostring(trackId)
sound.PlaybackSpeed = trackSpeed or sound.PlaybackSpeed
sound.Volume = 0
sound.SoundGroup = MUSIC_SOUND_GROUP
local soundData = table.create(4)
soundData.Sound = sound
soundData.OriginalVolume = trackVolume
soundData.LastAction = "Loaded"
self.LoadedTracks[trackName] = soundData
Promise.try(function ()
ContentProvider:PreloadAsync({sound})
end)
sound.Parent = MUSIC_SOUND_GROUP
end
--- Load a track from the preset Soundtrack module in Shared
-- trackName must be a valid key in the above module's table
-- @see LoadTrackById
function Butler:LoadTrackByName(trackName: string)
local soundtrackData = Soundtrack[trackName]
if not soundtrackData then
local issueMessage = string.format("Attempted to load a track that does not exist (\"%s\")", trackName)
self:DebugLog(issueMessage)
return
end
local trackId = soundtrackData.SoundId
local trackSpeed = soundtrackData.PlaybackSpeed
local trackVolume = soundtrackData.Volume
self:LoadTrackById(trackName, trackId, trackSpeed, trackVolume)
end
--- Prepare track for destruction
-- @param trackName name of a loaded track to unload
function Butler:UnloadTrack(trackName: string)
local track = self.LoadedTracks[trackName]
if not track then
local issueMessage = string.format("Attempted to unload a track that does not exist (\"%s\")", trackName)
self:DebugLog(issueMessage, "warning")
return
end
self:CancelTweenIn(track)
self:StopTrack(trackName)
local tween = track.Tween
if (tween) and not (tween.PlaybackState == Enum.PlaybackState.Completed or tween.PlaybackState == Enum.PlaybackState.Cancelled) then
tween.Completed:Wait()
end
track.Sound:Destroy()
track.Tween:Destroy()
track = nil
tween = nil
self.LoadedTracks[trackName] = nil
end
function Butler:UnloadAllTracks()
for trackName, _ in pairs(self.LoadedTracks) do
Promise.try(function ()
self:UnloadTrack(trackName)
end)
end
end
function Butler:PlayTrack(trackName: string)
local track = self:_GetLoadedTrack(trackName)
if not track then return end
local sound = track.Sound
self:_CancelTweenIn(track)
-- Very bootleg in place of an if statement
sound[sound.IsPaused and "Resume" or "Play"](sound)
local tween = TweenService:Create(sound, DEFAULT_TWEEN_INFO, {Volume = track.OriginalVolume})
track.Tween = tween
tween:Play()
end
function Butler:PauseTrack(trackName: string)
local track = self:_GetLoadedTrack(trackName)
if not track then return end
local sound = track.Sound
self:_CancelTweenIn(track)
local tween = TweenService:Create(sound, DEFAULT_TWEEN_INFO, {Volume = 0})
tween.Completed:Connect(function (playbackState)
if not (playbackState == Enum.PlaybackState.Completed) then return end
sound:Pause()
end)
track.Tween = tween
tween:Play()
end
function Butler:MuteTrack(trackName: string)
local track = self:_GetLoadedTrack(trackName)
if not track then return end
local sound = track.Sound
self:_CancelTweenIn(track)
local tween = TweenService:Create(sound, DEFAULT_TWEEN_INFO, {Volume = 0})
track.Tween = tween
tween:Play()
end
function Butler:UnmuteTrack(trackName: string)
local track = self:_GetLoadedTrack(trackName)
if not track then return end
local sound = track.Sound
self:_CancelTweenIn(track)
local tween = TweenService:Create(sound, DEFAULT_TWEEN_INFO, {Volume = track.OriginalVolume})
track.Tween = tween
tween:Play()
end
function Butler:StopTrack(trackName: string)
local track = self:_GetLoadedTrack(trackName)
if not track then return end
local sound = track.Sound
self:_CancelTweenIn(track)
local tween = TweenService:Create(sound, DEFAULT_TWEEN_INFO, {Volume = 0})
tween.Completed:Connect(function (playbackState)
if not (playbackState == Enum.PlaybackState.Completed) then return end
sound:Stop()
end)
track.Tween = tween
tween:Play()
end
function Butler:Start()
-- Left intentionally blank.
end
function Butler:Init()
Soundtrack = self.Shared.Soundtrack
Promise = self.Shared.Promise
end
return Butler
(The code looks long but the bottom half with all the functions are functionally identical. Some just use different values in the tween goals or have Completed connections.