How to improve my Interaction Code

Help and Feedback Code Review
#1

Basically I have an Press E to Interact with objects/parts, but i think on my opinion, is a big MESS, any suggestions or a quick way to get all Models and childrens inside the model of the model?, or anyways

I just repeated all the things in a painfull way i guess

wait(0.16)
local uis = game:GetService("UserInputService")
local Serverwerkspace = workspace
local camera = game:GetService("Workspace").CurrentCamera
--local board = script:WaitForChild("InteractionGui")
local board = game:GetService("ReplicatedStorage").InteractionGui
local ts = game:GetService("TweenService")

local player = game.Players.LocalPlayer
local character = player.Character
wait()
local head = character:WaitForChild("Head")
local rootpart = character:WaitForChild("HumanoidRootPart")
local ShowCooldown = false
local PressCooldown = false
--local MAX_DISTANCE = 35 

game:GetService("RunService").Stepped:Connect(function()	
	for i, v in pairs(game.Workspace:GetChildren()) do
		if v:IsA("BasePart") or v:IsA("Part") then
			if v:FindFirstChild("_Interactable") then
				if v:FindFirstChild("Event") then
					
				local position = v.Position
				local cameraPosition = camera.CFrame.Position
				local raycastParams = RaycastParams.new()
					wait()
				raycastParams.FilterType = Enum.RaycastFilterType.Blacklist
				raycastParams.FilterDescendantsInstances = {game:GetService('Players').LocalPlayer.Character}
			    local raycastResult = workspace:Raycast(cameraPosition, position - cameraPosition, raycastParams)
					
				local vector, onScreen = camera:WorldToScreenPoint(v.Position)
					--if (rootpart.Position - v.Position).Magnitude <= 20 and onScreen then
					if raycastResult and (v == raycastResult.Instance) and (rootpart.Position - v.Position).Magnitude <= 20 and (head.CFrame.p - camera.CFrame.p).Magnitude <= 30 and onScreen then
						
						if not ShowCooldown then
							print("In Screen And No Wall In-front")
						ShowCooldown = true
					local guiclone = board:Clone()
					guiclone.Parent = v
						-- item is on screen and player is within 20 studs
					end
				else
					if ShowCooldown then
							print("Wall Blocking Or Not In Screen")
						if v:FindFirstChild("InteractionGui") then
						 ShowCooldown = false
							v.InteractionGui:Destroy()
							end
					    end
					end
					uis.InputBegan:Connect(function(input, IsTyping)
						if IsTyping then return end

						if input.KeyCode == Enum.KeyCode.E then
							--game:GetService("RunService").Stepped:Connect(function()	
							--local camCFrame = game:GetService("Workspace").CurrentCamera.CFrame.Position
									if v:FindFirstChild("InteractionGui") then
										if v:FindFirstChild("_Interactable") then
												if not PressCooldown then
													PressCooldown = true
													local vector, onScreen = camera:WorldToScreenPoint(v.Position)
													--local myRay = Ray.new(rootpart.Position, v.Position/CFrame)

													if (rootpart.Position - v.Position).Magnitude <= 20 and onScreen then -- camera:WorldToScreenPoint(v.Position)
														--print("You Pressed E")
														local guicloned = v.InteractionGui
														local frame = guicloned.InteractionFrame
														local tweenInfo = TweenInfo.new(0.15, Enum.EasingStyle.Quart, Enum.EasingDirection.Out, 0, true, 0)
														wait()
														local sizetween = ts:Create(frame, tweenInfo, { Size = UDim2.new(0.10, 25,0.10, 25)})

														sizetween:Play()
														v.Event:FireServer()
														wait(0.2)
														PressCooldown = false

														-- item is on screen and player is within 20 studs
													end
												end
										end
									end
							--end)
						end
					end)
				end
				
				character.Humanoid.Died:Connect(function()
					if v:FindFirstChild("InteractionGui") then
						print("Deleted Incorrect GUI Given")
						v.InteractionGui:Destroy()
					end
				end)
			end
		end
		----------------------------
		if v:IsA("Folder") then
			for i, index in pairs(v:GetChildren()) do
			if index:IsA("BasePart") or index:IsA("Part") and index:FindFirstChild("_Interactable") then
				if index:FindFirstChild("Event") then

					local position = index.Position
					local cameraPosition = camera.CFrame.Position
					local raycastParams = RaycastParams.new()
					wait()
					raycastParams.FilterType = Enum.RaycastFilterType.Blacklist
					raycastParams.FilterDescendantsInstances = {game:GetService('Players').LocalPlayer.Character}
					local raycastResult = workspace:Raycast(cameraPosition, position - cameraPosition, raycastParams)

					local vector, onScreen = camera:WorldToScreenPoint(index.Position)
					--if (rootpart.Position - v.Position).Magnitude <= 20 and onScreen then
					if raycastResult and (index == raycastResult.Instance) and (rootpart.Position - index.Position).Magnitude <= 20 and (head.CFrame.p - camera.CFrame.p).Magnitude <= 30 and onScreen then

						if not ShowCooldown then
							print("In Screen And No Wall In-front")
							ShowCooldown = true
							local guiclone = board:Clone()
							guiclone.Parent = index
							-- item is on screen and player is within 20 studs
						end
					else
						if ShowCooldown then
							print("Wall Blocking Or Not In Screen")
							if index:FindFirstChild("InteractionGui") then
								ShowCooldown = false
								index.InteractionGui:Destroy()
							end
						end
					end
						uis.InputBegan:Connect(function(input, IsTyping)
							if IsTyping then return end

							if input.KeyCode == Enum.KeyCode.E then
								--game:GetService("RunService").Stepped:Connect(function()	
								--local camCFrame = game:GetService("Workspace").CurrentCamera.CFrame.Position
										if index:FindFirstChild("InteractionGui") then
											if index:FindFirstChild("_Interactable") then
													if not PressCooldown then
														PressCooldown = true
														local vector, onScreen = camera:WorldToScreenPoint(index.Position)
														--local myRay = Ray.new(rootpart.Position, v.Position/CFrame)

														if (rootpart.Position - index.Position).Magnitude <= 20 and onScreen then -- camera:WorldToScreenPoint(v.Position)
															--print("You Pressed E")
															local guicloned = index.InteractionGui
															local frame = guicloned.InteractionFrame
															local tweenInfo = TweenInfo.new(0.15, Enum.EasingStyle.Quart, Enum.EasingDirection.Out, 0, true, 0)
															wait()
															local sizetween = ts:Create(frame, tweenInfo, { Size = UDim2.new(0.10, 25,0.10, 25)})

															sizetween:Play()
															index.Event:FireServer()
															wait(0.2)
															PressCooldown = false

															-- item is on screen and player is within 20 studs
														end
													end
											end
										end
								--end)
							end
						end)
					end
					
				character.Humanoid.Died:Connect(function()
					if index:FindFirstChild("InteractionGui") then
						print("Deleted Incorrect GUI Given")
							index.InteractionGui:Destroy()
						
					end
				end)
			end
		  end
		end
		if v:IsA("Model") then
			
			for i, index2 in pairs(v:GetChildren()) do
				
				if index2:IsA("BasePart") or index2:IsA("Part") and index2:FindFirstChild("_Interactable") then
					if index2:FindFirstChild("Event") then

						local position = index2.Position
						local cameraPosition = camera.CFrame.Position
						local raycastParams = RaycastParams.new()
						wait()
						raycastParams.FilterType = Enum.RaycastFilterType.Blacklist
						raycastParams.FilterDescendantsInstances = {game:GetService('Players').LocalPlayer.Character}
						local raycastResult = workspace:Raycast(cameraPosition, position - cameraPosition, raycastParams)

						local vector, onScreen = camera:WorldToScreenPoint(index2.Position)
						--if (rootpart.Position - v.Position).Magnitude <= 20 and onScreen then
						if raycastResult and (index2 == raycastResult.Instance) and (rootpart.Position - index2.Position).Magnitude <= 20 and (head.CFrame.p - camera.CFrame.p).Magnitude <= 30 and onScreen then

							if not ShowCooldown then
								print("In Screen And No Wall In-front")
								ShowCooldown = true
								local guiclone = board:Clone()
								guiclone.Parent = index2
								-- item is on screen and player is within 20 studs
							end
						else
							if ShowCooldown then
								print("Wall Blocking Or Not In Screen")
								if index2:FindFirstChild("InteractionGui") then
									ShowCooldown = false
									index2.InteractionGui:Destroy()
								end
							end
						end
						uis.InputBegan:Connect(function(input, IsTyping)
							if IsTyping then return end

							if input.KeyCode == Enum.KeyCode.E then
								--game:GetService("RunService").Stepped:Connect(function()	
								--local camCFrame = game:GetService("Workspace").CurrentCamera.CFrame.Position
								if index2:FindFirstChild("InteractionGui") then
									if index2:FindFirstChild("_Interactable") then
										if not PressCooldown then
											PressCooldown = true
											local vector, onScreen = camera:WorldToScreenPoint(index2.Position)
											--local myRay = Ray.new(rootpart.Position, v.Position/CFrame)

											if (rootpart.Position - index2.Position).Magnitude <= 20 and onScreen then -- camera:WorldToScreenPoint(v.Position)
												--print("You Pressed E")
												local guicloned = index2.InteractionGui
												local frame = guicloned.InteractionFrame
												local tweenInfo = TweenInfo.new(0.15, Enum.EasingStyle.Quart, Enum.EasingDirection.Out, 0, true, 0)
												wait()
												local sizetween = ts:Create(frame, tweenInfo, { Size = UDim2.new(0.10, 25,0.10, 25)})

												sizetween:Play()
												index2.Event:FireServer()
												wait(0.2)
												PressCooldown = false

												-- item is on screen and player is within 20 studs
											end
										end
									end
								end
								--end)
							end
						end)
					end
				end
          end
	    end
	
	end
end)
--------------------------------------------------------------
1 Like
#2

