Is there a more efficient way of handling inputs?

[MorbidlyObeseMonke (ObeseMonke)]

Hello Devs,
I’ve recently created a working robot arm and was wondering if there is a more efficient way of writing my code. I feel like there is a much better way of doing this and I’m not quite sure how. I’m specifically looking for a better way to handle the inputs from the player.

Robot arm:

Local script

local UserInputService = game:GetService("UserInputService")
local remote = game.ReplicatedStorage:WaitForChild("RemoteEvent")

local player = game:GetService("Players").LocalPlayer

player.CharacterAdded:Connect(function(character)
	remote:FireServer("Startup")
end)

UserInputService.InputBegan:Connect(function(input, typing)
	if input.KeyCode == Enum.KeyCode.E and not typing then
		remote:FireServer("Right")
	end
	if input.KeyCode == Enum.KeyCode.Q and not typing then
		remote:FireServer("Left")
	end
	if input.KeyCode == Enum.KeyCode.R and not typing then
		remote:FireServer("Up")
	end
	if input.KeyCode == Enum.KeyCode.F and not typing then
		remote:FireServer("Down")
	end
	if input.KeyCode == Enum.KeyCode.Z and not typing then
		remote:FireServer("Forward")
	end
	if input.KeyCode == Enum.KeyCode.C and not typing then
		remote:FireServer("Backward")
	end
	if input.KeyCode == Enum.KeyCode.T and not typing then
		remote:FireServer("PickUp")
	end
end)

Server Script

local remote = game.ReplicatedStorage.RemoteEvent

remote.OnServerEvent:Connect(function(plr, message, action)
	if message == "Startup" then
		local Character = plr.Character
		local Arm = game.Workspace.Arm:Clone()
		Arm.Parent = Character.Torso

		Character.HumanoidRootPart.Anchored = true

		wait(0.1)

		Arm.PrimaryPart.CFrame = Character.Torso.CFrame + Vector3.new(0, 0, 1)
		Arm.PointTo.CFrame = Arm.PrimaryPart.CFrame + Vector3.new(0, 5, 0)
		Arm.PrimaryPart.WeldConstraint.Part1 = Character.Torso

		local weld = Instance.new("WeldConstraint")
		weld.Parent = Arm.PrimaryPart
		weld.Part0 = Arm.PrimaryPart
		weld.Part1 = Arm.PointTo

		Arm.PointTo.Anchored = false

		wait(3)

		Character.HumanoidRootPart.Anchored = false
	end

	local Character = plr.Character
	local Arm = Character.Torso.Arm

	if message == "Right" then
		local lookvec = Arm.PointTo.CFrame.RightVector * 1
		Arm.PointTo.Position = Arm.PointTo.Position + lookvec
	end
	if message == "Left" then
		local lookvec = Arm.PointTo.CFrame.RightVector * -1
		Arm.PointTo.Position = Arm.PointTo.Position + lookvec
	end
	if message == "Up" then
		local lookvec = Arm.PointTo.CFrame.UpVector * 1
		Arm.PointTo.Position = Arm.PointTo.Position + lookvec
	end
	if message == "Down" then
		local lookvec = Arm.PointTo.CFrame.UpVector * -1
		Arm.PointTo.Position = Arm.PointTo.Position + lookvec
	end
	if message == "Forward" then
		local lookvec = Arm.PointTo.CFrame.LookVector * 1
		Arm.PointTo.Position = Arm.PointTo.Position + lookvec
	end
	if message == "Backward" then
		local lookvec = Arm.PointTo.CFrame.LookVector * -1
		Arm.PointTo.Position = Arm.PointTo.Position + lookvec
	end
end)

Please could someone help me, or at least point me in the right direction.

[JarodOfOrbiter]

@MorbidlyObeseMonke

if there is a more efficient way of writing my code

Be careful with that word. People throw it around but they rarely use it for what it means. Micro-optimized code is ugly and unreadable. Don’t go for peak efficiency. If you want your code cleaned up though, There are some definite considerations.
You’re checking in every single if statement not typing, repetitive code can almost always be consolidated. This makes maintanance much quicker and safer down the line and will save some lines.
Next is to use elseif. KeyCode can only be one thing, so once you’ve found the successful if statement you don’t need to keep checking everything else. This will clean up the code and make it more efficient, but efficiency of this sort doesn’t matter when the code is faster than the human can button mash.

UserInputService.InputBegan:Connect(function(input, typing)
	if not typing then
		if input.KeyCode == Enum.KeyCode.E then
			remote:FireServer("Right")
		elseif input.KeyCode == Enum.KeyCode.Q then
			remote:FireServer("Left")
		elseif input.KeyCode == Enum.KeyCode.R then
			remote:FireServer("Up")
		elseif input.KeyCode == Enum.KeyCode.F then
			remote:FireServer("Down")
		elseif input.KeyCode == Enum.KeyCode.Z then
			remote:FireServer("Forward")
		elseif input.KeyCode == Enum.KeyCode.C then
			remote:FireServer("Backward")
		elseif input.KeyCode == Enum.KeyCode.T then
			remote:FireServer("PickUp")
		end
	end
end)

