Is there a way I can make this animation code shorter and more readable?

Help and Feedback Code Review
#1

So I made this code that basically waits for player input (W, A, S, D or two at the same time) and plays an animation. It’s my first time ever making a code totally by myself without looking it up on Youtube, but I feel like the code still looks goofy, I’m sure that I did lots of mistakes when making it even tho it works.

Is there any way I could improve it? Don’t just send me the code please I want to code it myself, I just need some help to find how

-- Wait for player to load and then disable animate script

while game.Players.LocalPlayer.Character:FindFirstChild("Animate").Enabled == true do
	game.Players.LocalPlayer.Character:FindFirstChild("Animate").Enabled = false
	wait(1)
end


-- get service
local uis = game:GetService("UserInputService")

-- variables
local forward = Enum.KeyCode.W
local backwards = Enum.KeyCode.S
local left = Enum.KeyCode.A
local right = Enum.KeyCode.D

local hum = game.Players.LocalPlayer.Character:WaitForChild("Humanoid")

-- animations
local forwardAnim = script:WaitForChild("forwardAnim")
local forwardTrack = hum:LoadAnimation(forwardAnim)

local backwardAnim = script:WaitForChild("backwardAnim")
local backwardTrack = hum:LoadAnimation(backwardAnim)

local leftAnim = script:WaitForChild("leftAnim")
local leftTrack = hum:LoadAnimation(leftAnim)
	
local rightAnim = script:WaitForChild("rightAnim")
local rightTrack = hum:LoadAnimation(rightAnim)

-- movement anims script
uis.InputBegan:Connect(function(input, isTyping)
	if isTyping then return end
		
	if input.KeyCode == forward then
		forwardTrack:Play()
	elseif input.KeyCode == forward and input.KeyCode == left or input.KeyCode == right then
		forwardTrack:Play()
	elseif input.KeyCode == backwards then
		backwardTrack:Play()
	elseif	input.KeyCode == left then
		leftTrack:Play()
	elseif input.KeyCode == right then
		rightTrack:Play()
	end
end)

uis.InputEnded:Connect(function(input, isTyping)
	if isTyping then return end
		
	if input.KeyCode == forward then
		forwardTrack:Stop()
	elseif input.KeyCode == forward and input.KeyCode == left or input.KeyCode == right then
		forwardTrack:Stop()
	elseif input.KeyCode == backwards then
		backwardTrack:Stop()
	elseif	input.KeyCode == left then
		leftTrack:Stop()
	elseif input.KeyCode == right then
		rightTrack:Stop()
	end
end)
#2

It’s not a bad script for a beginner. Theres a few suggestions I have.

For starters, it would be better practice to create a variable for Players, rather than repeating game.Players.

I would also suggest using Player.CharacterAdded:Wait() rather than creating a loop to check if the script is Enabled.

I noticed you were doing this within the InputBegan and InputEnded Functions:

elseif input.KeyCode == forward and input.KeyCode == left or input.KeyCode == right then

This line isn’t really needed at all as the function will only ever return 1 Keycode. It’ll never return 2 at the same time. It will just call it as a separate function.

Other than those, I think it’s pretty good. I did write a compact version if you’re interested? I put the Keycodes in a Table and I was able to make the Input Functions a lot smaller.

#3

I decided to change my code a bit and I don’t know if this version is better:

-- Services
local player = game:GetService("Players")
local uis = game:GetService("UserInputService")
local runService = game:GetService("RunService")

-- Variables
player.LocalPlayer.CharacterAdded:Wait()
local character = player:WaitForChild("Character")
local humanoid = character:WaitForChild("Humanoid")

-- Disable "Animate" script
local animate = character:WaitForChild("Animate")
animate.Enabled = false

-- Different Animations
local animations = {
	forwardAnimation = "",
	backwardsAnimation = "",
	leftAnimation = "",
	rightAnimation = "",
	idle = ""
}

-- Different Keys
local mKeys = {
	forward = Enum.KeyCode.W,
	backwards = Enum.KeyCode.S,
	left = Enum.KeyCode.A,
	right = Enum.KeyCode.D
}

