How would I make this smoother?

Help and Feedback Code Review
, , , ,
1

Hello, I am making a Miner NPC that goes to an ore, picks it up, and when its inventory is full, deposits it at a position, and I don’t know how I would make the miner pick up the ore chunks quicker and more efficienctly, here is the code for picking up chunks

		local function pickUpOre(ores)
			WalkAnim:Play()
			for _,ore in pairs(ores) do
				for count = 1,3 do
					task.wait(0.1)
					hum:MoveTo(ore.Position)
					hum.MoveToFinished:Wait() 
				end
				table.insert(oresPickedUp,ore.Name)
			end
			WalkAnim:Stop()
			return true
		end

and here is the whole script

Script 
local ReplicatedStorage = game:GetService('ReplicatedStorage')
local Players = game:GetService('Players')
local CollectionService = game:GetService('CollectionService')
local PathfindingService = game:GetService('PathfindingService')
local RaycastHitbox = require(ReplicatedStorage.RaycastHitboxV4)

local MinerNumber = 0

CollectionService:AddTag(ReplicatedStorage.AI.Miner,'Miner')

CollectionService:GetInstanceAddedSignal('Miner'):Connect(function(Miner)
	spawn(function()
		Miner.Name = Miner.Name..tostring(MinerNumber)
		MinerNumber += 1
		local humrp = Miner.HumanoidRootPart
		local hum = Miner.Humanoid
		local WalkAnim = hum:LoadAnimation(script.Parent.Anims.walk.WalkAnim)
		local MiningAnim = hum:LoadAnimation(script.Parent.Anims.Swing)
		local PickUpAnim = hum:LoadAnimation(script.Parent.Anims.PickUp)
		local oresPickedUp = {}
		local Mining = false
		local Params = RaycastParams.new()
		Params.FilterDescendantsInstances = {Miner,workspace.OreSpawningArea} --- remember to define our character!
		Params.FilterType = Enum.RaycastFilterType.Blacklist
		local newHitbox = RaycastHitbox.new(Miner.Handle)
		newHitbox:SetPoints(Miner.Handle,{Vector3.new(0,0,2)})
		newHitbox.RaycastParams = Params
		newHitbox.DetectionMode = RaycastHitbox.DetectionMode.PartMode

		local function getNearestOre()
			local Ores = workspace.Ores:GetChildren()
			local closestOre = nil
			local closestDistance = nil
			for _, Ore in pairs(Ores) do
				local my_distance = (Ore.Main.Position - humrp.Position).Magnitude
				if CollectionService:HasTag(Ore, "ToBeMined") then
					continue
				elseif closestOre == nil then -- nothing to compare add default
					closestOre = Ore
					closestDistance = my_distance
				elseif Ore.Main and closestDistance > my_distance then -- default was set, time to compare
					closestOre = Ore
					closestDistance = my_distance
				end
			end
			return closestOre
		end

		local function depositOres(walkToOre)
			local path = PathfindingService:CreatePath()

			local success, errorMessage = pcall(function()
				path:ComputeAsync(humrp.Position, workspace.DepositOres.Position)
			end)
			WalkAnim:Play()
			if success and path.Status == Enum.PathStatus.Success then
				local waypoints = path:GetWaypoints()
				for _,waypoint in pairs(waypoints) do

					if waypoint.Action == Enum.PathWaypointAction.Jump then
						hum:ChangeState(Enum.HumanoidStateType.Jumping)
					end

					hum:MoveTo(waypoint.Position)
					hum.MoveToFinished:Wait()
				end
				for _,ore in pairs(oresPickedUp) do
					require(ReplicatedStorage.FactoryResources).Resources[ore] += 1
				end
				oresPickedUp = {}
				print(require(ReplicatedStorage.FactoryResources).Resources)
				walkToOre()
			else
				task.wait(0.5)
				depositOres()
			end
		end

		local function pickUpOre(ores)
			WalkAnim:Play()
			for _,ore in pairs(ores) do
				for count = 1,3 do
					task.wait(0.1)
					hum:MoveTo(ore.Position)
					hum.MoveToFinished:Wait() 
				end
				table.insert(oresPickedUp,ore.Name)
			end
			WalkAnim:Stop()
			return true
		end

		local function breakOre(nearest)
			if Mining == false then
				CollectionService:RemoveTag(nearest, "ToBeMined")
				Mining = true
				Miner:SetPrimaryPartCFrame(CFrame.new(humrp.Position,nearest.Main.Position))
				MiningAnim:Play()
				newHitbox:HitStart()

				-- I apologize to my future self on my terrible readability on this code

				newHitbox.OnHit:Connect(function(hit)
					if hit:FindFirstAncestor('Ores') then
						local oreName = hit.Parent.Name
						local OreHealth = hit.Parent.Health.Value
						if OreHealth >= 2 then
							hit.Parent.Health.Value -= 1
							Miner.Handle.Hit:Play()
							local Particles = script.Smoke:Clone()
							delay(0.3,function()
								local newPart = Instance.new('Part')
								newPart.Transparency = 1
								newPart.Anchored = true
								newPart.Position = Miner.Handle.Attachment.WorldPosition
								newPart.Parent = Miner
								Particles.Parent = newPart
								delay(0.5,function()
									Particles.Enabled = false
									game:GetService('Debris'):AddItem(newPart,1)
								end)
							end)
						else
							Miner.Handle.Break:Play()
							local ores = {}

							for _,ore in pairs(hit.Parent:GetChildren()) do
								if ore.Name == 'Ore' then
									ore.Name = ore.Parent.Name
									ore.Anchored = false
									ore.Parent = workspace
									table.insert(ores,ore)

									ore.Touched:Connect(function(Hit)
										if Players:GetPlayerFromCharacter(Hit.Parent) then
											require(ReplicatedStorage.FactoryResources).Resources[oreName] += 1
											print(require(ReplicatedStorage.FactoryResources).Resources[oreName])
											ore:Destroy()
										elseif string.match(Hit.Parent.Name,'Miner') then
											PickUpAnim:Play()
											ore:Destroy()
										end
									end)								
								end
							end
							
							if hit.Parent and hit.Parent.Name ~= 'Workspace' and hit.Parent:IsA('Model') then
								hit.Parent:Destroy()
							end
							
							local allOresPickedUp = pickUpOre(ores)

							while task.wait(1) do

								if allOresPickedUp then
									break
								end
							end
						end
					end
				end)
				task.wait(0.75)
				newHitbox:HitStop()
				Mining = false
			end
		end

		local function walkToOre()
			if #oresPickedUp < 4 then
				local path = PathfindingService:CreatePath()

				local nearest = getNearestOre()
				if nearest == nil or nearest.Main == nil then
					task.wait(0.5)
					walkToOre()
					return
				end
				CollectionService:AddTag(nearest, "ToBeMined")

				local success, errorMessage = pcall(function()
					path:ComputeAsync(humrp.Position, nearest.Main.Position + ((humrp.Position-nearest.Main.Position).Unit * 6))
				end)
				WalkAnim:Play()
				if success and path.Status == Enum.PathStatus.Success then
					local waypoints = path:GetWaypoints()
					for _,waypoint in pairs(waypoints) do

						if waypoint.Action == Enum.PathWaypointAction.Jump then
							hum:ChangeState(Enum.HumanoidStateType.Jumping)
						end

						if nearest and nearest.Parent and (humrp.Position - nearest:GetPivot().Position).Magnitude <= 10 then
							breakOre(nearest)
						end

						hum:MoveTo(waypoint.Position)
						hum.MoveToFinished:Wait()
					end
				end

				-- removes tag if failed to mine
				if nearest and nearest.Parent then
					CollectionService:RemoveTag(nearest, "ToBeMined")
				end

				WalkAnim:Stop()
				task.wait(0.5)
				walkToOre()
			else
				depositOres(walkToOre)
			end
		end
		walkToOre()
	end)
end)