You could also make it a little nicer with a keybinds table.

local keybinds = {
	[Enum.KeyCode.E] = "Right",
	[Enum.KeyCode.Q] = "Left",
	[Enum.KeyCode.R] = "Up",
	[Enum.KeyCode.F] = "Down",
	[Enum.KeyCode.Z] = "Forward",
	[Enum.KeyCode.C] = "Backward",
	[Enum.KeyCode.T] = "PickUp",
}

UserInputService.InputBegan:Connect(function(input, typing)
	if not typing then
		local movement = keybinds[input.KeyCode]
		if movement then
			remote:FireServer(movement)
		end
	end
end)

But again this isn’t about efficiency and it shouldn’t be. Don’t worry about efficiency when you’re dealing with user input.

Do the same with the other script. Use elseif. You’re declaring the character in two places when you only need to declare it once.

@MorbidlyObeseMonke

Arm.PointTo.Position = Arm.PointTo.Position + lookvec

This line is the same every time. Simplify it.

local remote = game.ReplicatedStorage.RemoteEvent

remote.OnServerEvent:Connect(function(plr, message, action)
	local Character = plr.Character
	if message == "Startup" then
		local Character = plr.Character
		local Arm = game.Workspace.Arm:Clone()
		Arm.Parent = Character.Torso

		Character.HumanoidRootPart.Anchored = true

		wait(0.1)

		Arm.PrimaryPart.CFrame = Character.Torso.CFrame + Vector3.new(0, 0, 1)
		Arm.PointTo.CFrame = Arm.PrimaryPart.CFrame + Vector3.new(0, 5, 0)
		Arm.PrimaryPart.WeldConstraint.Part1 = Character.Torso

		local weld = Instance.new("WeldConstraint")
		weld.Parent = Arm.PrimaryPart
		weld.Part0 = Arm.PrimaryPart
		weld.Part1 = Arm.PointTo

		Arm.PointTo.Anchored = false

		wait(3)

		Character.HumanoidRootPart.Anchored = false
	else
		local Arm = Character.Torso.Arm
		local lookvec
		if message == "Right" then
			lookvec = Arm.PointTo.CFrame.RightVector * 1
		elseif message == "Left" then
			lookvec = Arm.PointTo.CFrame.RightVector * -1
		elseif message == "Up" then
			lookvec = Arm.PointTo.CFrame.UpVector * 1
		elseif message == "Down" then
			lookvec = Arm.PointTo.CFrame.UpVector * -1
		elseif message == "Forward" then
			lookvec = Arm.PointTo.CFrame.LookVector * 1
		elseif message == "Backward" then
			lookvec = Arm.PointTo.CFrame.LookVector * -1
		else return end
		Arm.PointTo.Position = Arm.PointTo.Position + lookvec
end)

[MorbidlyObeseMonke (ObeseMonke)]

Thank you for your response, I really appreciate you helping me out.

[okeanskiy]

Unrelated to your question, but:

The Startup and the Movement Actions should be their own separate RemoteEvents.

Then, the Startup RemoteEvent should be removed, because it’s only called on CharacterAdded, which the server already has accessed to. Putting a Remote as a middleman only increases the total time it takes to load your arm.

You don’t need to anchor the root part or add waiting during creation of the arm, so if that feels like an inefficiency, then remove it.

Then, and most importantly, a general rule of thumb is that anything the player moves should either be client-authoritative or at the very least client-predictive. You can make this client authoritative by giving NetworkOwnership of the Arm.PrimaryPart to the player, and then moving the movement code to the Client. Now there is no delay between input and movement, and you’re only remote calls should be for the “PickUp” action.

Once you’ve done that you’ll probably want to move the arm not based on how many times input began, but how long input was held. You can use ContextActionService and set keys_pressed[input.KeyCode] = true or nil on InputState.Began and InputState.End. Then, every RenderStep, move the robot arm in the direction your keys_pressed indicate and multiply by (DeltaTime * movement_speed), instead of 1.

For binding/unbind actions you can either use a mixture of Torso.ChildAdded (since you part the arm to the Torso, which will only work on R6 rigs, I should mention) to bind / CharacterRemoving to unbind. Or better yet make the Arm Model a CollectionService Tag and bind on InstanceAdded / unbind on InstanceRemoved. Be sure to add an attribute though (player name or userid) so clients know what Arm is theirs.

[MorbidlyObeseMonke (ObeseMonke)]

Thank you for your suggestions. I will definitely work to add some of the things you suggested.

[BullfrogBait]

Very small but:

Arm.PointTo.Position = Arm.PointTo.Position + lookvec can possibly be shortened into Arm.PointTo.Position += lookvec