Within proximity of another player and key press Code Review

What my code is suppose to do? When a player is close to another player and press the key r something happens

My current code works to a certain extent but not as I would like it to. The current code if I press the key r 3x in one second it only shows 1 time that I have pressed the R key

I do believe that the problem is the way I am checking if the r key is pressed

local players = game:GetService("Players")
local RunService = game:GetService("RunService")
local uiService = game:GetService("UserInputService")
local player = players.LocalPlayer
local radius = 10

local ePressed = false

game:GetService("UserInputService").InputBegan:connect(function(inputObject, gameProcessedEvent)
	if inputObject.KeyCode == Enum.KeyCode.R then
		ePressed = true
		wait(1)
		ePressed = false
	end
end)

local debounce = true
uiService.InputBegan:Connect(function(key)
	for i,v in pairs (players:GetChildren()) do
		if v.Name ~= player.Name and debounce == true then
			if (player.Character.HumanoidRootPart.Position-v.Character.HumanoidRootPart.Position).magnitude < radius and ePressed == true then
				--Player is within radius
				--Display billboard gui on player root part
				debounce = false
				print("Within radius!")

				debounce = true	
				
			end
		end
	end
end)


Probably should add a debounce on whenever the key “R” is pressed. Also, you might want to put the debounce for the second event before you start the cycle.

Doesn’t make a difference provided no yielding methods have been called. Even if the event is triggered again it doesn’t get processed until something else yields.

Processing isn’t actually parallel here, it just quickly swaps to do the next job in the queue whenever the thread yields.

GetChildren and Vector3 methods don’t yield.

2 Likes

My advice would be to do a single InputBegan connection. There’s no reason to have them split.

Also if you’ve defined uiService then there’s no need to do GetService again for that first connection.

Minor stuff that won’t make much difference to performance at the moment but in terms of others understanding the code and maintainability it makes sense to only have 1 connection and to use your variables.

Edit: one extra minor thing. If you swap your conditions around in the second connection you can save processing. If the first term of an and condition is false, the rest don’t get evaluated, so put your ePressed comparison first, and then the magnitude check will only happen if ePressed is the right state. Even better, put the ePressed and debounce checks before you even open the for loop and you can save yourself the need to run any of it if either of them are false.

2 Likes

Ah, I was just about to say that. :sweat_smile:

I think the solution is to do a single InputBegan (as BanTech suggested) and remove the debounce. So, something like this:

uiService.InputBegan:connect(function(inputObject, gameProcessedEvent)
	if inputObject.KeyCode ~= Enum.KeyCode.R then return end
	--Put the block of code that checks which players are close.
end)

------ Since the problem is that repeated presses are sunk, the cause of it would be the debounce. Removing it will solve this problem. *However,* I do not suggest that you remove the debounce as you will not want multiple threads running in parallel doing the same task -- interfering with each other.

Additionally, I want to point out, you have a wait here:

if inputObject.KeyCode == Enum.KeyCode.R then
	ePressed = true
	wait(1)
	ePressed = false
end

I think this is also part of the problem since you have no debounce on this event, multiple threads are running, switching this true and false at random. Thus, causing some cases where there should’ve of been print being sunk.

1 Like

Thank you for giving me feed back @utrain & @BanTech. I took it into consideration. Here is the updated version

local players = game:GetService("Players")
local RunService = game:GetService("RunService")
local uiService = game:GetService("UserInputService")
local player = players.LocalPlayer
local my_Player_char = player.Character
local radius = 10

local withinPlayerDistance = false

game:GetService("UserInputService").InputBegan:connect(function(inputObject, gameProcessedEvent)
	if inputObject.KeyCode == Enum.KeyCode.R and withinPlayerDistance == true then
		print("R have been clicked and the player was with in distance of another player")
		withinPlayerDistance = true
		wait(1)
		withinPlayerDistance = false
	end
end)