local clone = ReplicatedStorage.AI.Miner:Clone()
clone.Parent = workspace
task.wait(0.3)
local clone = ReplicatedStorage.AI.Miner:Clone()
clone.Parent = workspace
clone:SetPrimaryPartCFrame(CFrame.new(Vector3.new(-150.777, 4, 50.515)))
task.wait(0.3)
local clone = ReplicatedStorage.AI.Miner:Clone()
clone.Parent = workspace
clone:SetPrimaryPartCFrame(CFrame.new(Vector3.new(-150.777, 4, 50.515)))
task.wait(0.3)
local clone = ReplicatedStorage.AI.Miner:Clone()
clone.Parent = workspace
clone:SetPrimaryPartCFrame(CFrame.new(Vector3.new(-150.777, 4, 50.515)))


here is a video showing the bug in action, my picking up script is imprecise and depends on a for count loop to pick up the ores, making the npc stagger/jitter.

I’ve tried implementing an if ore == nil then and other methods but I couldn’t get anything to work.

I’m also worried that I won’t get a reply from this category as its so under used

2 Likes
2

I’d say the solution is to make your ore pickup script more precise, this loop really isn’t a good way to fix the problem at hand

If you would like a fix immediately, I believe you can replace

if ore ~= nil

with

if ore and ore.Parent

You know more about your code so I could be missing something, but this should work

A fix I’d recommend is entirely removing the .Touched event, you already have a function to make the npc walk to the ore, so why can’t you just directly use that function to pick up the ore after the moveto finishes?

2 Likes
3

Why repeat the pickup sequence 3 times? I feel that’s a bit redundant, so removing the loop around this section of code:

for count = 1,3 do
  task.wait(0.1)
  hum:MoveTo(ore.Position)
  hum.MoveToFinished:Wait()
end

Would likely solve your issues.

Also, as pointed out by @uwuCulturist, checking if the ore itself is nil is useless, since it’s still an Instance until it gets garbage collected. Rather, check if ore.Parent is nil, since that’s the only thing that changes.

1 Like
4

The ore pickup function is based on a .touched event, so I assume it was a way to try to bruteforce the event into firing

2 Likes
5

Thanks man, removing the .Touched event really did make this 100% precise, I don’t know how I didn’t think of that in the first place.

6

np, the purpose of asking questions like this is to learn, so now you know for future reference

2 Likes