Feedback knife tool

Help and Feedback Code Review
, , ,
#1

Relatively beginner scripter, looking for pointers , I feel as if my script sucks, or atleast is flawed or in someway unoptimized or being executed in a bad way with better alternatives overall. Looking for someone with more experience who can confirm or deny with some tips,

--Client Module Script
local Knife = {}
--//Services
local ReplicatedStorage = game:GetService("ReplicatedStorage")
local UserInputService = game:GetService("UserInputService")
local Debris = game:GetService("Debris")
--//Requires
local AnimationPlayer = require(script.Parent:WaitForChild("AnimationPlayer"))
--//Variables
local events = ReplicatedStorage:WaitForChild("Events")
local requestDamage = events:WaitForChild("Damage")

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

local params = OverlapParams.new()
params.FilterType = Enum.RaycastFilterType.Exclude
params.FilterDescendantsInstances = {character}

--Everytime tool is activated
local function activateTool(tool)
	if AnimationPlayer.isSwinging or AnimationPlayer.isThrowing then return end
	
	local hitbox = tool:WaitForChild("Hitbox")
	local startTime = os.clock()
	local hitCharacters = {}
	
	task.spawn(AnimationPlayer.swingKnife, tool)
	
	while AnimationPlayer.isSwinging do
		if not tool:FindFirstChild("Hitbox") then break end
		
		local parts = workspace:GetPartsInPart(hitbox, params)
		for _, part in pairs(parts) do
			if part.Parent:FindFirstChild("Humanoid") then
				local char = part.Parent
				local humanoid = char:FindFirstChild("Humanoid")
				if humanoid and humanoid.Health > 0 and not table.find(hitCharacters, char) then
					table.insert(hitCharacters, char)
					if #hitCharacters > 2 then return end
					requestDamage:FireServer(char) -- server damages
				end
			end
		end
		task.wait()
	end
	hitCharacters = {}
end

--When user tries to throw knife
local function knifeThrow(inputObj, isTyping)
	
end
UserInputService.InputBegan:Connect(knifeThrow)

--Setup a knife tool when added to player/char
local function detectTool(tool)
	if tool:IsA("Tool") and tool:GetAttribute("Type") == "Knife" then
		if not tool:GetAttribute("ActivatedConnection") then
			
			tool:SetAttribute("ActivatedConnection", true)
			tool.Activated:Connect(function()
				activateTool(tool)
			end)
		end
	end
end
lplayer.Backpack.ChildAdded:Connect(detectTool)
character.ChildAdded:Connect(detectTool)

--Setup the character on respawn
local function setupCharacter(char)
	character = char
	humanoid = char:WaitForChild("Humanoid")
	params.FilterDescendantsInstances = {character}
	character.ChildAdded:Connect(detectTool)
end
lplayer.CharacterAdded:Connect(setupCharacter)

return Knife

--Client Module Script
local AnimationPlayer = {}
--//Services
local ReplicatedStorage = game:GetService("ReplicatedStorage")
local UserInputService = game:GetService("UserInputService")
local Debris = game:GetService("Debris")
local ContentProvider = game:GetService("ContentProvider")
--//Requires
local itemsList = require(ReplicatedStorage:WaitForChild("Dictionary"):WaitForChild("ItemsList"))
--//Variables
local animations = ReplicatedStorage:WaitForChild("Animations")
local lplayer = game.Players.LocalPlayer
local character = lplayer.Character or lplayer.CharacterAdded:Wait()
local humanoid = character:WaitForChild("Humanoid")
local animator = humanoid:WaitForChild("Animator")

--Swinging knife animation combo
local currentTrack = 0
function AnimationPlayer.swingKnife(tool)
	if AnimationPlayer.isSwinging then return end
	AnimationPlayer.isSwinging = true
	currentTrack += 1
	
	local animationPath = itemsList[tool.Name].Animations.Slashes
	if currentTrack > #animationPath:GetChildren() then currentTrack = 1 end
	
	local anims = animationPath:GetChildren()
	local clone = anims[currentTrack]:Clone()
	local track = animator:LoadAnimation(clone)
	track:Play()
	track.Stopped:Wait()
	task.wait(0.1)
	
	AnimationPlayer.isSwinging = false
	Debris:AddItem(clone,0.25)
end

-- Throwing knife animation
function AnimationPlayer.throwKnife(tool)
	
end

--Setup the character on respawn
local function setupCharacter(char)
	character = char
	humanoid = char:WaitForChild("Humanoid")
	animator = humanoid:WaitForChild("Animator")
	AnimationPlayer.isSwinging = false
	AnimationPlayer.isThrowing = false
end
lplayer.CharacterAdded:Connect(setupCharacter)

--Preload all animations (TEMPORARY)
print("Loading")
for _,mainFolder in pairs(animations:GetChildren()) do
	for _,folder in pairs(mainFolder:GetChildren()) do
		for _,anims in pairs(folder:GetChildren()) do
			ContentProvider:PreloadAsync({anims})
		end
	end
end
print("Loaded all assets")

return AnimationPlayer
1 Like
#2

