Are these scripts optimized? / Will these scripts prevent exploits?

The question is in the title, I’m a meh scripter and I have no clue if these scripts satisfy my needs.
Here are the scripts for a SINGLE weapon.

LocalScript
local plr = game:GetService("Players").LocalPlayer
local char = plr.Character or plr.CharacterAdded:Wait()
local hrp = char:WaitForChild("HumanoidRootPart")
local hum = char:WaitForChild("Humanoid")

local uis = game:GetService("UserInputService")

local rep = script.Parent:WaitForChild("Hahalol3")

local db = false
local dbe = false

-- Check if animation exists
local idle = script:FindFirstChild("Idle")
if not idle then
	warn("darn you suck at this. (Idle) not found")
	return
end

local swing = script:FindFirstChild("Swing")
if not swing then
	warn("darn you suck at this. (Swing) not found")
	return
end

local weee = script:FindFirstChild("Weee")
if not weee then
	warn("darn you suck at this. (Weee) not found")
	return
end

local animator = hum:WaitForChild("Animator")
local idletrack = animator:LoadAnimation(idle)
local swingtrack = animator:LoadAnimation(swing)
local weetrack = animator:LoadAnimation(weee)

script.Parent.Equipped:Connect(function()
	if idletrack then
		idletrack:Play()
	end
	
	uis.InputBegan:Connect(function(i, c)
		if c then return end
		if i.KeyCode == Enum.KeyCode.E then
			if weetrack then
				if dbe == false and db == false then
					dbe = true
					db = true
					weetrack:Play()
					rep:FireServer("mrbeastgivememoney")
					task.wait(1)
					db = false
					task.wait(4)
					dbe = false
				end
			end
		end
	end)
end)

script.Parent.Unequipped:Connect(function()
	if idletrack then
		idletrack:Stop()
	end
end)

local conn = nil

uis.InputBegan:Connect(function(i)
	if i.UserInputType == Enum.UserInputType.Keyboard then
		conn = script.Parent.Activated:Connect(function()
			if swingtrack then
				if db == false then
					db = true
					dbe = true
					swingtrack:Play()
					rep:FireServer("mrfeast")
					task.wait(1.2)
					db = false
					dbe = false
				end
			end
		end)
	else
		if conn then
			conn:Disconnect()
			conn = nil
		end
	end
end)
Script
local hitboxx = require(game.ServerScriptService.MuchachoHitbox)

local players = game:GetService("Players")

local run = game:GetService("RunService")

local function getPlayerStats(plr)
	if not plr then return nil end

	local stats = plr:FindFirstChild("Stats")
	if not stats then return nil end

	local strength = stats:FindFirstChild("Strength")
	local defense = stats:FindFirstChild("Defense")
	local health = stats:FindFirstChild("Health")
	local remainingPoints = stats:FindFirstChild("RemainingPoints")

	return {
		strength = strength,
		defense = defense,
		health = health,
		remainingPoints = remainingPoints
	}
end

local calculatedDamage

local function HahaCalculationFunny(player)
	local stats = getPlayerStats(player)

	if not stats or not stats.strength then
		warn("Player stats or strength not found")
		return
	end

	run.Stepped:Connect(function()
		calculatedDamage = stats.strength.Value * 2 / 0.5 + 10
	end)
	print("Calculated damage for", player.Name, ":", calculatedDamage)
end

for _, player in players:GetPlayers() do
	HahaCalculationFunny(player)
end

players.PlayerAdded:Connect(HahaCalculationFunny)

local rep = script.Parent:WaitForChild("Hahalol3")

local conn