I like to create different scripts that manage the different types of things in my game. E.g. I’d have an InteractableManager script in ServerScriptService that is responsible for Interatable things, and different managers for all the other things.

I'd set up the InteractableManager like this: 
function isInteratable(instance)
    --Return true/false indicating that instance is interatable or not
    --I.e. check if a child called "_Interactable" exists. Although I'd change it to use CollectionService
    --    and check for a tag instead, but your way works too.
end

function setupInteractable(instance)
    local connection --If you want to do manual clean up, you might need to store this in a dictionary for later
    --Although connections automatically get disconnected and GC'ed when the corresponding instance is Destroyed
    connection = RunService.Stepped:Connect(function()
        --All the code inside the `if v:FindFirstChild("Event") then` clause
    end)
end

--Optionally implement this if you want to detect and do something when an interactable stops existing
function cleanupInteractable(instance)

end

game.Workspace.ChildAdded:Connect(function(instance)
    --Checking every child of workspace is probably overkill. Pretty sure CollectionService:GetInstanceAddedSignal("Interactable"):Connect
    --    would be way quicker.
    
    if isInteratable(instance) then
        setupInteractable(instance)
    end
end)

This already helps a lot with the insane indentation levels. Breaking your code out into smaller helper functions is a good way to make your code easier to understand and work with. A lot more can be done to make the code that would go inside the setup function better, but this is a start.

