How could I improve or clean this code?

Help and Feedback Code Review
, , ,
#1

Is there anyway i could improve this grabbing system?

Some feedback will be very much appreciated!

local Players = game:GetService("Players")
local Player = Players.LocalPlayer
repeat wait() until Player.Character
local Char = Player.Character
local Humanoid = Char:WaitForChild("Humanoid")
local HRP = Char:WaitForChild("HumanoidRootPart")
local PlayerGui = Player:WaitForChild("PlayerGui")
local GrabGui = PlayerGui:WaitForChild("GrabGui")
local UIS = game:GetService("UserInputService")
local RS = game:GetService("RunService")
local ReplicatedStorage = game:GetService("ReplicatedStorage")
local Mouse = Player:GetMouse() 
local Camera = workspace.CurrentCamera 

local SetNetworkOwnerEvent = ReplicatedStorage:WaitForChild("SetNetworkOwnerEvent")
local GrabEvent = ReplicatedStorage:WaitForChild("GrabEvent")

local Holding = false
local MaxGrabDistance = 10

local Object
local GrabbingForce
local GrabbingGyro

local RotatingR = false
local RotatingT = false

local RotateIncrement = math.rad(2)
local RotateCFrame = CFrame.Angles(0, 0, 0)

local ObjectDistance

local Animation = script:WaitForChild("HoldAnimation")
local AnimTrack = nil

function PlayAnimation(PlayAnim)
	if PlayAnim and not AnimTrack then
		AnimTrack = Humanoid:LoadAnimation(Animation)
		AnimTrack:Play()
	elseif AnimTrack and not PlayAnim then
		AnimTrack:Stop()
		AnimTrack = nil
	end
end

-- Functions
function Grab()
	print("Grabbing")
	local BodyPos = Instance.new("BodyPosition")
	local BodyGyro = Instance.new("BodyGyro")
	BodyPos.D = 650
	BodyGyro.D = 150
	BodyGyro.MaxTorque = Vector3.new(2000, 2000, 2000)
	Object = Mouse.Target
	local CanGrab = Object.CanGrab
	GrabEvent:FireServer(CanGrab, false)
	CanGrab.Value = false
	
	SetNetworkOwnerEvent:FireServer(Object, true)
	
	RotateCFrame = Object.CFrame
		
	GrabbingForce = BodyPos
	GrabbingGyro = BodyGyro
	
	GrabbingForce.Name = "GrabbingForce"
	GrabbingForce.Parent = Object

	GrabbingGyro.Name = "GrabbingGyro"
	GrabbingGyro.Parent = Object
	
	ObjectDistance = (Mouse.Target.Position - HRP.Position).Magnitude
	
	PlayAnimation(true)
	
	Holding = true
end

function Release()
	if Holding == true then
		print("Releasing")
		GrabbingForce:Destroy()
		GrabbingGyro:Destroy()
		SetNetworkOwnerEvent:FireServer(Object, false)
		GrabEvent:FireServer(Object:FindFirstChild("CanGrab"), true)
		local CanGrab = Object.CanGrab
		CanGrab.Value = true
		
		
		PlayAnimation(false)
				
		Holding = false
		
		Object = nil
	end
end

-- Start Grab
UIS.InputBegan:Connect(function(inputObject) 
    if (inputObject.UserInputType == Enum.UserInputType.MouseButton1 or inputObject.KeyCode == Enum.KeyCode.ButtonR2) and Mouse.Target and not Mouse.Target.Locked and Humanoid.Health > 0 then
		local magnitude = (Mouse.Target.Position - HRP.Position).Magnitude
		local CanGrabValue = Mouse.Target:FindFirstChild("CanGrab")
		if Mouse.Target and not Mouse.Target.Locked and magnitude < MaxGrabDistance and not Holding and CanGrabValue and CanGrabValue.Value == true then
			Grab()
		else
			Release()
		end
	end
end)

-- End Grab
UIS.InputEnded:Connect(function(inputObject)
    if inputObject.UserInputType == Enum.UserInputType.MouseButton1 or inputObject.KeyCode == Enum.KeyCode.ButtonR2 then
		Release()
	end
end)

