How can I improve this Long Jump script?

Help and Feedback Code Review
#1

Im trying to make a game like Super Mario 64, and so I made a long jump script.

Im wondering how I can improve it whether it be for player preformance, or a way to exploit / abuse a flaw in the code.

Code:

--\\ Variables //--
repeat task.wait() until game.Players.LocalPlayer

local UIS = game:GetService("UserInputService")
local RS = game:GetService("ReplicatedStorage")
local TweenService = game:GetService("TweenService")
local RunService = game:GetService('RunService')

local Character = game.Players.LocalPlayer.Character
local JumpAnimation = Character.Humanoid:LoadAnimation(RS.LongJumpAnimation.LongJumps)
local DView = 70
local Table = {}

local root = script.Parent.PrimaryPart
local humanoid = root.Parent:WaitForChild("Humanoid")
local state = " "

local stateText = script.Parent:WaitForChild("Head"):FindFirstChildOfClass("BillboardGui").TextLabel

local Animator = humanoid:WaitForChild("Animator")

--\\ Animations //--
local runAnimId = Instance.new("Animation")
runAnimId.AnimationId = "rbxassetid://10493878394"

local walkAnimId = Instance.new("Animation")
walkAnimId.AnimationId = "rbxassetid://10494088845"

local jumpAnimId = Instance.new("Animation")
jumpAnimId.AnimationId = "rbxassetid://10494212955"

local fallAnimId = Instance.new("Animation")
fallAnimId.AnimationId = "rbxassetid://10494451597"

local runAnim = Animator:LoadAnimation(runAnimId)
local walkAnim = Animator:LoadAnimation(walkAnimId)
local jumpAnim = Animator:LoadAnimation(jumpAnimId)
local fallAnim = Animator:LoadAnimation(fallAnimId)


--\\ Functions //--

function speedWatch(speed)
	if speed == 32 then
		local boostVal = 1.25
		return boostVal
	elseif speed == 16 then
		local boostVal = 1.15
		return boostVal
	else
		local boostVal = 1
		return boostVal
	end
end

local function StateCheck()
	local current = humanoid:GetState()
	if current == Enum.HumanoidStateType.Running and humanoid.MoveDirection.Magnitude > 0 then
		local Speed = humanoid.WalkSpeed
		if Speed > .75 and Speed < 32 then
			state = "WALKING"
			if runAnim.IsPlaying then
				runAnim:Stop()
			end
			if not walkAnim.IsPlaying then
				walkAnim:Play()
			end
			if jumpAnim.IsPlaying then
				jumpAnim:Stop()
			end
			if fallAnim.IsPlaying then
				fallAnim:Stop()
			end
		elseif Speed >= 32 then
			state = "RUNNING"
			if not runAnim.IsPlaying then
				runAnim:Play()
			end
			if walkAnim.IsPlaying then
				walkAnim:Stop()
			end
			if jumpAnim.IsPlaying then
				jumpAnim:Stop()
			end
			if fallAnim.IsPlaying then
				fallAnim:Stop()
			end
		end
	elseif current == Enum.HumanoidStateType.Jumping then
		state = "JUMPING"
		if runAnim.IsPlaying then
			runAnim:Stop()
		end
		if walkAnim.IsPlaying then
			walkAnim:Stop()
		end
		if fallAnim.IsPlaying then
			fallAnim:Stop()
		end
		if not jumpAnim.IsPlaying then
			jumpAnim:Play()
		end
	elseif current == Enum.HumanoidStateType.Freefall then
		state = "FREEFALL"
		if runAnim.IsPlaying then
			runAnim:Stop()
		end
		if walkAnim.IsPlaying then
			walkAnim:Stop()
		end
		if jumpAnim.IsPlaying then
			jumpAnim:Stop()
		end
		if not fallAnim.IsPlaying then
			fallAnim:Play()
		end
	else
		state = "IDLE"
		humanoid.WalkSpeed = 16
		if runAnim.IsPlaying then
			runAnim:Stop()
		end
		if walkAnim.IsPlaying then
			walkAnim:Stop()
		end
		if jumpAnim.IsPlaying then
			jumpAnim:Stop()
		end
		if fallAnim.IsPlaying then
			fallAnim:Stop()
		end
	end
	print(state)
