How I can improve my "AI" Code?

Help and Feedback Code Review
,
#1

Hello! I’m making this thread to know how I can improve my “AI” System. What it does is basically follow the player. My code:

wait(5)
--\\VARIABLES://--
local Character = script.Parent
local Humanoid = Character.Humanoid
local RegionPart = Character.RegionPart
local InitialPoint = Character.Torso.Position
local MoveToStart
local ChasingCharacterName = ""
local WalkAnimation
--\\END OF VARIABLES.//--

Humanoid:SetStateEnabled(Enum.HumanoidStateType.Ragdoll,false)
Humanoid:SetStateEnabled(Enum.HumanoidStateType.Climbing,false)
Humanoid:SetStateEnabled(Enum.HumanoidStateType.RunningNoPhysics,false)
Humanoid:SetStateEnabled(Enum.HumanoidStateType.Seated,false)

--\\THREAD SECTION://--
local DetectThread = coroutine.wrap(function()
while wait(0.1) do
local Region = Region3.new(RegionPart.Position - (RegionPart.Size/2), RegionPart.Position + (RegionPart.Size/2))
local Parts = workspace:FindPartsInRegion3WithIgnoreList(Region, Character:GetChildren())
for i,v in pairs(Parts) do
    if v.Parent:FindFirstChild("Humanoid") and v.Parent.Parent ~= workspace["Passive NPCS"] then
	    if ChasingCharacterName ~= "" then 
	      if ChasingCharacterName == v.Parent.Name then
			ChasingCharacterName = v.Parent.Name
	        Humanoid:MoveTo(v.Parent.Torso.Position)
		    print("Found a character. Character name: "..v.Parent.Name)
            MoveToStart()
	       end
	    end
    end
end
end
end)
DetectThread()

function WaitThreadStopMoving()
local CWaitThread = coroutine.wrap(function()
wait(5)
if ChasingCharacterName == "" then
MoveToStart()
Humanoid:MoveTo(InitialPoint)
print("Moving to Initial Point...")
end
end)
CWaitThread()
end
--\\END OF THREAD SECTION.//--

--\\ANIMATION AND EVENTS SECTION://--
local AnimationInstanceWalk = Instance.new("Animation")
AnimationInstanceWalk.AnimationId = "rbxassetid://180426354"
WalkAnimation = Humanoid:LoadAnimation(AnimationInstanceWalk)

MoveToStart = function()
if WalkAnimation.IsPlaying == false then
WalkAnimation:Play()
end
end

Humanoid.MoveToFinished:Connect(function()
ChasingCharacterName = ""
WaitThreadStopMoving()
WalkAnimation:Stop()
end)
--\\END OF ANIMATION AND EVENTS SECTION.//--

Any help is appreciated!

5 Likes
#2

If the code is working, then all I can suggest is indenting. Also do a stress test with around 100 of these and see if the game lags. If it does I would suggest trying to make a custom humanoid if this is going to be used in a game. Other then that it looks good :slight_smile:

1 Like
#3

As someone who is about to look into their first AI system, your comment piqued my interest. Can you please expand on this? What is a custom humanoid and why would it help reduce lag?

1 Like
#4

Well basically roblox’s humanoids have alot of overkill features that generally are not required in a npc. There for, we instead just emulate one(this wont be needed when luanoids are released). Its as simple as just adding for instance in this case, just a animation controller with a character model by the looks of it, and a bindable event for movetofinished.

#5

If your going to use this many times, I recommend using modules, just makes everything easier to just add methods to do little tasks and overall keeps your code nice and organized. Also like @Dev_HDWC said, you should indent your code, its just good practice :slight_smile:

#6

Thanks! I have a plugin to indent my code. I forgot to use it :sweat_smile:. I will look to make my own module too.

1 Like
#7

It looks great!

One suggestion I have is pathfinding if you want your AI to maneuver through a map:

1 Like
#8

