I’m trying to make a combat system like the one in Toontown. I’ve come far, into making multiple people joining the battle.
I would like someone to help me make this code more efficient and structured better.
local Players = game:GetService("Players")
local ReplicatedStorage = game:GetService("ReplicatedStorage")
local ChatService = game:GetService("Chat")
local BattleTemplate = ReplicatedStorage:WaitForChild("BattleTemplate")
local inBattle = script.Parent.Stats.InBattle
local battleCopy
local inBattleTable = {}
local cog = script.Parent
local db = false
local fr = false
local function stopMoving(Player) -- CFrame function too
for i, v in pairs(inBattleTable)do
inBattleTable[i].Character.Humanoid.WalkSpeed = 0
inBattleTable[i].Character.Humanoid.JumpPower = 0
end
end
script.Parent.Touch.Touched:Connect(function(hitPart)
if hitPart.Parent:IsA("Model") and hitPart.Parent:FindFirstChild("Humanoid") then
local Player = Players:GetPlayerFromCharacter(hitPart.Parent)
if Player then
if not inBattle.Value then
table.insert(inBattleTable, 1, Player)
inBattle.Value = true
battleCopy = BattleTemplate:Clone()
battleCopy.Parent = workspace
stopMoving(Player)
script.Parent.Humanoid.WalkSpeed = 0
ChatService:Chat(script.Parent.Head, "You're going to look good on my resume.", "Red")
battleCopy:SetPrimaryPartCFrame(script.Parent.Foot_Right.CFrame)
wait(2)
script.Parent:MoveTo(battleCopy.CogSpot1.Position)
Player.Character:MoveTo(battleCopy.PersonSpot1.Position)
Player.Character.HumanoidRootPart.CFrame = CFrame.new(Player.Character.HumanoidRootPart.Position, script.Parent.Head.Position)
Player.PlayerGui.BattleUI.Enabled = true
elseif inBattle.Value and #inBattleTable <= 4 and not db then
db = true
print(#inBattleTable)
if #inBattleTable == 3 then
table.insert(inBattleTable, 4, Player)
Player.Character:MoveTo(battleCopy.PersonSpotLeft.Position)
Player.Character.HumanoidRootPart.CFrame = CFrame.new(Player.Character.HumanoidRootPart.Position, script.Parent.Head.Position)
stopMoving(Player)
end
if #inBattleTable == 2 then
table.insert(inBattleTable, 3, Player)
inBattleTable[1].Character:MoveTo(battleCopy.PersonSpotRight.Position)
inBattleTable[2].Character:MoveTo(battleCopy.PersonSpot2.Position)
Player.Character:MoveTo(battleCopy.PersonSpot1.Position)
Player.Character.HumanoidRootPart.CFrame = CFrame.new(Player.Character.HumanoidRootPart.Position, script.Parent.Head.Position)
stopMoving(Player)
end
if #inBattleTable == 1 and fr then
table.insert(inBattleTable, 2, Player)
inBattleTable[1].Character:MoveTo(battleCopy.PersonSpot2.Position)
Player.Character:MoveTo(battleCopy.PersonSpot1.Position)
Player.Character.HumanoidRootPart.CFrame = CFrame.new(Player.Character.HumanoidRootPart.Position, script.Parent.Head.Position)
stopMoving(Player)
end
fr = true
wait(2)
db = false
end
end
end
end)
You can use only Players:GetPlayerFromCharacter() to detect whether it’s a player or not, the other things are just a waste of space because that function is powerful enough to check if it’s a player’s or not.
You can actually use table.insert(inBattleTable,Player), instead of giving it an index.
Since I can’t fully understand your full code, that’s all I can give.
A basic character should be a model and have a humanoid right? The players:GetPlayerFromCharacter() function acts like the conditional statement, which checks if it’s a model, has a humanoid and check it’s a player through C++ code, so that function saves you lot of space already.
Do make your variable names as specific as possible for readability. I’m assuming your db variable stands for debounce. Same applies for your fr variable.
local debounce = false
In your stopMoving function, in your loop you seem to have extra indexing when the inBattleTable[i] is already cached as v. so instead you could do:
local function stopMoving(Player) -- CFrame function too
for i, v in pairs(inBattleTable)do
v.Character.Humanoid.WalkSpeed = 0
v.Character.Humanoid.JumpPower = 0
end
end
Note that having to index a key like ‘inBattleTable[i]’ does take time. Also using elseif statements are faster than having a bunch of if statements in the same scope.
Also you seem to be really repetitive here:
if #inBattleTable == 2 then
table.insert(inBattleTable, 3, Player)
inBattleTable[1].Character:MoveTo(battleCopy.PersonSpotRight.Position)
inBattleTable[2].Character:MoveTo(battleCopy.PersonSpot2.Position)
Player.Character:MoveTo(battleCopy.PersonSpot1.Position)
Player.Character.HumanoidRootPart.CFrame = CFrame.new(Player.Character.HumanoidRootPart.Position, script.Parent.Head.Position)
stopMoving(Player)
end
if #inBattleTable == 1 and fr then
table.insert(inBattleTable, 2, Player)
inBattleTable[1].Character:MoveTo(battleCopy.PersonSpot2.Position)
Player.Character:MoveTo(battleCopy.PersonSpot1.Position)
Player.Character.HumanoidRootPart.CFrame = CFrame.new(Player.Character.HumanoidRootPart.Position, script.Parent.Head.Position)
stopMoving(Player)
end
Instead you can create a function taking the #inBattleTable as an argument. Here is an example:
-- I'm assuming the function already has access to the Player variable
local function exampleName(tableLength)
tableLength = ( type(tableLength) == 'number' and tableLength ) or error(('Invalid datatype given: %s'):format(type(tableLength))
table.insert(inBattleTable, tableLength + 1, Player)
Character:MoveTo(battleCopy.PersonSpotLeft.Position)
Character.HumanoidRootPart.CFrame = CFrame.new(Character.HumanoidRootPart.Position, script.Parent.Head.Position)
stopMoving(Player)
end
This will make your code a lot less messy and more readable. Having a variable for the character would also be good to prevent extra indexing.
API being deprecated also doesn’t mean it’s bad. It just means that roblox is not going to update it anymore.
Yes but it is bad practice if you decide to use deprecated functions because not only it doesn’t follow convention, it can possibly break your code in the near future.
I don’t think roblox will remove / break deprecated functions. If that was the case, some roblox games would break. The only time roblox broke games was when they converted to Filtering Enabled. And the most updated solution roblox offers doesn’t mean it’s the best.
Deprecated functions can be removed at any time. As a general practice, deprecated APIs should always be avoided in new code no matter the context in software development.
It is in the name, after all. Deprecation labels that API as inefficient, unsafe, or superceded by a more maintainable, safer, more efficient, more expressive, or just a better solution overall.
In this case, CFrame.new(position, lookAt) has been superceded by CFrame.lookAt(at, lookAt, up).
Why would roblox remove deprecated functions if that would break a few older games from functioning properly. Also it seems most developers are not fond of the new CFrame.lookAt and are still continuing to use CFrame.new(). Instead of providing a better replacement, roblox gave a more inconvenient and inefficient one to use as seen from these threads.
What are you talking about? CFrame.lookAt is the better replacement; it is fully backwards compatible, is more powerful since it allows you to provide upVector, and reads more clearly in your code. If you read my thread that you’ve linked, a staff member confirms that lookAt was the intended replacement for the now deprecated CFrame constructor. The slower method was using fromMatrix, which was provided as an alternative to lookAt on the devhub while the new lookAt method did not exist. In all cases the new lookAt method is superior, and you should prefer using it.