RS.RenderStepped:Connect(function()
	
	-- Grab GUI
	if Mouse.Target and not Mouse.Target.Locked then
		local magnitude = (Mouse.Target.Position - HRP.Position).Magnitude
		if magnitude < MaxGrabDistance and not Holding then
			local CanGrabValue = Mouse.Target:FindFirstChild("CanGrab")
			if CanGrabValue and CanGrabValue.Value == true then
				GrabGui.GrabText.Visible = true
			else
				GrabGui.GrabText.Visible = false
			end
		else
			GrabGui.GrabText.Visible = false
		end
	else
		GrabGui.GrabText.Visible = false
	end
	-- Grabbing
	if Holding and Object and Object:IsA("BasePart") then
		local magnitude = (Object.Position - HRP.Position).Magnitude
		if Holding and Object and GrabbingForce and GrabbingGyro and magnitude < (MaxGrabDistance + 5) then
			GrabbingForce.Position = Camera.CFrame.Position + (Mouse.UnitRay.Direction * (ObjectDistance + (workspace.CurrentCamera.CoordinateFrame.p - HRP.Position + Vector3.new(0, 1, 0)).magnitude - 2))
			
			if RotatingR then
				local rotateCFrame2 = CFrame.Angles(RotateIncrement, 0, 0)
				RotateCFrame = GrabbingGyro.CFrame:ToWorldSpace(rotateCFrame2)
				GrabbingGyro.CFrame = RotateCFrame
				print("R")
			end
			if RotatingT then
				local rotateCFrame2 = CFrame.Angles(0, RotateIncrement, 0)
				RotateCFrame = GrabbingGyro.CFrame:ToWorldSpace(rotateCFrame2)
				GrabbingGyro.CFrame = RotateCFrame
				print("T")
			end
			
			
		else
			Release()
		end
		-- Anti fly
		if Object then
			Object.Touched:Connect(function(hit)
				if hit.Parent and hit.Parent == Char then
					Release()
				end
			end)
		end
	end
	
	-- Item price tag
	if Mouse.Target then
		local PurchasableItem = Mouse.Target:FindFirstChild("PurchasableItem")
		local magnitude = (Mouse.Target.Position - HRP.Position).Magnitude
		if not Holding and PurchasableItem and magnitude <= MaxGrabDistance then
			local Gui = Mouse.Target:FindFirstChild("ItemInfoGui")
			if Gui then
				Gui.Enabled = true
				repeat
					wait()
				until not Mouse.Target or (Mouse.Target and not Mouse.Target:FindFirstChild("PurchasableItem")) or Holding
				Gui.Enabled = false
			end
		end
	end
end)

--== Rotate/Tilt Object ==--

-- Start Rotate/Tilt
UIS.InputBegan:Connect(function(inpObj, gp)
	if inpObj.KeyCode == Enum.KeyCode.R then -- Rotate
		RotatingR = true
	end
	if inpObj.KeyCode == Enum.KeyCode.T then -- Tilt
		RotatingT = true
	end
end)

-- Stop Rotate/Tilt
UIS.InputEnded:Connect(function(inpObj, gp)
	if inpObj.KeyCode == Enum.KeyCode.R then -- Rotate
		RotatingR = false
	end
	if inpObj.KeyCode == Enum.KeyCode.T then -- Tilt
		RotatingT = false
	end
end)

--== Make sure that theres no floating object when the player leaves or dies ==--
Players.PlayerRemoving:Connect(function(player)
	if Object and Object.Parent and player == Player then
		Release()
	end
end)

Humanoid.Died:Connect(function()
	if Object and Object.Parent then
		Release()
	end
end)
4 Likes
#2

You could put the variables in a module script just like how you put them in a table to make the script look nicer, but it’s already better than mine. Mine are super messy.

3 Likes
#3

Or why don’t you just make a click detector and when it’s clicked, the animation plays and the item is cloned into the player’s backpack, the item on the floor gets destroyed.
Put the click detector inside the part.

1 Like
#4

if you look at the script. it has nothing to do with tools. Its just an object grabber. Not a tool grabber.

#6

Having click detectors is not really an efficient way to do this. Also the object has to have a bool value called “CanGrab” to grab the object.

#8

I don’t know if this code can be shorter or not, but I do have a suggestion: Do only one InputBegan and one InputEnded for the Rotate/Tilt stuff.

UIS.InputBegan:Connect(function(inpObj, gp)
	if inpObj.KeyCode == Enum.KeyCode.R then -- Rotate
		RotatingR = true
    end
    if inpObj.KeyCode == Enum.KeyCode.T then -- Tilt
		RotatingT = true
	end
end)

UIS.InputEnded:Connect(function(inpObj, gp)
	if inpObj.KeyCode == Enum.KeyCode.R then -- Rotate
		RotatingR = false
    end
    if inpObj.KeyCode == Enum.KeyCode.T then -- Tilt
		RotatingT = false
	end
end)
2 Likes
#9

I dont think your getting what the grabbing system is for. Its not for tools, or anything to do with the player’s inventory or backpack. It has to do with dragging objects. Think of it as Lumber Tycoon 2

