Need some help with improving this code

Help and Feedback Scripting Support
1

I’m currently working on recreating the Wizard 101 battle system as a fun project and I have this kind of cool thing in it Animated GIF - Find & Share on GIPHY when a player touches an enemy it spawns the battle area and starts the battle between the player and the enemy however as you can see there’s 4 spots on each side this is because other players and enemies will be able to join the battle if they enter the battle area if those spaces are available of course. Although I handle the enemies on the server in a script

This is the code I have for the touch stuff (it’s a local script btw)

--For when player touches enemy that isn't in battle (so starting a battle)
for i,v in pairs(workspace.Enemies:GetChildren()) do
    v.Humanoid.Touched:Connect(function(hit)
        if hit.Parent:FindFirstChild("Humanoid") then
            if not v:FindFirstChild("In Battle") then
                if deb == false then
                    local player = players:GetPlayerFromCharacter(hit.Parent)
                    if not player then return end
                    deb = true
                    print(player,v,"Start Battle - Client")
                    startBattle(player,v) --player that started it and the enemy
                    wait(2)
                    deb = false
                end
            else
                print("Enemy In Battle")
            end
        end
    end)
end

--Joining an existing battle by touching the battle area
spawn(function()
    while wait(3) do
        for i,v in pairs(workspace:GetChildren()) do
            if v.Name == "Battle Area" then
                v.detection.Touched:Connect(function(hit)
                    print("Battle Area Detection Part Touched")
                    if hit.Parent:FindFirstChild("Humanoid") and hit.Parent:FindFirstChild("In Battle") == nil then
                        print("Humanoid Found & Not In Battle")
                        local player = players:GetPlayerFromCharacter(hit.Parent)
                        if not player then return end
                        print(player,v,"Enter Battle - Client")
                        enterBattle(player,v)
                    else
                        print("Player Is Already In Battle")
                    end
                end)
            end
        end
    end    
end)

Now this works however I just know 100% I could probably make this shorter and much nicer although I’ve just been unable to do that without breaking it :confused:

If anyone understands what I’m trying to do here and is able to help me make this not as messy and wack that would be great here’s the rest of the local script it’s just the other functions basically if you wanna know what enterBattle or startBattle is Local Script - Pastebin.com

1 Like
2 
spawn(function()
    while wait(3) do
        for i,v in pairs(workspace:GetChildren()) do
            if v.Name == "Battle Area" then
                v.detection.Touched:Connect(function(hit)
                    --...
                end)
            end
        end
    end    
end)

This is potentially really bad. You’re setting up a new touched connection every 3 seconds, even if one already exists. This can lead to hard-to-squash bugs because suddenly the event fires twice (or 50 times after 150 seconds) even though it “should” only fire once.

Instead, set up each battle area when it’s put into Workspace,

like so: 
game.Workspace.ChildAdded:Connect(function(child)
    if child.Name == "Battle Area" then
        local battleArea = c
        battleArea.Detection.TouchedConnect(function(touchingPart)
            -- ...
        end)
    end
end)

This way, a connection only gets set up once per battle arena (assuming a battle arena never enters workspace more than once). You also save the server the work of looking through every child of workspace, which could be a lot depending on how you organize things.

Your debug prints are liars. The “Player Is Already In Battle” thing prints if the touching thing didn’t have a humanoid.

Here's how I'd change the whole spawn loop battle area detection thing: 
game.Workspace.ChildAdded:Connect(function(child)
    if child.Name == "Battle Area" then
        local battleArea = c
        battleArea.Detection.TouchedConnect(function(touchingPart)
            local character = touchingPart.Parent
            local humanoid = character:FindFirstChildOfClass("Humanoid")
            local player = players:GetPlayerFromCharacter(character)

            if player and humanoid then
                local isInBattle = character:FindFirstChild("In Battle") ~= nil
                if not isInBattle then
                    enterBattle(player, battleArea)
                end
            end
        end)
    end
end)

I took out the debounce stuff because IMO it’s ugly AF and unnecessary if you actually understand how your code works. Calling enterBattle should make the isInBattle condition true before yielding, so even if several parts of a player’s character touches at the same time, it will work properly.