rep.OnServerEvent:Connect(function(player: Player, act: string)
	if act == "mrbeastgivememoney" then
		local character = player.Character
		if not character then return end
		
		local humanoid = character:FindFirstChild("Humanoid")
		if not humanoid then return end
		
		local originalWalkSpeed = humanoid.WalkSpeed
		
		conn = run.Stepped:Connect(function()
			humanoid.WalkSpeed = 8
			task.wait(0.62)
			humanoid.WalkSpeed = originalWalkSpeed
			conn:Disconnect()
		end)

		local humanoidRootPart = character:FindFirstChild("HumanoidRootPart")
		if not humanoidRootPart then return end

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

		local hitbox2 = hitboxx.CreateHitbox()
		hitbox2.Size = Vector3.new(5, 7, 5)
		hitbox2.Offset = CFrame.new(0, 0, -5)
		hitbox2.CFrame = humanoidRootPart
		hitbox2.DetectionMode = "Default"
		hitbox2.OverlapParams = overlap
		hitbox2.VelocityPrediction = true
		
		task.wait(0.42)
		hitbox2:Start()

		hitbox2.Touched:Connect(function(hit, humanoid)
			script.Parent.Handle.Sound:Play()
			print(humanoid.Parent.Name)
			local creator = Instance.new("ObjectValue")
			creator.Name = "creator"
			creator.Value = player
			creator.Parent = humanoid
			game:GetService("Debris"):AddItem(creator, 5)
				
			local target = humanoid.Parent
			local vibecheck = target:GetAttribute("Blocking")
			
			if vibecheck == true then
				print("blocked")
				return
			end
				
			local targetPlayer = players:GetPlayerFromCharacter(humanoid.Parent)
			if targetPlayer and _G.applyDamageWithDefense then
				_G.applyDamageWithDefense(targetPlayer, calculatedDamage)
				humanoid.Parent.HumanoidRootPart.Orientation += Vector3.new(90, 0, 0)
				
				if humanoid.Health <= 0 then
					local sound = Instance.new("Sound")
					sound.SoundId = "rbxassetid://18299846717"
					sound.Parent = humanoid.Parent.Head
					sound:Play()
				end
			else
				humanoid:TakeDamage(calculatedDamage)
				humanoid.Parent.HumanoidRootPart.Orientation += Vector3.new(90, 0, 0)
				
				if humanoid.Health <= 0 then
					local sound = Instance.new("Sound")
					sound.SoundId = "rbxassetid://18299846717"
					sound.Parent = humanoid.Parent.Head
					sound:Play()
				end
			end
		end)
		
		task.wait(0.2)
		hitbox2:Stop()
	elseif act == "mrfeast" then
		local character = player.Character
		if not character then return end

		local humanoidRootPart = character:FindFirstChild("HumanoidRootPart")
		if not humanoidRootPart then return end
		
		local overlap = OverlapParams.new()
		overlap.FilterType = Enum.RaycastFilterType.Exclude
		overlap.FilterDescendantsInstances = {character}

		local hitbox = hitboxx.CreateHitbox()
		hitbox.Size = Vector3.new(5, 5, 5)
		hitbox.CFrame = humanoidRootPart
		hitbox.Offset = CFrame.new(0, 0, -5)
		hitbox.DetectionMode = "Default"
		hitbox.OverlapParams = overlap
		hitbox.VelocityPrediction = true
		
		task.wait(0.4)
		hitbox:Start()
		
		hitbox.Touched:Connect(function(hit, humanoid)
			print(humanoid.Parent.Name)

			local vel = Instance.new("BodyVelocity")
			vel.MaxForce = Vector3.new(1,1,1) * math.huge
			vel.Velocity = humanoidRootPart.CFrame.LookVector * 45 + humanoidRootPart.CFrame.UpVector * 20
			vel.Parent = humanoid.Parent.HumanoidRootPart
			game:GetService("Debris"):AddItem(vel, 0.1)

			local creator = Instance.new("ObjectValue")
			creator.Name = "creator"
			creator.Value = player
			creator.Parent = humanoid
			game:GetService("Debris"):AddItem(creator, 5)

			local target = humanoid.Parent
			local vibecheck = target:GetAttribute("Blocking")

			if vibecheck == true then
				print("blocked")
				return
			end

			local targetPlayer = players:GetPlayerFromCharacter(humanoid.Parent)
			if targetPlayer and _G.applyDamageWithDefense then
				_G.applyDamageWithDefense(targetPlayer, calculatedDamage / 2)
				
				if humanoid.Health <= 0 then
					local sound = Instance.new("Sound")
					sound.SoundId = "rbxassetid://18299846717"
					sound.Parent = humanoid.Parent.Head
					sound:Play()
				end
			else
				humanoid:TakeDamage(calculatedDamage / 2)
				
				if humanoid.Health <= 0 then
					local sound = Instance.new("Sound")
					sound.SoundId = "rbxassetid://18299846717"
					sound.Parent = humanoid.Parent.Head
					sound:Play()
				end
			end
		end)
		task.wait(0.1)
		hitbox:Stop()
	end
end)