local debounce = true
local showingBbGUI = false
uiService.InputBegan:Connect(function(key)
	local getPlayers = players:GetPlayers()
	for i,v in pairs(getPlayers) do
		if v.Name ~= player.Name then
			if (my_Player_char.HumanoidRootPart.Positionv.Character.HumanoidRootPart.Position).magnitude < radius then
				withinPlayerDistance = true
				--Within Distance
				if showingBbGUI == false then
					showingBbGUI = true
					local billBoardGui = player.PlayerGui.BbGuiPressE
					billBoardGui.Adornee = v.Character.HumanoidRootPart
					billBoardGui.Enabled = true
					print("Billboard Gui is set")
				end

			else
				withinPlayerDistance = false
				showingBbGUI = false
				
				local billBoardGui = player.PlayerGui.BbGuiPressE
				billBoardGui.Adornee = nil
				billBoardGui.Enabled = false
				--Not within Distance anymore/or is not in distance of a player
			end
		end
	end
end)

Hey there! I’d like to bring to attention what are, in my eyes, a few pitfalls of this revision.

  1. I highly advise you keep your codebase modular. Very modular. Should you make a function that does a simple assignment? Not really, but don’t be afraid of using them as aliases to rename a certain function.

  2. Don’t worry about prefixing variables with “my”, if you wanted to, prefix every script with your copyright license! (Joking aside, it’s redundant. It’s a great habit to get yourselves out of)

  3. Keep your code consistent, adopt a style guide. This will really iron out a lot of issues in the long run. (Keep services either PascalCase I advise doing this! or camelCase; pick one. Keep function and variable names snake_case, camelCase I advise this too!, or PascalCase; pick one).

I made a rendition of your snippet given to demonstrate these practices. Realistically, you’d probably make utility modules to aid in these functions across your entire game. When possible and within reason, keep your code DRY—Don’t Repeat Yourself.

local Players = game:GetService("Players")
local RunService = game:GetService("RunService")
local UserInputService = game:GetService("UserInputService")

--[[
	Controls whether or not this LocalScript should emit debugging messages.
	@type {boolean}
]]
local DEBUG = true

--[[
	The radius (in studs) to be observed by this LocalScript from the LocalPlayer.
	@readonly
	@type {integer}
]]
local RADIUS = 10