end

local Deb = false

UIS.InputBegan:Connect(function(Input, IsTyping)
	print(state == "RUNNING" and state ~= "IDLE" and state ~= "JUMPING" and state ~= "LONG_JUMP")
	if state == "RUNNING" and state ~= "IDLE" and state ~= "JUMPING" and state ~= "LONG_JUMP" and state ~= "FREEFALL" then
		if IsTyping then return end
		if UIS:IsKeyDown(Enum.KeyCode.LeftShift) and UIS:IsKeyDown(Enum.KeyCode.Space) and humanoid.Running then
			if not Deb then
				Deb = true
				
				for i = 1,20 do
					task.spawn(function()
						task.wait(.0075)
						state = "LONG_JUMP"
					end)
				end
				local speed = Character.Humanoid.WalkSpeed

				local speedBoost = speedWatch(humanoid.WalkSpeed)

				JumpAnimation:Play()

				humanoid.Jump = false humanoid.JumpPower = 0
				root:ApplyImpulse(root.CFrame.LookVector * (1000 * speedBoost))
				workspace.Gravity = 145.8
				root:ApplyImpulse(root.CFrame.YVector * (475 * speedBoost))
				task.wait(0.25)
				workspace.Gravity = 196.2
				humanoid.Jump = false humanoid.JumpPower = 50

				JumpAnimation:Stop()

				Deb = false
			end
		end
	end
end)

RunService.Heartbeat:Connect(StateCheck)

--\\ Loops //--

while task.wait() do
	stateText.Text = "STATE: "..state
	print(state)
end
#2

Instead of repeat task.wait() until game.Players.LocalPlayer, you can just do this:

local Players = game:GetService("Players")

local LocalPlayer = Players.LocalPlayer or Players:GetPropertyChangedSignal("LocalPlayer"):Wait()

I’ll look through it more right now.

#3

LocalPlayer will always equal to your Player instance. But LocalPlayer.Character might be nil so it’s best to do

local Character = if Player.Character then Player.Character else Player.CharacterAdded:Wait()
#4

There are some edge cases where it won’t be and it will return nil. (Especially when the script is in ReplicatedFirst)

#5

Yes but this script isn’t in ReplicatedFirst and also this script only works once the first time you spawn in and nevet after. If this script were to be in StarterCharacterScripts than it would cause a MemoryLeak so.

#6

What? How would this cause a memory leak?

This edgecase can still happen. (keyword is edgecase)

#7

If it were in StarterCharacterScript since it’s connecting to UserInputService everytime causing a MemoryLeak. And also It doesn’t matter since Player.Character will pretty much always be nil when the script starts.

#8

Not to mention the Hearbeat connection.

#9

You should experiment using UserInputService. It automatically disconnects when the script is destroyed, along with every other connection.

#10

But does Heartbeat do that or not?

#11

Im very confused

#12

When you’re defining your animations you’re repeating the same lines of code just with different animation IDs, in order to shorten this you could try:

--\\ Animations \\--
local function LoadAnimation(id)
	local animation = Instance.new("Animation")
	animation.AnimationId = id

	local animationTrack = Animator:LoadAnimation(animation)
	return animationTrack
end

I would all your animations to a table because in your StateCheck function you’re also repeating the same code but with small changes, you should name them after the states they correspond to.

local Animations = {
	RUNNING = LoadAnimation("rbxassetid://10493878394"),
	WALKING = LoadAnimation("rbxassetid://10494088845"),
	JUMPING = LoadAnimation("rbxassetid://10494088845"),
	FREEFALL = LoadAnimation("rbxassetid://10494451597"),
}

