Simplyfying punching code

Help and Feedback Code Review
#1

Hello,
how would i go about optimizing/simplyfying this code as i repeat multiple statements over and over. Is there any way of not repeating the same actions?

local UIS = game:GetService("UserInputService")
local context_action_service = game:GetService("ContextActionService")
local punch_remote = game.ReplicatedStorage:WaitForChild("punch_remote")
local character = game.Players.LocalPlayer.Character or game.Players.LocalPlayer.CharacterAdded:Wait()
local humanoid = character:WaitForChild("Humanoid")
local animator = humanoid:WaitForChild("Animator")

local debounce = false
local combo = 0

local combo_settings = {
    max_combo = 3,
    cooldown = .25
}

local c1_anim = Instance.new("Animation")
c1_anim.AnimationId = "rbxassetid://18576726303"
local c1_anim_load = animator:LoadAnimation(c1_anim)

local c2_anim = Instance.new("Animation")
c2_anim.AnimationId = "rbxassetid://18576729183"
local c2_anim_load = animator:LoadAnimation(c2_anim)

local c3_anim = Instance.new("Animation")
c3_anim.AnimationId = "rbxassetid://18576731629"
local c3_anim_load = animator:LoadAnimation(c3_anim)

function on_Input(input, GPE)
    if not debounce then
        debounce = true
        
        combo += 1
        
        if combo == 1 then
            c1_anim_load:Play()
            c1_anim_load.Stopped:Wait()
        elseif combo == 2 then
            c2_anim_load:Play()
            c2_anim_load.Stopped:Wait()
        else
            c3_anim_load:Play()
            c3_anim_load.Stopped:Wait()
            combo = 0
        end
        
        punch_remote:FireServer()
        
        task.wait(combo_settings.cooldown)
        debounce = false
    end
end

context_action_service:BindAction("punch", on_Input, true, Enum.UserInputType.MouseButton1)
context_action_service:SetTitle("punch", "Punch")
1 Like
#2

This isn’t already too bad length wise, but if you really wanted to shorten it you could have a function that does this:

local c1_anim = Instance.new("Animation")
c1_anim.AnimationId = "rbxassetid://18576726303"
local c1_anim_load = animator:LoadAnimation(c1_anim)

and assigns it to a table with an index:

then in your combo script you can just do:

Combos[combo]:Play()
Combos[combo].Stoppped:Wait()
1 Like
#3

I guess you could sort of simply this by using a dictionairy of some sort but mostly I think this is unavoidable to simplify,

by the way I recommend you move this to #help-and-feedback:code-review since your code works, or else it may get taken down.

1 Like
#4

I typed this up, enjoy.

-- Services
local Services: {
    UIS: UserInputService,
    ContextActionService: ContextActionService,
    ReplicatedStorage: ReplicatedStorage,
    Players: Players
} = {
    UIS = game:GetService("UserInputService"),
    ContextActionService = game:GetService("ContextActionService"),
    ReplicatedStorage = game:GetService("ReplicatedStorage"),
    Players = game:GetService("Players")
}

-- Remote Event
local punchRemote: RemoteEvent = Services.ReplicatedStorage:WaitForChild("punch_remote")

-- Character and Humanoid
local player: Player = Services.Players.LocalPlayer
local character: Model = player.Character or player.CharacterAdded:Wait()
local humanoid: Humanoid = character:WaitForChild("Humanoid")
local animator: Animator = humanoid:WaitForChild("Animator")

-- State Variables
local debounce: boolean = false
local combo: number = 0

-- Combo Settings
local comboSettings: { maxCombo: number, cooldown: number } = {
    maxCombo = 3,
    cooldown = 0.25
}

-- Animation Instances
local animations: { [number]: Animation } = {
    [1] = Instance.new("Animation"),
    [2] = Instance.new("Animation"),
    [3] = Instance.new("Animation")
}

animations[1].AnimationId = "rbxassetid://18576726303"
animations[2].AnimationId = "rbxassetid://18576729183"
animations[3].AnimationId = "rbxassetid://18576731629"

-- Animation Loaders
local animationLoaders: { [number]: AnimationTrack } = {
    [1] = animator:LoadAnimation(animations[1]),
    [2] = animator:LoadAnimation(animations[2]),
    [3] = animator:LoadAnimation(animations[3])
}