--[[
	The gui element to be expressed when a player enters the given radius.
	@readonly (this variable alone should never be in the left side of an assignment statement)
	(This value's properties are still mutable, so we shouldn't use LOUD_SNAKE_CASE.
	@type {BillboardGui}
]]
local eGuiElement = Players.LocalPlayer.PlayerGui.PressEGui

--[[
	A hoisted variable that stores another player that is currently closest and selected to the local player.
	Once selected, this value will not change until said player leave's the local player's radius.
	@type {Player?}
]]
local selectedPlayer

--[[
	Retrieves every player except for the local player.
	@returns {Player[]}
]]
local function getOtherPlayers()
	local players = Players:GetPlayers()
	local otherPlayers = {}
	for _, player in ipairs(players) do
		if player.Name ~= Players.LocalPlayer.Name then
			table.insert(otherPlayers, player)
		end
	end
	return otherPlayers
end

--[[
	Calculates the distance between two player's models.
	@param {Player} playerOne
	@param {Player} playerTwo
	@returns {number}
]]
local function calculateDistanceBetweenPlayers(playerOne, playerTwo)
	local playerOnePosition = playerOne.Character.HumanoidRootPart.Position
	local playerTwoPosition = playerTwo.Character.HumanoidRootPart.Position
	return (playerOnePosition - playerTwoPosition).Magnitude
end

--[[
	Retrieves every player whose character is within a radius.
	@returns {Player[]}
]]
local function getOtherPlayersInRadius()
	local otherPlayers = getOtherPlayers()
	local otherPlayersInRadius = {}
	for _, otherPlayer in ipairs(otherPlayers) do
		local distance = calculateDistanceBetweenPlayers(Players.LocalPlayer, otherPlayer)
		if distance < RADIUS then
			table.insert(otherPlayersInRadius, otherPlayer)
		end
	end
	return otherPlayersInRadius
end

--[[
	A function that returns the closest player within a radius.
	If no player is close, returns nil.
	@returns {Player?}
]]
local function getClosestPlayerInRadius()
	local otherPlayersInRadius = getOtherPlayersInRadius()
	
	local closestDistance = math.huge
	local closestPlayer
	
	for _, player in ipairs(otherPlayersInRadius) do
		local distance = calculateDistanceBetweenPlayers(Players.LocalPlayer, player)
		if distance < closestDistance then
			closestDistance = distance
			closestPlayer = player
		end
	end
	
	return closestPlayer
end

--[[
	Returns if the gui elements are currently enabled.
	(An alias for an expression whose purpose isn't immediately evident)
	@returns {boolean}
]]
local function isEGuiElementEnabled()
	return eGuiElement.Adornee ~= nil
end

--[[
	Enables the local player's "PressEGui" and attaches it to the target player's character.
	@param {Player} targetPlayer
	@returns {nil}
]]
local function enableEGuiElement(targetPlayer)
	eGuiElement.Adornee = targetPlayer.Character.HumanoidRootPart
	eGuiElement.Enabled = true
	if DEBUG then
		local message = ("Billboard GUI element set to %q"):format(targetPlayer.Name)
		print(message)
	end
end

--[[
	Unattaches the local player's "PressEGui" and disables it.
	@returns {nil}
]]
local function disableEGuiElement()
	eGuiElement.Adornee = nil
	eGuiElement.Enabled = false
end

--[[
	Sets the selected player to targetPlayer and updates the coresponding GUI elements.
	If targetPlayer is nil, disables and unattaches the corresponding GUI elements.
	@param {Player?} targetPlayer
	@returns {nil}
]]
local function setSelectedPlayer(targetPlayer)
	if targetPlayer then
		if not isEGuiElementEnabled() then
			enableEGuiElement(targetPlayer)
		end
	elseif isEGuiElementEnabled() then
		disableEGuiElement()
	end
	selectedPlayer = targetPlayer
end

--[[
	Updates the selected player. Does no actions if the currently selected player is still valid.
	@returns {nil}
]]
local function updateSelectedPlayer()
	-- Check the selected player is still valid
	if selectedPlayer then
		local distance = calculateDistanceBetweenPlayers(Players.LocalPlayer, selectedPlayer)
		local isValid = distance <= RADIUS
		if not isValid then
			-- Set selectedPlayer to nil directly as opposed to using setSelectedPlayer
			-- to force the function to "fall through" to the next "case", similar to a
			-- switch statement.
			selectedPlayer = nil;
		end
	end
	if not selectedPlayer then
		local closestPlayerInRadius = getClosestPlayerInRadius()
		setSelectedPlayer(closestPlayerInRadius)
	end
end

--[[
	Event listener for when the local player's "R" key is pressed.
	@returns {nil}
]]
local function onRKeyPressed()
	if selectedPlayer then
		if DEBUG then
			local message = ("R has been clicked while another player, %q, was within the radius."):format(selectedPlayer.Name)
			print(message)
		end
	end
end

--[[
	Event listener for when the local player supples input.
	@param {InputObject} input
	@returns {nil}
]]
local function onInputBeganListener(input)
	if input.KeyCode == Enum.KeyCode.R then
		updateSelectedPlayer()
		onRKeyPressed(selectedPlayer)
	end
end

UserInputService.InputBegan:Connect(onInputBeganListener)

I recommend copying and pasting this into your IDE of choice.

If you have any questions about the rationale behind something, ask! This isn’t the best style, since style is subjective, it just shows how consistency and modularity can bring clarity to your code.

1 Like

I have just tested your code and I believe there are some missing parameters (line 68)

	for _, otherPlayer in ipairs(otherPlayers) do
		if calculateDistanceBetweenPlayers(Players.LocalPlayer, otherPlayer) < RADIUS then --Tryed to fix it by passing thorough Players.LocalPlayer, otherPlayer
			table.insert(otherPlayersInRadius, otherPlayer)
		end
	end

And line 108

--Where you are calling this function you are not passing a parameter and it seem to be a number which I don't quite understand yet
local function enableEGuiElement(targetPlayer)
	eGuiElement.Adornee = targetPlayer.Character.HumanoidRootPart
	eGuiElement.Enabled = true
	if DEBUG then
		local message = ("Billboard GUI element set to %q"):format(targetPlayer.Name)
		print(message)
	end
end

Other then that I do appreciate your replay this is truly mind opening on different ways of scripting

You’re absolutely correct as to my type mismatch. The revised code for that function is as follows (I’ll edit my last post to reflect this as well):

--[[
	Retrieves every player whose character is within a radius.
	@returns {Player[]}
]]
local function getOtherPlayersInRadius()
	local otherPlayers = getOtherPlayers()
	local otherPlayersInRadius = {}
	for _, otherPlayer in ipairs(otherPlayers) do
		-- Previously forgot to supply both player parameters
		local distance = calculateDistanceBetweenPlayers(Players.LocalPlayer, otherPlayer)
		if distance < RADIUS then
			table.insert(otherPlayersInRadius, otherPlayer)
		end
	end
	return otherPlayersInRadius
end

And the other fix:

--[[
	Sets the selected player to targetPlayer and updates the coresponding GUI elements.
	If targetPlayer is nil, disables and unattaches the corresponding GUI elements.
	@param {Player?} targetPlayer
	@returns {nil}
]]
local function setSelectedPlayer(targetPlayer)
	if targetPlayer then
		if not isEGuiElementEnabled() then
			-- Previously forgot to supply the 'targetPlayer' parameter.
			enableEGuiElement(targetPlayer)
		end
	elseif isEGuiElementEnabled() then
		disableEGuiElement()
	end
	selectedPlayer = targetPlayer
end
1 Like

I previously have entered those parameter but that did not fix this problem:
image
I can’t understand why it is a number because when I try to print the parameter targetPlayer it prints a number
image
This error is coming from the function enableEGuiElement() --line 109

local function enableEGuiElement(targetPlayer)
	print(targetPlayer)
	eGuiElement.Adornee = targetPlayer.Character.HumanoidRootPart
	eGuiElement.Enabled = true
	if DEBUG then
		local message = ("Billboard GUI element set to %q"):format(targetPlayer.Name)
		print(message)
	end
end

Yikes! This is why I’m much more comfortable writing code I can write a spec for. I believe I found the issue, though:

getClosestPlayerInRadius didn’t send the closest player, it sent the closest distance, thus the rogue number.

I rebuilt getClosestPlayerInRadius to be much more sensible:

--[[
	A function that returns the closest player within a radius.
	If no player is close, returns nil.
	@returns {Player?}
]]
local function getClosestPlayerInRadius()
	local otherPlayersInRadius = getOtherPlayersInRadius()
	
	local closestDistance = math.huge
	local closestPlayer
	
	for _, player in ipairs(otherPlayersInRadius) do
		local distance = calculateDistanceBetweenPlayers(Players.LocalPlayer, player)
		if distance < closestDistance then
			closestDistance = distance
			closestPlayer = player
		end
	end
	
	return closestPlayer
end
1 Like

I am truly enjoying reading the code the way that you coded it

But I have this question…if you wanted the gui to be visible upon approach to the nearest player not upon pressing the r key how would you have gona to write it? I am assuming you still would have used a function that loops like so as I did in my code

local uiService = game:GetService("UserInputService")
uiService.InputBegan:Connect(function()
	
end)

And then upon press of a key and near a player then some action happens

I would bind the gui update functions to RunService.RenderStepped and the R key logic to UserInputService.InputBegan.

1 Like