-- Function
function walkingAnimations()
	if humanoid.MoveDirection.Magnitude > 0 then
		if uis:IsKeyDown(mKeys.forward) then
			local animTrack = humanoid:LoadAnimation(animations.forwardAnimation)
			animTrack:Play()
		elseif uis:IsKeyDown(mKeys.backwards) then
			local animTrack = humanoid:LoadAnimation(animations.backwardsAnimation)
			animTrack:Play()
		elseif uis:IsKeyDown(mKeys.left) then
			local animTrack = humanoid:LoadAnimation(animations.leftAnimation)
			animTrack:Play()
		elseif uis:IsKeyDown(mKeys.right) then
			local animTrack = humanoid:LoadAnimation(animations.rightAnimation)
			animTrack:Play()
		end
	else
		local animTrack = humanoid:LoadAnimation(animations.idle)
		animTrack:Play()
	end
end

runService.RenderStepped:Connect(walkingAnimations)

Now the problem is that "Infinite yield possible on 'Players:WaitForChild(“Character)” and can’t seem to find how to fix that… maybe should I just go back to the other version?

#4

Character isn’t a Child inside Player. You can fix that by doing Player.Character

#5

An Issue I can see with your original script would be that if you press 2 or more buttons at the same time. All Animations will play. You’d need to Stop all other Animations first.

#6

This script looks less efficient than the first one. You’re loading the Animation within a loop. You only need to load it once

#7

Yeah that’s what I was going to say, let me just go back to first script, can you show me the compact version that you did?

#8

In the second version I just noticed I was just going to continuously play an animation and never stop it

#9

This is what I came up with, although it is untested.

-- get service
local uis = game:GetService("UserInputService")
local Players = game:GetService("Players")

-- variables
local Player = Players.LocalPlayer
Player.CharacterAdded:Wait()
local Character = Player.Character
local hum = Character:WaitForChild("Humanoid")

-- animations
local forwardAnim = script:WaitForChild("forwardAnim")
local backwardAnim = script:WaitForChild("backwardAnim")
local leftAnim = script:WaitForChild("leftAnim")
local rightAnim = script:WaitForChild("rightAnim")

local forwardTrack = hum:LoadAnimation(forwardAnim)
local backwardTrack = hum:LoadAnimation(backwardAnim)
local leftTrack = hum:LoadAnimation(leftAnim)
local rightTrack = hum:LoadAnimation(rightAnim)

local Keys = {
	[Enum.KeyCode.W] = forwardTrack,
	[Enum.KeyCode.S] = backwardTrack,
	[Enum.KeyCode.A] = leftTrack,
	[Enum.KeyCode.D] = rightTrack
}

-- functions
function StopAll()
	for _,v in pairs(Keys) do
		v:Stop()
	end
end

uis.InputBegan:Connect(function(input, isTyping)
	local KeyCode = input.KeyCode
	if Keys[KeyCode] and not isTyping then
		StopAll();Keys[KeyCode]:Play()
	end
end)

uis.InputEnded:Connect(function(input, isTyping)
	local KeyCode = input.KeyCode
	if Keys[KeyCode] and not isTyping then
		StopAll()
	end
end)

You could probably add Attributes to each Animation and load them into a table with a “for loop” rather than doing 4 separate WaitForChild’s but this works too

1 Like
#10

So from what I can understand:

Here you stop all already-running animations:
image

Then here, you say that KeyCode = the input, so if a player pressed W it will look in the table for the track that corresponds to the KeyCode and then play it?
image

And here you just simply tell that when the keys are released, all animations stops (and it’s here I guess that I can add an Idle animation later on?):
image

#11

Looking back at it. I think calling StopAll() on the InputEnded function wouldn’t be good because if a player is holding A and W then releases A while continuing to hold W… the Animation would just stop.

So maybe just add:

Keys[KeyCode]:Stop()

Other than that, I guess you could easily make a function that checks what Animations are playing and if none are playing then play Idle.

I’m unsure to why you need to play Idle though? Shouldn’t it already be playing? Unless you turned off Roblox’s default idle animation.

Seems like you understand though!

#12

I mean if I wanted to use a custom idle, but looking back at it I can just simply change the idle animation in Roblox’s Default Animate module, thanks for you help tho!

1 Like
#13

Either works really, I hope this helped!

#14

This topic was automatically closed 14 days after the last reply. New replies are no longer allowed.