Any suggestions??

3 Likes

I do see that there is a lot of nesting if statements inside the client that makes it a bit harder to read so I would change them to, for example

if not weetrack then return end

thats what u caught? :grimacing:

your scripts are not necessarily unoptimized. they arent really optimized either. also youre doing some really weird things in your code.

stats.strength.Value * 2 / 0.5 + 10

this is a prime example
but im gonna go over your whole codebase

first dont use db or dbe for cooldowns, and name your variables morew expressively. id make a cooldown module instead of using booleans (if you need one let me know). name uis to UserInputService, and name your other variables their full name. dont be lazy like this.
localize all your services not just players. and be explicit in what rep is.

local idle = script:FindFirstChild("Idle")
if not idle then
	warn("darn you suck at this. (Idle) not found")
	return
end

local swing = script:FindFirstChild("Swing")
if not swing then
	warn("darn you suck at this. (Swing) not found")
	return
end

local weee = script:FindFirstChild("Weee")
if not weee then
	warn("darn you suck at this. (Weee) not found")
	return
end

here just do WaitForChild for all the objects, and remove the conditions. i would also store animations in like replicatedstorage unless you want your tool to be a drag-n-drop free model.

local animator = hum:WaitForChild("Animator")
local idletrack = animator:LoadAnimation(idle)
local swingtrack = animator:LoadAnimation(swing)
local weetrack = animator:LoadAnimation(weee)

iirc theres no guarantee that these will load. so have a custom animation loader that ensures that it loads.