I “flattened” some of the deeply nested if statements, because they’re ugly to look at and hard to understand. Since the three lookups can be done without any checks without erroring, I changed it to do it like that. That way, you can turn three if statements into a single one.

Your enemy touched code has no way of dealing with new enemies being added after runtime.

I'd change it like so: 
local function setupEnemy(enemy)
    enemy.Humanoid.Touched:Connect(function(hit)
        if enemy:FindFirstChild("In Battle") then return end

        local character = hit.Parent
        local humanoid = character:FindFirstChildOfClass("Humanoid")
        local player = players:GetPlayerFromCharacter(character)

        if humanoid and player then
            startBattle(player, enemy) --player that started it and the enemy
        end
    end)
end

--For when player touches enemy that isn't in battle (so starting a battle)
for _, enemy in pairs(workspace.Enemies:GetChildren()) do
    setupEnemy(enemy)
end

workspace.Enemies.ChildAdded:Connect(setupEnemy)

Hope this is helpful, let me know if anything is weird you have any questions :slight_smile: :+1:

2 Likes
3

Oh hey you’re the guy that helped me with my previous post lol anyways this is pretty helpful and informative so thanks! Kind of funny that you said to do

game.Workspace.ChildAdded:Connect(function(child)
    if child.Name == "Battle Area" then
        local battleArea = c
        battleArea.Detection.TouchedConnect(function(touchingPart)
            -- ...
        end)
    end
end)

Because this is actually what I did in a newer version I did so cool to know I was on the right track. Anyways for enemies I don’t really have a proper system although I handle it in a script and I basically just use the same code except I check if the humanoids parent is inside the enemy folder something like that and then I move it to the enemy spaces instead of the player ones. I’ve never worked with creating ai and enemies so not sure how to do that stuff yet.

edit: Nvm I read the last part wrong I thought you were talking about something else

1 Like
4

It is a pretty bad idea to run a loop on the workspace. You should instead put the enemies in a folder.

1 Like
5

Hey is it possible to do something like this?

local function setupEnemy(enemy)
	enemy.Humanoid.Touched:Connect(function(hit)
		if enemy:FindFirstChild("In Battle") then return end
		
		local character = hit.Parent
		local humanoid = character:FindFirstChildOfClass("Humanoid")
		local player = players:GetPlayerFromCharacter(character)
	
		if humanoid and player then
			local create = createBattle:InvokeServer(player,enemy)
			if create == true then
				cameraTween(camera,battleArea.cam,2)
				player.Character.HumanoidRootPart.CFrame = CFrame.new(player.Character.HumanoidRootPart.Position, enemy.HumanoidRootPart.Position)
				playerGui.ScreenGui.Frame.Battle.Visible = true						
			end
		end
	end)
end

for _,enemy in pairs(workspace.Enemies:GetChildren()) do
	setupEnemy(enemy)
end

workspace.Enemies.ChildAdded:Connect(setupEnemy)
6

I’d name the variable something like success, to give a better idea of what it means.

local create = createBattle:InvokeServer(player,enemy)
if create == true then
    --...				
end

This is going to be an issue, because InvokeServer is yielding. That means the script stops doing anything for a while, in this case until the server has returned some value. This means the enemy might get touched again before the client-server-client round trip is done, so you can’t count on this check to actually work properly: if enemy:FindFirstChild("In Battle") then return end. You could solve this by creating a new variable in the outermost scope that keeps track of whether or not the script is waiting for a response from the server, and only do the thing if it’s not waiting.

EDIT: But TBH all of this should be managed on the server anyway, not on the client.

1 Like
7

Oh wait so instead of detecting if the player touched an enemy or battle area on the client I should do it on the server? I thought it made more sense to do it on the client lol

1 Like
8

In some cases doing hit detection on the client can make sense, but in this case it’s

a) game- critical that it only happens legitimately (if client had control, a hacker could initiate any battle from anywhere)
b) not important that it’s super resposive (if it takes a round trip of 500ms before player sees that the battle has begun, it won’t be too annoying for the player)

1 Like