1 Like
#10

The reason i didn’t do that, is so the player can hold both R and T to rotate and tilt an object at the same time.

#11

Oh you meant something like Lumber Tycoon 2, I thought it was picking up items, as it looks like it.

1 Like
#12

This should do what you want. You can hold both R and T.

#13

That wouldn’t work either, as you can only detect 1 key when ever you use InputBegan. Since there is only one InputObject value in the function.

1 Like
#14

If you press 2 keys that event is supposed to fire twice so…

1 Like
#15

oh wait. True, ill try it out.

#16

huh, your right. Thanks!

(30 chars)

#17

You could only have to reparent body forces rather than recreating them.

You can also try using one function for what @FlashFlame_Roblox said.

There is not much to be improved otherwise but guard clauses can definitely make the code a lot shorter and readable.

EDIT: Below reply as well you should listen to

1 Like
#18

Use a event-based approach:

-- Use
local char = player.Character or player.CharacterAdded:Wait()

-- Instead of
repeat wait() until player.Character
1 Like
#19

  1. Don’t use Instance:WaitForChild() unless it’s necessary. In the beginning of your code, you seem to use this computationally expensive function in places it’s not needed, such as script:WaitForChild("HoldAnimation"). Unless you’re creating that animation in another script, script.HoldAnimation should suffice.

  2. As mentioned above, don’t use repeat wait() until Player.Character. Also, have you considered simply putting this script in StarterCharacterScripts? That would eliminate the need for this wait altogether. Also, you could replace your Players.PlayerRemoving and Humanoid.Died listeners with a single Character.AncestryChanged listener, and check if Character:IsDescendantOf(game) to determine whether the player left or if the character died.

  3. Don’t use Instance:FindFirstChild() unless necessary. In your Release function, you use Object:FindFirstChild("CanGrab"), directly followed by local CanGrab = Object.CanGrab. Take a look at this thread to help you address issues 1 and 3. First of all, do you need to use FindFirstChild there? Is CanGrab always going to be inside the Object, or is it possible that it isn’t? If it’s always there, you’re doing something inefficient for no reason. If it’s not always there, make sure you account for the fact that if the client doesn’t see it, nil will be sent to the GrabEvent remote. Plus, the next line will throw an error because Object.CanGrab does not exist.

  4. What does your server code for handling the SetNetworkOwnerEvent RemoteEvent look like? Are you making sure that the object they’re passing in is in some sort of whitelist of “grabbable” objects? If not, this is a huge security vulnerability, letting the client claim network ownership of any part it wants.

  5. Condense your multiple UIS.InputBegan/UIS.InputEnded listeners into one single listener, as mentioned above.

  6. Simplify your RenderStepped code. Keep in mind that everything in that listener will run every single frame, so it’s of utmost importance to keep it as simple as possible. For starters, I would simplify those conditional statements and remove redundancies. An example would be in the first statement, you perform a relatively expensive distance check between the Mouse.Target and the Character’s HumanoidRootPart every single frame, regardless of whether or not they’re even holding anything. Before any of that code, check if not Holding then because it’s a lot faster than checking the distance between parts that frequently. There’s many other places in that chunk of code that can be simplified, so definitely think through it and figure out what you can simplify.

  7. Keep in mind that by rotating the object by a constant angle every frame, you’re making the rotational speed of the object dependent on frame rate. Someone who only gets 20 fps in your game will see parts spinning 1/3 as fast as someone getting 60 fps. RunService.RenderStepped is called with a parameter that represents the amount of time between consecutive calls. Take a look at the RunService documentation as well as the documentation for RenderStepped (linked on that same page) to see an example of this.

3 Likes
#20

The reason why i use :WaitForChild() for most variables, is because its reliable. If I just do script.WhatEver. Sometimes it doesn’t work, and i end up getting an error saying; WhatEver is not a valid memeber of script. Even though it is. Well, at least before 2018 it was an issue, i dont know if the bug has been fixed yet.

Here’s an example of what im talking about: "Not a valid member" Situation

So until I can rely on variables without WaitForChild, i’ll be using the WaitForChild() function.

Aside from that; thanks for your feedback!

1 Like
#21

Never use RenderStepped if you aren’t updating something that needs to be changed before a frame renders (something that may affect the viewport, such as the Camera CFrame). Use Heartbeat instead (if it needs to run before physics simulate, Stepped). This is also taking into account that you have a lot of actions running in RunService as well which can slow down the render tick rate and thus when other tasks in a frame are performed (if ever, assuming the frame isn’t dropped).

4 Likes
#22

Huh, that’s interesting. Thanks for the tip!