Is there a better way I could do my AI?

Help and Feedback Code Review
#1

I have an npc script where players can hire npc-employees on their plot of land, and the npcs will do stuff (Guards will protect their plot from criminals, and Managers can hire workers, etc…).

Currently what I’m doing is i’m spawning a function every time a player hires a worker.

function NPCGuard()
   -- Do the NPC Stuff here
end

function NPCManager()
   -- Do the NPC Stuff here
end


HireWorker.OnServerEvent:Connect(function(plr, worker)
    if worker == "Guard" then
        coroutine.wrap(function()
            NPCGuard()
         end)()
    elseif worker == "Manager" then
        coroutine.wrap(function()
            NPCManager()
        end)()
    end
end)

I’m wondering if I should be doing all my npc code in one function rather than spawning a function every time a player hires an npc.

1 Like
#2

I do like the use of coroutines, but in this case they’re not really necessary. If there was other code after calling NPCGaurd() or NPCManager() then it’d be useful, because you wouldn’t want that code to yeild to your NPC functions.
But whenever that event fires, the handler function will run independently, so it already behaves like a coroutine.

For organization I like how you’re calling events to handle different workers.

2 Likes
#3

Organizing it like this makes it easy to add more types.

local npcTypes = {
    Guard = function()

    end,

    Manager = function()

    end
}

HireWorker.OnServerEvent:Connect(function(player, workerType, ...)
    if npcTypes[workerType] then
        coroutine.wrap(npcTypes[workerType])(...)
    else
        player:Kick() -- kick the player since they're exploiting
        -- we know this because they provided a non-implemented type
        -- make sure you don't have an error in your code,
        -- as this will kick the player for no reason
    end
end)

Note that I also added a basic sanity check and I fixed how you use coroutine.wrap():

coroutine.wrap(function()
    functionName()
    -- works, but why do it like this
end)()

coroutine.wrap(functionName)() -- looks cleaner imo
#4

Please never do this. It’s just never a good idea to punish the player this harshly for suspicious activity. Sometimes code breaks or data just doesn’t get sent and it isn’t the player’s fault. Now if you were adding a flag system then yes you could flag this as suspicious but never just do this flat out.

3 Likes
#5

That is why he would rather kick them than ban them. An innocent player would get a little frustrated and quickly get back into action while an exploiter would have a rather tough time trying to exploit the game via a poke method which would lead to a couple more days until the game is exploited after release, quite a big deal.

1 Like
#6

This isn’t a valid argument. You can’t just kick someone for no reason. Innocent players will just get frustrated and exploiters will know not to fire that remote with that parameter empty.

You should never outright interrupt the players experience. If I were playing your game and randomly got kicked, I better REALLY love your game or be getting a prize from it. Unless you have evidence (Aside from one faulty remote) that they are malicious, then you cannot take any actions.

Sure it may lead to the game not being exploited for another few hours at most, but that’s all you’re saving. An exploiter will send that remote, get kicked, go back, read what a valid request looks like, then send their own request that looks valid.

Instead you would want to have a system that keeps track of what they’re doing. Are they acting suspicious? Are they in places they shouldn’t be? Have they sent a weird request one time then followed it up with a bunch of valid requests at the same time? If these behaviors persist, congratulations you have an exploiter! (or a really glitchy player connection. It id assume that you’d have enough checks to make that statistically impossible) Now of course if you have way more sensitive events like send money or whatever then you can weigh that way more but you really shouldn’t just base it off of that.

TL;DR: you can’t just wave a kick around like it won’t accidentally go off. What you should be doing is tracking people who seem to have suspicious behaviors and not interrupt anyone until you have evidence against them.

2 Likes
#7

But this certain kick is not based on suspicion. It kicks you for doing something a normal client physically cannot accomplish. The code should be respectively tested for any errors, too. If done right it would not even have to be a gimmick.

2 Likes