-- Input Handling
local function onInput(input: InputObject, gameProcessedEvent: boolean)
    if gameProcessedEvent then return end
    if input.UserInputType == Enum.UserInputType.MouseButton1 and not debounce then
        debounce = true
        combo = (combo % comboSettings.maxCombo) + 1

        local currentAnimation: AnimationTrack = animationLoaders[combo]
        currentAnimation:Play()
        currentAnimation.Stopped:Wait()

        punchRemote:FireServer()

        task.wait(comboSettings.cooldown)
        debounce = false
    end
end

-- Binding Action
Services.ContextActionService:BindAction("punch", onInput, true, Enum.UserInputType.MouseButton1)
Services.ContextActionService:SetTitle("punch", "Punch")
1 Like
#5

I don’t agree with this, there are ways to optimize this code.

#6 
local CAS=game:GetService("ContextActionService")
local punch_remote=game.ReplicatedStorage:WaitForChild("punch_remote")
local character=game.Players.LocalPlayer.Character or game.Players.LocalPlayer.CharacterAdded:Wait()
local animator=character:WaitForChild("Humanoid"):WaitForChild("Animator")
local db,combo=true,0

local animations={
	animator:LoadAnimation(Instance.new("Animation",{AnimationId="rbxassetid://18576726303"})),
	animator:LoadAnimation(Instance.new("Animation",{AnimationId="rbxassetid://18576729183"})),
	animator:LoadAnimation(Instance.new("Animation",{AnimationId="rbxassetid://18576731629"}))
}

CAS:BindAction("punch",function()
	if(db)then db=false
		combo=(combo%3)+1;animations[combo]:Play()
		animations[combo].Stopped:Wait()
		punch_remote:FireServer()
		task.wait(.25)db=true
	end
end, true, Enum.UserInputType.MouseButton1)
5 Likes
#7

Not sure why you put services in a table, and shorthand them, while also going through fully typing them as well.

Also, theres no reason to have tables for animations, tables for animation ids, and now tables for animation tracks. Imagine wanting to add an animation, and instead of adding one rbxassetid you have to replace it in three places. So, if anything this complicates the code.

#8

Extendability, memoizataion, luau static typing optimizations with the new luau compiler. Can be further optimized by using a for loop to create new tables on runtime, but for simplicity they were included. Not to mention if OP modulates the tables, it would make everything efficient and less than 50 lines.

#9

This isn’t a good solution, there may be cases where OP needs to reference the animation object and not just the track. A better solution would be to have a table that creates the animation objects based on the combo with a for loop and loads the animations into two separate cached tables. The way you have this coded is ridged and will require refactoring if functionality were to change. It’s important to realize that just because code is short, does not mean it is good longterm.

#10

Just a heads up AnimationTrack.Animation exists. (It returns the Animation object that created the AnimationTrack) Also generally speaking the use case where you need to get the animation name is pretty rare, the only time I really can recall using it is when doing Animator:GetPlayingAnimationTracks() .

Also heads up, this does not exist in the Roblox API, something similar exists in RbxUtility though.

Instance.new("Animation",{AnimationId="rbxassetid://18576726303"})
1 Like
#11

Hey,

I was wondering about the bracketed arguments for the instance.new invocation.

Thanks,

#12

You’re right … wasn’t in the editor atm

local CAS = game:GetService("ContextActionService")
local punch_remote = game.ReplicatedStorage:WaitForChild("punch_remote")
local character = game.Players.LocalPlayer.Character or game.Players.LocalPlayer.CharacterAdded:Wait()
local animator = character:WaitForChild("Humanoid"):WaitForChild("Animator")

local debounce, combo = false, 0

local function createAnimation(id)
    local anim = Instance.new("Animation")
    anim.AnimationId = id
    return animator:LoadAnimation(anim)
end

local animations = {
    createAnimation("rbxassetid://18576726303"),
    createAnimation("rbxassetid://18576729183"),
    createAnimation("rbxassetid://18576731629")
}

CAS:BindAction("punch", function()
    if not debounce then
        debounce = true
        combo = (combo % 3) + 1
        animations[combo]:Play()
        animations[combo].Stopped:Wait()
        punch_remote:FireServer()
        task.wait(.25)
        debounce = false
    end
end, true, Enum.UserInputType.MouseButton1)
#13

Hey,

Is that a new APi feature? When did they add this? What are the performance implications, is it 500-1000ms extra like setting the parent?

Thanks,

#14

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