First of all, why are you using Debris? (edit: why are you even cloning the animation, just use the original one, you get an animation track anyway, so it’s not going to be affected)
You see, Instance:Destroy only sets the Instance’s parent to nil and disconnects any events. It doesn’t free the memory or anything, that’s the garbage collector’s job.
You are calling debris on an animation, which has no event handlers, and its parent is already nil. So you don’t need to use debris, and you can just leave it and let the garbage collector handle it. Don’t use debris in general, because it is much slower than just doing:

task.spawn(function()
  task.wait(your_time)
  your_object:Destroy()
end)

In the very last line of the activate function you’re resetting hitcharacters, however, this doesn’t matter since the stack is cleaned up when the function ends anyway, so when you call it again it will not be the same table because the variable is local to the function, this is just a useless assignment, just remove it.

next, when preloading animations, you create an array for each item and preload it like that. You could just remove the enclosing loop, and pass the result of folder:GetChildren() to ContentProvider:PreloadAsync directly, which saves on a loop, and on a bunch of extra tables. (from the comments, this is temporary, so doesn’t matter, just a heads up ig).

You can use ContextActionService.LocalToolEquipped and ContextActionService.LocalToolUnequipped instead of Instance.ChildAdded, this way you don’t have to check if the child is a tool, and it saves on time that would normally be wasted on checking the type of the instance.

Here:

local hitbox = tool:WaitForChild("Hitbox")

you are waiting until hitbox exists, but here you’re continuously checking for it:

if not tool:FindFirstChild("Hitbox") then break end

which will slow down your code, for no reason, because you’re already sure hitbox exists, because otherwise your code wouldn’t even be running in the first place, and would still be at the wait for child call with a warning.

I might be blind, but you’re not using startTime, remove it, it’s just a waste.
Also, why does detectTool exist? I know it’s called, when a tool is equipped/unequipped, but… why? Why can’t you just… have one local script per tool, or if you don’t want to, have one module script per weapon that represents the tool? Same with setupCharacter, why does this exist, why isn’t this just a local script, you wouldn’t have this issue with a local script, since it resets when the player respawns.

finally, the first script has no reason to be a module, you’re returning an empty table. This should just be a local script. In fact, none of these scripts should be separate modules, just merge these into one local script. You don’t need to duplicate dependencies, since they are mostly the same, and you can inline playing animations inside of your activate function.

This feels like those recovering OOP developers that write everything in OOP, instead of using the paradigm that works best., because they don’t know any better.
My last point, is the while loop, I don’t think it should be there, I don’t know why, which is why I saved this for last, but it just feels odd. That’s just my opinion though.

Hope this helps. (edit: you probably have impostor syndrome, just from the first paragraph of your post, but I’m no psychologist, so idk)

#3

The reason why he’s using debris is because debris is a pretty fast way to remove stuff. its pretty consistent, but still needs more work to make it right.

  1. you can use a timed wait on waitforchild so you can do tool:WaitForChild(“Hitbox”,5) and if it doesn’t exist then you can just ignore anything as it will return nil
  2. Yes, animation track can be used by itself without having to clone it.
  3. Yes, instance:Destroy() does remove the object, and doesn’t remove the listed momory alloc. Recommend adding a instance.Destroying:Once(function() x = nil end) call to remove the object from the script to actually do anything.
  4. Yes, if anything is in a local gets added to a function, it autocleans once done.
  5. Single scripts in every tool is even more annoying to code. Recommend adding it as a module in Replicated Storage.
2 Likes
#4

Yes, I agree, which is why I put my tools in modules, where each tool is just a module with a unique :attack() function, well not always, this is a simplified example but you know what I mean.

You’re right, I forgot if the script has references, the garbage collector doesn’t free it.

I was talking about how the check is useless, because you already checked before using WaitForChild, I wasn’t complaining about the risk of infinite yields, since I trust that Hitbox will probably always exist in the tool, but I don’t know this developer’s (system. edit: I’m high, yes I agree, you can ignore everything if hitbox is nil, I skipped over that part somehow)

#5

Working to incorporate all of this so thanks for the help, though I have a couple questions,

Maybe I’m doing it wrong, but I was under the impression one script structure is good? one client script with modules and one server script with modules, that’s what I was aiming for, which is why it’s also not in just a client script, there’s no scripts directly in tools either, everything is in StarterPlayerScripts. And the reason I then have an animations module is because I planned to play every animation within that module such as for example emotes as well as the attack animations.

#6

Script structure looked fine at a glance. I don’t really care how things look as structure.
In the programming scene, there is only one thing that is always consistent:
“If it works, it works, don’t touch it or try to change it, unless you want to.”

#7

In terms of the tool, what’s the best way to implement it? E.g., a module inside the tool, a local script within the tool, or everything in StarterPlayerScripts, or is it just preference?

#8

And as for your last part of the overview, you feel as if I shouldn’t use the while loop,
Do you suggest anything as an alternative? or maybe have a little more insight to why I shouldn’t do that?
Maybe I should use RunService or something? but even then why, what would be the benefit?

#9

As I stated before. Recommend putting it inside of ReplicatedStorage, as RS can be used by both server/client. you can use something like require(x)() you can use it like this requirement if you return function(x) end

#10

In what scenario would I need the server to access a local script tool module though?

#11

Not really often. Its just good practice to put stuff into Rep Storage incase if you want to put it onto server or disable it.