Scripting HelpersARCHIVE
A little piece of Roblox history. Preserved for everyone, one question at a time.Read about the archive →
COMMUNITY QUESTION

I'm looking for ways to improve this remote event?

This is a code I've written. As far as I am aware, it works however I somehow feel it's very inefficient and I'm looking for ways to improve upon it.

Could you help?

rep = game:GetService("ReplicatedStorage")
Jump = true

local function ender(anim, player, speed)

anim:Stop()

player.Character.Humanoid.WalkSpeed = speed	
end

rep["B1-1COMHE_EV"].OnServerEvent:connect(function(Player)
	
	launched = false
	local CurrentSpeed = Player.Character.Humanoid.WalkSpeed
	
	Player.Character.Humanoid.WalkSpeed = 2
	
	local Animation = Instance.new("Animation")
	Animation.AnimationId = "rbxassetid://503455087"
	local AnimTrack = Player.Character.Humanoid:LoadAnimation(Animation)
	AnimTrack:Play()
	Jump = false
	
	rep["B1-1COMHE_EV"]["B1-1COMHE_EV2"].OnServerEvent:connect(function(Player)
		if launched == false then
			print(launched)
			print("4")
			launched = true
			ender(AnimTrack, Player, CurrentSpeed)
			Jump = true
					
			local Animation2 = Instance.new("Animation")
			Animation2.AnimationId = "rbxassetid://505811047"
			local AnimTrack2 = Player.Character.Humanoid:LoadAnimation(Animation2)
			AnimTrack2:Play()
				
		end	
	end)
		
	coroutine.wrap(function()
		wait(5)
		if launched == false then
			print(launched)
			print("d")
			launched = true
			ender(AnimTrack, Player, CurrentSpeed)
			Jump = true
			
			local Animation2 = Instance.new("Animation")
			Animation2.AnimationId = "rbxassetid://505811047"
			local AnimTrack2 = Player.Character.Humanoid:LoadAnimation(Animation2)
			AnimTrack2:Play()
		
		end
	end)()	

	wait(6)
	
	launched = false
	
	Player.Character.Humanoid.Changed:connect(function(p)
		if p == "Jump" and not Jump then
			Player.Character.Humanoid.Jump = false
		end
	end)		
end)

1 answer

There's a bunch of things here that don't make sense to me.

launch is a global variable, even though it looks specific to a player.

Jump has the same value as launched.

rep["B1-1COMHE_EV"]["B1-1COMHE_EV2"].OnServerEvent is being connected to inside another event.

You're using coroutine.wrap for absolutely no reason.

Code is duplicated between an event and an if.


Given that you've not explained what this code does and it's not self explanatory, I'm just going to make guesses about what you meant to do.


Here's what I do:

  1. Get rid of coroutine.wrap. You should use delay -- except even that is completely unnecessary since you have a wait(6) that subsumes that entire time anyway.

  2. Delete Jump.

  3. Extract the duplicated code into a launch(player) function

  4. Make launched into local launched.

  5. Use not launched instead of launched == false.

  6. Disconnect the EV2 connection

  7. Move the Jump disabling event up and disconnect it at the bottom

  8. Use better names. rep, p, ender, CurrentSpeed are bad names because they don't describe what they do/are. It's idiomatic to use lowercase names for local variables (i.e., player not Player)

  9. Really ender doesn't deserve to be a function.

Here is what I get after applying these changes:

ReplicatedStorage = game:GetService("ReplicatedStorage")

function launch(player, setSpeed)
	player.Character.Humanoid.WalkSpeed = setSpeed

	local Animation2 = Instance.new("Animation")
	Animation2.AnimationId = "rbxassetid://505811047"
	local AnimTrack2 = player.Character.Humanoid:LoadAnimation(Animation2)
	AnimTrack2:Play()
end

ReplicatedStorage["B1-1COMHE_EV"].OnServerEvent:connect(function(player)
	local jumpDisabler = player.Character.Humanoid.Changed:connect(function(property)
		if property == "Jump" then
			player.Character.Humanoid.Jump = false
		end
	end)

	local launched = false
	local oldSpeed = player.Character.Humanoid.WalkSpeed

	player.Character.Humanoid.WalkSpeed = 2

	local Animation = Instance.new("Animation")
	Animation.AnimationId = "rbxassetid://503455087"
	local track = player.Character.Humanoid:LoadAnimation(Animation)
	track:Play()

	local ev2 = ReplicatedStorage["B1-1COMHE_EV"]["B1-1COMHE_EV2"].OnServerEvent:connect(function(player)
		if not launched then
			launched = true
			track:Stop()
			launch(player, oldSpeed)
		end 
	end)

	wait(5)
	if not launched then
		track:Stop()
		launch(player, oldSpeed)
	end
	ev2:disconnect()

	wait(1)
	jumpDisabler:disconnect()
end)

This still has bugs.

Namely, if two players do this sequence at the same time, EV2 from the second player will affect the first player.

You also should do error checking. player.Character can be nil.

Maybe a better way is to keep track of players' states, and have otherwise stateless events:

ReplicatedStorage = game:GetService("ReplicatedStorage")

local stateOf = {}

local jumpingDisablerOf = {}
function disableJumping(character)
	if not character or jumpingDisablerOf[character] then
		return
	end
	jumpingDisablerOf[character] = character.Humanoid.Changed:connect(function(property)
		if property == "Jump" then
			player.Character.Humanoid.Jump = false
		end
	end)
end

function enableJumping(character)
	if not character or not jumpingDisablerOf[character] then
		return
	end
	jumpingDisablerOf[character]:disconnect()
	jumpingDisablerOf[character] = nil
end

function PlayerLaunch(player)
	local oldMode = stateOf[player]
	if not oldMode or oldMode.mode ~= "waiting" then
		-- shouldn't Launch right now
		return
	end
	player.Character.Humanoid.WalkSpeed = oldMode.oldSpeed

	local Animation2 = Instance.new("Animation")
	Animation2.AnimationId = "rbxassetid://505811047"
	local AnimTrack2 = player.Character.Humanoid:LoadAnimation(Animation2)
	AnimTrack2:Play()

	stateOf[player] = {
		mode = "launching",
	}

	wait(1)
	enableJumping(player.Character)
end

function PlayerWait(player)
	if stateOf[player] then
		-- player is already busy
		return
	end
	
	local oldSpeed = player.Character.Humanoid.WalkSpeed
	
	local Animation = Instance.new("Animation")
	Animation.AnimationId = "rbxassetid://503455087"
	local track = player.Character.Humanoid:LoadAnimation(Animation)
	track:Play()

	player.Character.Humanoid.WalkSpeed = 2
	disableJumping(player.Character)

	stateOf[player] = {
		mode = "waiting",
		oldSpeed = oldSpeed,
		track = track,
	}
	
	wait(5)
	PlayerLaunch(player)
	stateOf[player] = nil
end

ReplicatedStorage["B1-1COMHE_EV"].OnServerEvent:connect(function(player)
	if not player.Character then
		return
	end
	PlayerWait(player)
end)

ReplicatedStorage["B1-1COMHE_EV"]["B1-1COMHE_EV2"].OnServerEvent:connect(function(player)
	if not player.Character then
		return
	end
	PlayerLaunch(player)
end)

This discussion is preserved as it was. New answers, comments, and votes are closed.