Creating a dictionary of these animations will be useful when shortening the StateCheck function. Instead of checking whether or not each animation IsPlaying and then either playing or stopping the animation individually, is going to make your script a lot longer than it needs to be. Instead you can create a function that loops through the dictionary of animations and then play a specified animation that will play, and make all the others stop:

local function ExcludeStopAnimations(animationName)
	for name, animationTrack in pairs(Animations) do
		if not animationTrack.IsPlaying then
			continue
		end

		if name == animationName then
			animationTrack:Play()
		else
			animationTrack:Stop()
		end
 	end
end

After defining this function you can use it to clean up the StateCheck function:

local function StateCheck()
	local current = humanoid:GetState()
	if current == Enum.HumanoidStateType.Running and humanoid.MoveDirection.Magnitude > 0 then
		local Speed = humanoid.WalkSpeed
		if Speed > .75 and Speed < 32 then
			state = "WALKING"
		elseif Speed >= 32 then
			state = "RUNNING"
		elseif current == Enum.HumanoidStateType.Jumping then
			state = "JUMPING"
		elseif current == Enum.HumanoidStateType.Freefall then
			state = "FREEFALL"
		else
			state = "IDLE"
			humanoid.WalkSpeed = 16
		end
	end
	ExcludeStopAnimations(state)
end

At the end of your script you have this loop:

while task.wait() do
	stateText.Text = "STATE: "..state
	print(state)
end

This is unnecessary because you can simply change the text every time the state is changed. I saw a few lines of code that confused me a little bit:

for i = 1,20 do
	task.spawn(function()
		task.wait(.0075)
		state = "LONG_JUMP"
	end)
end

I’m guessing that this is meant to keep the state set to LONG_JUMP for a certain amount of time. Instead of doing this loop, you could define a variable at the beginning of your script named stateLock and set it to false. The only other function in your script that changes the state is the StateCheck function, in order to stop it from changing the state while it is locked you can add this if statement to the beginning of the function:

if stateLock == true then
    return
end

Instead of the loop I adressed before, you could simply wait some amount of time, set the state to LONG_JUMP and then set stateLock to true, once the amount of time is finished you unlock the state so it can be changed once again:

task.delay(0.0075*20, function()
	stateLock = false
end)
stateLock = true
state = "LONG_JUMP"

I agree with @Katrist that you shouldn’t have a loop in the beginning of the script and you should instead define the player variable and wait for the player if it doesn’t currently exist

Other than that, there’s nothing more that I could think of changing. Let me know if you have any questions about the changes I suggested or if you have any trouble implementing them.

2 Likes
#13

Wouldn’t I be techincally making about 100 animation instances?

#14

No, you’d be making the same amount of animation instances, because instead of:

--\\ Animations //--
local runAnimId = Instance.new("Animation")
runAnimId.AnimationId = "rbxassetid://10493878394"

local walkAnimId = Instance.new("Animation")
walkAnimId.AnimationId = "rbxassetid://10494088845"

local jumpAnimId = Instance.new("Animation")
jumpAnimId.AnimationId = "rbxassetid://10494212955"

local fallAnimId = Instance.new("Animation")
fallAnimId.AnimationId = "rbxassetid://10494451597"

local runAnim = Animator:LoadAnimation(runAnimId)
local walkAnim = Animator:LoadAnimation(walkAnimId)
local jumpAnim = Animator:LoadAnimation(jumpAnimId)
local fallAnim = Animator:LoadAnimation(fallAnimId)

You would have:

--\\ Animations \\--
local function LoadAnimation(id)
	local animation = Instance.new("Animation")
	animation.AnimationId = id

	local animationTrack = Animator:LoadAnimation(animation)
	return animationTrack
end

local Animations = {
	RUNNING = LoadAnimation("rbxassetid://10493878394"),
	WALKING = LoadAnimation("rbxassetid://10494088845"),
	JUMPING = LoadAnimation("rbxassetid://10494088845"),
	FREEFALL = LoadAnimation("rbxassetid://10494451597"),
}

It would just be a little bit shorter, and make the code for state checking a little bit shorter as well, along with making it easier to add more states and animations.

1 Like