Hope this helps!

2 Likes
#3

del ete those comments
if your code works then this should be #help-and-feedback:code-review end

1 Like
#4

why not use proximity prompts???

#6

This code is going to be a massive problem in games. You are calling .Stepped which goes off of FPS. You are constantly creating a connection with the Humanoid.Died function which a player was to die their client would as well (actual game client).

You are looping through every instance in the workspace instead of a folder as well. This code is going to cause something known as a client desync when it’s used. What is a client desync? It’s when the client and the server become out of sync. It usually occurs due to extreme lag.

What this means essentially is that if you have another system (gun system). In a matter of minutes, the gun system will become progressively slower to shoot with (lag related). Eventually it will become so bad that although the player feels fine, everything in the game for said player will feel like it’s in slow motion.

You need to rewrite your code quite a bit, a start would be using a while wait() loop for per say 0.1 seconds and another start would be putting all of your interactable items into a folder.

2 Likes
#7

This post was flagged by the community and is temporarily hidden.

#8

comments dont improve performance they are ignored by roblox

#9

Make sure to use

local players = game:GetService("Players")
local Player = players.LocalPlayer or players.PlayerAdded:wait()

When requesting the local player object on the client.

And the use this to get the player’s character

local Character = player.Character or player.CharacterAdded:wait()

Rather than using wait(), as shown here in your code

local player = game.Players.LocalPlayer
local character = player.Character
wait()
local head = character:WaitForChild("Head")
local rootpart = character:WaitForChild("HumanoidRootPart")

Wait should be avoided. It’s good practice. You can read more here:

Why to avoid using wait()