Feedback on a messy animation module

Help and Feedback Code Review
#1

I’m making an animation module for my platformer game, it looks pretty messy to me, and I’m also not sure if this will even work or not.

Here is the local script

local char = game.Players.LocalPlayer.Character or game.Players.LocalPlayer.CharacterAdded:Wait()
local humanoid = char:WaitForChild("Humanoid")

local module = require(game:GetService("ReplicatedStorage"):WaitForChild("Player"):WaitForChild("AnimationModule"))

game:GetService("RunService").Heartbeat:Connect(function()
	if humanoid.WalkSpeed > 0 and not module.stateLock then
		module.state = "sprinting"

		module.animations.sprinting:Play()
		
	elseif humanoid.WalkSpeed == 0 and not module.stateLock then
		module.animations.idle:Play()
	end
end)

Module Script

local module = {}

local char = game.Players.LocalPlayer.Character or game.Players.LocalPlayer.CharacterAdded:Wait()
local humanoid = char:WaitForChild("Humanoid")

module.state = "NaN"
module.stateLock = false

Instance.new("Animation", script).Name = "idle"
Instance.new("Animation", script).Name = "sprinting"
Instance.new("Animation", script).Name = "jumping"
Instance.new("Animation", script).Name = "climbing"
Instance.new("Animation", script).Name = "swimming"

Instance.new("Animation", script).Name = "longJump"
Instance.new("Animation", script).Name = "crouch"
Instance.new("Animation", script).Name = "dive"
Instance.new("Animation", script).Name = "groundPound"

script:WaitForChild("idle").AnimationId = "rbxassetid://1"
script:WaitForChild("sprinting").AnimationId = "rbxassetid://1"
script:WaitForChild("jumping").AnimationId = "rbxassetid://1"
script:WaitForChild("climbing").AnimationId = "rbxassetid://1"
script:WaitForChild("swimming").AnimationId = "rbxassetid://1"

script:WaitForChild("longJump").AnimationId = "rbxassetid://1"
script:WaitForChild("crouch").AnimationId = "rbxassetid://1"
script:WaitForChild("dive").AnimationId = "rbxassetid://1"
script:WaitForChild("groundPound").AnimationId = "rbxassetid://1"

module.animations = {
	-- Core Animations
	idle = humanoid:LoadAnimation(script:WaitForChild("idle"));
	sprinting = humanoid:LoadAnimation(script:WaitForChild("sprinting"));
	jumping = humanoid:LoadAnimation(script:WaitForChild("jumping"));
	climbing = humanoid:LoadAnimation(script:WaitForChild("climbing"));
	swimming = humanoid:LoadAnimation(script:WaitForChild("swimming"));
	
	-- Moveset Animations
	longJump = humanoid:LoadAnimation(script:WaitForChild("longJump"));
	crouch = humanoid:LoadAnimation(script:WaitForChild("crouch"));
	dive = humanoid:LoadAnimation(script:WaitForChild("dive"));
	groundPound = humanoid:LoadAnimation(script:WaitForChild("groundPound"));
}

function forceAnimation(animation)
	for i, v in pairs(module.animations) do
		if v.Name ~= animation then
			if v.IsPlaying then
				v:Stop()
			end
		end
	end
	
	animation:Play()
end

return module
#2

You repeat instance creation too much. Make a table of animation data, and then loop through that table to create the animations. This would knock out about 50-70% of your code.

3 Likes
#3

We can cut off game.Players.LocalPlayer by defining it in both code by
local Player=game:GetService('Players').LocalPlayer
then char can be
local char=Player.Character or Player.CharacterAdded:Wait()
if you are defining Character Once it should be in StarterCharacterScripts
so we can simplify this to be local char=script.Parent

we should put the modulescript in the script itself so it can be
require(script:WaitForChild('AnimationModule'))

the Animation module is pretty messy so we can create an AnimationConstructor by a function

function CreateAnimation(...):{[number]:Animation}
	local Names={...}
	local Animations={}
	for c,v in pairs(Names) do
		local Animation=Instance.new('Animation',script)
		Animation.Name=v
		Animations[c]=Animation
	end
	return Animations
end

and then create Animation by
local Animations=CreateAnimation(idle,sprinting,jumping,climbing,swimming,longJump,crouch,dive,groundPound)
this still looks ugly but is better.

Hunamoid:LoadAnimation is deprecated so we need to use the Animator in the Humanoid

And then we can Set an AnimationId and Load it in the same for c,v loop

local AnimIds={1,1,1,1,1 ,1,1,1,1}
module.animations = {}
for c, v in pairs(Animations) do
	Animations[c].AnimationId=`rbxassetid://{AnimIds}`
	module.animations[v.Name]=Animator:LoadAnimation(v)
end

force Animation can stay the same

but we end up with
LOCALSCRIPT

local RunService=game:GetService('RunService')
local Player=game.Players.LocalPlayer
local char = Player.Character or Player.CharacterAdded:Wait()
local humanoid = char:WaitForChild("Humanoid")

local module = require(script.ModuleScript)

RunService.Heartbeat:Connect(function()
	if humanoid.WalkSpeed > 0 and not module.stateLock then
		module.state = "sprinting"
		module.animations.sprinting:Play()
	elseif humanoid.WalkSpeed == 0 and not module.stateLock then
		module.animations.idle:Play()
	end
end)

ANIMATIONMODULE

local module = {}
local Player=game.Players.LocalPlayer
local char = Player.Character or Player.CharacterAdded:Wait()
local humanoid:Humanoid = char:WaitForChild("Humanoid")
local Animator:Animator=humanoid:WaitForChild('Animator')
module.state = "NaN"
module.stateLock = false
function CreateAnimation(...):{[number]:Animation}
	local Names={...}
	local Animations={}
	for c,v in pairs(Names) do
		local Animation=Instance.new('Animation',script)
		Animation.Name=v
		Animations[c]=Animation
	end
	return Animations
end
local Animations=CreateAnimation(`idle`,`sprinting`,`jumping`,`climbing`,`swimming`,`longJump`,`crouch`,`dive`,`groundPound`)
local AnimIds={1,1,1,1,1 ,1,1,1,1}
module.animations = {}
for c, v in pairs(Animations) do
	Animations[c].AnimationId=`rbxassetid://{AnimIds}`
	module.animations[v.Name]=Animator:LoadAnimation(v)
end
function forceAnimation(animation)
	for i, v in pairs(module.animations) do
		if v.Name ~= animation and v.IsPlaying then
			v:Stop()
		end
	end
	animation:Play()
end

return module

make sure the modulescript is a child of localscript