Just a suggestion, you should check out behavior trees, it will help you out tons.

1 Like
#9

If you don’t plan on expanding your code to be any more complex, I would recommend against refactoring it to use modules, custom character controllers, pathfinding or behavior trees. Your way of doing it is a lot simpler, and because it’s such a short program it’s not hard to understand. If it needs to work together with a lot of other code in a complex way, then it might make sense but that’s not clear from your post.

All in all, your script is not bad or unreadable, but it could be made a lot better. Here are my thoughts on how to do that:

In DetectThread, you can change the ignore list like so:

local Parts = workspace:FindPartsInRegion3WithIgnoreList(Region, {Character, workspace["Passive NPCs"]})

The ignore list ignores instances AND all of their descendants, so you don’t need to ignore the children of Character, you can just ignore the Character itself. By ignoring the Passive NPCs folder as well, you can omit the check that you do later on.

You don’t need the comments telling the reader where a “section” ends, that’s clear from the section headers. It’s especially not necessary when the script is this short. I’d argue that the section headers are also pretty unnecessary in this case. You’d be much better off with comments describing what the different coroutines and loops do, as well as a short explanation of the purpose of the script at the top.

The logic that uses ChasingCharacterName doesn’t seem right. There’s no way it can every be anything other than "", because the code that makes it something else only runs if it’s already different from "", so I’d be surprised if this even works in the first place. If you need to refer to a specific character, just do that instead of the name of the character that you have to search for later.

If that part of the code is actually correct, then you should move the "if ChasingCharacterName ~= “” then " check outwards, so it comes before the whole Region3 business. That way you don’t unnecessarily find parts in the region if it never gets used anyway. If you plan on having a lot of AI agents then this is a pretty good place to optimize it without any effort.

Instead of using a Region3 with a blacklist, use a whitelist. That way you can explicitly define exactly which parts you want instead of which ones you don’t want, which is guaranteed to become outdated as your project grows. You can use CollectionService to easily get a list of things tagged with e.g. “ai_target”) and only look for those in the Region3.

Using coroutines in this way doesn’t actually do anything for you, it just makes the logic hard to follow. It would be a lot clearer if it was just a main loop that calls functions for finding a target, chasing it, and returning to the original position. That way the reader can see “what” happens all in the same place, and look at the functions for details on “how” it happens if they care.

The wait(5) at the top is unnecessary and not guaranteed to be enough time. You should use WaitForChild if that’s your reason for even having it in the first place.

The “RegionPart” and “WaitThreadStopMoving” variable names are unclear. I’d make the first one longer to describe what it’s for, and make it clear from the variable name what the second one is supposed to do.

The MoveToStart variable name is unclear because it’s forward- declared, which in your case is necessary because of the way you organized stuff into sections. Declare and define MoveToStart at the same time, and move the calls to CWaitThread and DetectThread further down if you insist on using coroutines. If you think that makes it harder to read, you can have an initialization function that’s defined at the top of your script but called on the very last line. That way everything that needs to be is defined before it’s used, and it’s much easier for the reader to follow the flow of the script, i.e. what gets executed when.

3 Likes
#10

Wow! Thanks alot! About the coroutines I actually use them to code not yield, and I’m pretty bad naming parts, I will consider everything here, this wont only help me on the AI script but help me in every script that I use region3 or I name bad variables names. Thanks alot again. What I did to improve the performance was add a table with all characters that I want AI to search. Using the Region3 With WhiteList. :happy1:

#11

I’m happy to hear that :slight_smile:

You can use coroutines to prevent the main thread from yielding, which is useful if you have several loops that need to run at the same time. But you don’t actually need more than one main loop in this script, so if you wanted to you could get rid of them and I think that’d make the code even clearer. The “detection loop” doesn’t need to run at the same time as the “chasing loop”, as far as I can tell.

3 Likes
#12

Alright I will remove them, thanks alot again!