My music script is longer than I'd like it to be

Help and Feedback Code Review
#1

Provide an overview of:

  • What does the code do and what are you not satisfied with?
    – The code runs music on the client when given parameters from a remote function. Currently it causes microstutters when it’s called and it feels like it could be much shorter and efficient.
  • What potential improvements have you considered?
    – I’ve considered using multiple events instead of just having one event with versatile parameters, but that just seems more complicated and I’d rather have one functional, flexible script.
  • How (specifically) do you want to improve the code?
    – There are a lot of checks and loops that seem like they’re running more calculations than necessary, and the code itself is not very flexible if I want to make any changes to it. It overall feels inefficient.

The parameters of MusicEvent determines what the script does when called. If it receives 1, it plays a random lobby track, if it receives 0, it ends the music, and if it receives a path to a specific song, it will play it. The folder GameMusic has two sub-folders in it, Lobby and Game.

local SoundService = game:GetService("SoundService")
local GameMusic = game.Workspace.GameMusic
local lobbysongtable = {}
local MusicEvent = game.ReplicatedStorage:WaitForChild("MusicEvent")

for i,v in pairs(GameMusic.Lobby:GetChildren()) do
	if v:IsA("Sound") then
		table.insert(lobbysongtable,v)
	end
end

local function playsong(song)
	if song ~= 0 then
		print("received request to play "..tostring(song))
		if song == 1 then -- lobby mode
			lobbysongtable[math.random(1,#lobbysongtable)]:Play()
		else
			if song:IsA("Sound") then -- specific mode (Play the specified song)
				song:Play()
			end
		end
	end
end

local function endsong(song)
	local playing = false -- is there a song playing?
	local played = nil -- if so, what is it?
	for i,v in pairs(GameMusic:GetDescendants()) do -- find the song that's playing if there is one
		if v:IsA("Sound") and v.IsPlaying then
			playing = true
			played = v
		end
	end
	if playing then -- is there a song playing? End it.
		for i = 1, 15 do -- fade out
			played.Volume = played.Volume*0.8
			wait()
		end
		played:Stop() -- complete finish
		print("Fadeout completed")
		playsong(song) -- play the next song in que
	else
		playsong(song) -- if there is no song to stop, just play the next one
	end
end


MusicEvent.OnClientEvent:Connect(function(nextsong)
	endsong(nextsong)
	if nextsong == 0 then
		print("received request to mute songs")
	end
end)
1 Like
#2

Your script seems generally fine in terms of structure. It’s dead simple, and in code that’s a good thing. I don’t see why the script would be causing microstutters unless there are a ton of descendants of game music. I do have a couple improvements to two things:

  • Getting the current song
  • Making the fade out smoother

To get the current song better, you can store the current song in a variable so you don’t need to loop through it every time:

-- define the variable at the top of the script ;)
local currentSong

-- inside playsong
song:Play()
currentSong = song

local function endsong(song)
   if currentSong and currentSong.IsPlaying then
      -- fade out the song
   else
      -- play new song
   end
end

And to make the song fade out smoother, and actually reach 0 (multiplying by 0.8 never actually reaches 0), you can use TweenService:

local tween = game:GetService("TweenService"):Create(played, TweenInfo.new(0.15), {Volume = 0})
tween:Play()
tween.Completed:Wait()
played:Stop()

I also found a bug in your code, and that is Volume is never set back to 1 after a song has been faded out. So you should either reset it when you start playing the sound, or after the sound has been stopped.

3 Likes
#3

Thanks for pointing out the volume bug, I noticed it after a song came up twice in a row lol. As for the microstutters, turns out they were unrelated.

Thanks for your suggestions, this is only my second week coding and I want to make sure that I’m doing it right.

1 Like