script.Parent.Equipped:Connect(function()
	if idletrack then
		idletrack:Play()
	end
	
	uis.InputBegan:Connect(function(i, c)

every time the tool’s equipped you make a new inputbegan event thats never disconnceted. this is a memory leak. have this as a standalone function

if dbe == false and db == false then
					dbe = true
					db = true
					weetrack:Play()
					rep:FireServer("mrbeastgivememoney")
					task.wait(1)
					db = false
					task.wait(4)
					dbe = false
				end

this is poorly written and you cant code like this long-term. using a cooldown module removes code like this.

local rootCooldownHandler = cooldownHandler.New()
--- ... 
if (not rootCooldownHandler:Has({'swing', 'recovery'})) then
					rootCooldownHandler:Set('swing', true, 1) -- 1 second
                    rootCooldownHandler:Set('recovery', true, 5) -- 5 seconds
					attackAnimation:Play() -- weetrack
					attackRemote:FireServer() -- also use either a free module from communityservice for networking, or make your own. using straight remote events arent very sustainable in the long-run.
				end

also here:

script.Parent.Unequipped:Connect(function()
	if idletrack then
		idletrack:Stop()
	end
end)

this should disconnect ALL connections. including the inputbegan and other one(s).
personally, since i dont use roblox tools i make my own, i would also bind the unequip function to AncestryChanged.

memorize this structure:

tool.AncestryChanged:Connect(function(_, p)
    if (not p) then -- if its destroyed then
        unequip()
    end
end)

just to handle any other cases where unequipped is potentially not fired. also variabilize script.Parent as tool


local function HahaCalculationFunny(player)
	local stats = getPlayerStats(player)

	if not stats or not stats.strength then
		warn("Player stats or strength not found")
		return
	end

	run.Stepped:Connect(function()
		calculatedDamage = stats.strength.Value * 2 / 0.5 + 10
	end)
	print("Calculated damage for", player.Name, ":", calculatedDamage)
end

for _, player in players:GetPlayers() do
	HahaCalculationFunny(player)
end

why are you naming your function like this. i genuinely dont understand why newer programmers name thier functions like this especially in a code review. also this is not well-written. you should just determine calculatedDamage either when the attack function is running, or when stats.strength is changed via stats.strength.Changed:Connect(fn). its good that your looping through all players. using stepped puts significiant stress on the server and its not even necessary here. plus you never disconncet the connection so this is a memory leak too.

local conn

rep.OnServerEvent:Connect(function(player: Player, act: string)
	if act == "mrbeastgivememoney" then
		local character = player.Character
		if not character then return end
		
		local humanoid = character:FindFirstChild("Humanoid")
		if not humanoid then return end
		
		local originalWalkSpeed = humanoid.WalkSpeed
		
		conn = run.Stepped:Connect(function()
			humanoid.WalkSpeed = 8
			task.wait(0.62)
			humanoid.WalkSpeed = originalWalkSpeed
			conn:Disconnect()
		end)

each player needs their own conncetion. otherwise if a player swings and then x < 0.62 seconds later another player swings, the first players walkspeed is never set back to their original walkspeed. also a lot of games handle walkspeed by setting it directly and it always yields unexpected behaviour ive seen it so so many times. instead, i came up with my own solution, a prioritizer module. if you add something to prioritizer.New() using ::Set(priority, data), then that data is added to the table in the form of local allPriorities = {{priotity, data}, {priroity2, data2}, ...}, and then that allPriorities array is sorted using table.sort(allPriorities, fn) such that the highest priority is first. and then after the sort, u call a callback function which takes in the first elemetn of allPriorities allPriorities[1] and uses its data to set the player walkspeed. and removing a walkspeed is simply myPrioritiZer:Remove(priority) and then you sort again.
this is elite ball knowledge, came up w it myself but i digress. this would solve your issue tho, and u create a new prioritizer for each player

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

		local hitbox2 = hitboxx.CreateHitbox()
		hitbox2.Size = Vector3.new(5, 7, 5)
		hitbox2.Offset = CFrame.new(0, 0, -5)
		hitbox2.CFrame = humanoidRootPart
		hitbox2.DetectionMode = "Default"
		hitbox2.OverlapParams = overlap
		hitbox2.VelocityPrediction = true

on the server i would use magnitude. this reduces stress.

task.wait(0.42)

each weapon must have different windup lengths, right? otherwise thats a terrible combat system. design-wise.

if targetPlayer and _G.applyDamageWithDefense then
				_G.applyDamageWithDefense(targetPlayer, calculatedDamage)

don’t use _G use modules.

local sound = Instance.new("Sound")
					sound.SoundId = "rbxassetid://18299846717"
					sound.Parent = humanoid.Parent.Head
					sound:Play()

handle sounds on client. saves the server a lot of potential stress. just remoteEvent:FireAllClients('combatSound', targetHumanoidRootPart)

also i realized that each attack has its own conditino on the server. dont do that. have one central attack/hitbox function and just change the delays and damageoutput and whatnot. youre writing too much repeititve boilerplate code.

you can make your weaponsettings a modulecript per weapon, and can reuse the hitboxing fucntion for every weapon not just per tool.


they will not prevent exploits all especially since you posted it online

if i were still in my peak roblox days i would log all your remotes and observe that remote:FireServer(‘mrfeast’) correlates with attacking. i would spam the remote to insta kill whoevers in front of me. also this isn’t known by most game devs, but to further epxloit this i would probably rotate myself really fast in circles to get a kill aura, or force my character to point to the nearest player at all times and have a kill aura that way. i dont know what to call this exploit but before you use the humanoid cframe to get a hitbox i would see if that cframe’s verified and non exploited.
on the server i’d constantly check to see if the difference between the lsat and current humanoidrootpart cframe and/or velocity is valid, and then set a .verifiedCFrame variable for the player, and use that for the hitbox.

also get rid of those dumbass ‘act’ names. it wont work for exploit prevention. preventing exploits is active rather than passive.


overall your codes decent. i just have more experience so i notice these things a lot easier. theres just a few mistakes. while your codes not necessarily unoptimized, theres a lot of fatal flaws that let it be exploitable. my ‘corrections’ arent even all the things that can be corrected because any more would require years of programming experience and understanding. eventually your combat system security will also be bulletproof it just takes a lot of time and perfectionism.

so to answer the title: your scripts are not optimized, and they will not prevent exploits.

To clarify things,

  • The act names are changed specifically for this post. They are different than what I posted here.
  • The names I give for functions can be critisized but to be completely honest, I like naming them stupid things because I’m a guy that can’t take things seriously sometimes so they’re for just for giggles.
  • The hitbox module I use is MuchachoHitbox. ← Should’ve included this in the post so MY BAD (You can see it here)

but other than that, ouch, you did NOT hold back. My apologies, you seem genuinely offended.

1 Like

first dont use db or dbe for cooldowns, and name your variables morew expressively. id make a cooldown module instead of using booleans (if you need one let me know).

Yes please, oh my god.
Also, why not use booleans?

1 Like

The reason why I have just that because I had to go when I started my message so I just did that to help get more message in.

2 Likes

nah your good i was not offended except when you named your function HahaCalculationFunny
nah but actually tho i hope i helped you out a ton, sorry for being too critical i wrote that like 20 minutes after waking up

here you go

local cooldown = {}
cooldown.__index = cooldown

function cooldown.New()
	return setmetatable({list={}}, cooldown)
end

function cooldown:Set(name, length)
	-- just incase you have a function where it doesnt check if the cooldowns active but also applies it
	-- i added functionality where u can restart cooldowns here
	
	local ptr = {}
	self.list[name] = ptr
	task.delay(length, function()
		-- check if list still exists. it acts as a quick 'isDestroyed' variable
		if (not self.list) then
			return
		end
		
		if (self.list[name] == ptr) then
			self.list[name] = nil
		end
	end)
end

function cooldown:_singleHas(name)
	if (self.list[name]) then
		return true
	end
end

function cooldown:_multiHas(names)
	for _, name in ipairs(names) do
		if (self:_singleHas(name)) then
			return true
		end
	end
end

function cooldown:Has(names)
	local typeNames = type(names)
	if (typeNames == 'table') then
		return self:_multiHas(names)
	else
		return self:_singleHas(names)
	end
end

function cooldown:Destroy()
	self.list = nil
	setmetatable(self, nil)
end

return cooldown

i just made this.
the reason off the top of my head is that you generally want certain levels of complexity when ur coding to avoid any unintentional behaviour. thats why. i dont really think of the reason, but its much more organized than raw booleans

Could you also help me set it up? I’m not good at using ModuleScripts. I could invite you to edit my game. : D

im good but heres how you do it step by step

  1. make a new module
  2. paste the code in it (erase everything beforehandd)
  3. make a script
  4. do local cooldownHandler = require(path.to.module) in that script
  5. do myCooldown = cooldownHandler.New() to create a new cooldown instance
  6. now u can call myCooldown:Has(...), or myCooldown:Set(...). ::Set(name, length) creates a value for length amount of seconds, Has(name or tabelOfNames) checks to see if any of the name(s) are active from running ::Set
  7. make sure once your done with the cooldown handler u call Destroy

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