More effective way to do this? (While loops)

Help and Feedback Code Review
#1

Hi there, I know using infinite while () loops isn’t a good idea but I’m not sure how else I’d go about doing this with the desired effect?

Script:

while wait() do 
    local distance = 10
    for _, v in pairs (game.Workspace:GetChildren()) do
        if v:IsA("Model") and v.Name ~= "Dummy" then
            for _, npc in pairs (NPCs:GetChildren()) do
                if npc:IsA("Model") and npc:FindFirstChild("Humanoid").Health > 0 then
                    local humanRP = v:WaitForChild("HumanoidRootPart")
                    if (npc.HumanoidRootPart.Position - humanRP.Position).magnitude < distance then
                        npc.Humanoid:MoveTo(humanRP.Position)
                    end
                end
            end
        end
    end
end

Thanks,

1 Like
#2

I don’t understand your code 100% but maybe you could run it every time the humanRP position changes

2 Likes
#3

You should avoid using wait(), use RunService instead.

1 Like
#4

I don’t think you can do that because this code is run on the server. Run service cannot be used on the server

#5

Oh, I forgot about that.

Anyways, I found a community tutorial on how to avoid using wait() and I think it will greatly increase the reliability of your code:

#6

Oh my. I use while wait() do alot for my NPCs systems and server-sided stuff.

#7

RunService.Stepped and RunService.Heartbeat still works for the server. I recommend using Heartbeat.

Edit : Oops your not the poster, sorry.

1 Like
#8

RunService was the one that only works for the Client. My bad. Sorry.

1 Like
#9

local humanRP = v:WaitForChild("HumanoidRootPart")

  1. WaitForChild means if any of them don’t have and never will have a root part, then this loop will lock up forever.
    Instead, just ignore anything that doesn’t contain a rootpart (does not :FindFirstChild(“HumanoidRootPart”), or, slightly better yet, does not have a Model.PrimaryPart.

  2. For that matter, why are you finding v’s HumanoidRootPart multiple times? It doesn’t change between loops. Move the line with humanRP 2 lines upward. No change in behavior whatsoever, but an appreciable increase in performance.

  3. A NPC may find that two different vs are within 10 studs of them. If so, it will MoveTo twice. Flip the loops so that NPCs are looking for vs to move to, instead of having vs look for NPCs that should move to them. This will also let you look for the closest v, instead of sometimes abandoning closer targets to ones that randomly happen to be later in the workspace’s child list.

  4. If v is supposed to be player characters, then just iterate over players instead, which lets you avoid iterating over everything in workspace.

  5. The algorithmic time this loop takes to run is (m*n), where m is players (I assume) and n is NPCs. If there are few of one and few or many of the other, you’re fine. But if there are many of both, then you’re in big trouble.
    e.g. 1 player and 50 NPCs is 50 checks. Add one more player and you get 100 checks. 5 players, 250 checks. It adds up extremely fast.
    There’s no “simple” way to solve this, you’d have to split the world into rooms/regions/octrees/whatever positional data structure to avoid checking players/NPCs that obviously aren’t near each other.

  6. I suggest renaming v to target for clarity

  7. If you keep the NPCs model clean of anything that isn’t a NPC, then you can turn
    if npc:IsA("Model") and npc:FindFirstChild("Humanoid").Health > 0 then
    into
    if npc.Humanoid.Health > 0 then
    because anything in that model is guaranteed to be an NPC.

  8. Ignore the people arguing about RunService, it does not matter at all.

  9. Consider making the loop run less frequently

3 Likes
#10

Run service can, you cant use render stepped on server, but you can use runservice.HeartBeat()

1 Like
#11

That is false, RunService can be used on both states except that you can’t use RunService.RenderStepped as it is client only.

@Friendly4Crafter No, there is no need for that unless you care about high precision. wait() should be responsive as long as you have good performance on the server and not a lot of yielded threads that need to be resumed.

@Eestlane771 Your first point isn’t 100% valid, it could be very well that the character was just loaded and the root part may not have been created fast enough on the server, since character is dynamically created.

Better alternative, it uses WaitForChild with a timeout. After 15 seconds, if there is still no humanoid root part, it’s very very likely because the exploiter deleted their humanoid root part as soon as it was added.

local primaryPart = character.PrimaryPart or character:WaitForChild("HumanoidRootPart", 15)
2 Likes