Is this a safe way of handling client-side collectables

Firstly, sorry for how long this is and secondly, part of my feels this should be in code review.

Each player needs a set of keys which they need to collect, and their currently collected amount is tracked as a leaderstat.

The collectables need to be client sided, that way:
Say player1 collects the first key, it disappears for them on their screen, but not on player2’s screen.
That way, player2 is still able to collect the key.

The idea behind my code:

  • Store all the keys positions in a table
  • When a player joins, fire a remote even to the player which renders the keys for them on their screen.
  • When we do so, I connect a “pick up key” remote event to the key’s touched event.
  • When the server receives this event, we do some validation to make sure that the player requesting to pick up the key is actually somewhat near the key and hasn’t collected it already (debounce)
  • From there, I can increment the collected amount and tell the client to delete the key on their screen.

My main concern is that this setup with the remote event may allow players to constantly fire to the server saying that they’ve picked up a key
So is my validation steps enough to prevent any malicious use?

Here is the hierarchy and code.
image

Key Handler (server)

-- Services
local ReplicatedStorage = game:GetService("ReplicatedStorage")
local PlayerService = game:GetService("Players")

-- Remote Events
local RenderKeyEvent = ReplicatedStorage.RenderKey
local DestroyKeyEvent = ReplicatedStorage.DestroyKey
local PickupKeyEvent = ReplicatedStorage.PickupKey

-- Data
local keys = {
	key1 = {position = Vector3.new(30,4,0), name = "key1"},
	key2 = {position = Vector3.new(30,4,30), name = "key2"},
	key3 = {position = Vector3.new(-30,4,0), name = "key3"},
	key4 = {position = Vector3.new(30,4,-30), name = "key4"},
	key5 = {position = Vector3.new(-30,4,30), name = "key5"},
	key6 = {position = Vector3.new(0,4,30), name = "key6"},
	key7 = {position = Vector3.new(0, 4,-30), name = "key7"}
}

-- Functions
function playerJoined(player)
	-- Create leader stats
	local leaderstats = Instance.new("Folder", player)
	leaderstats.Name = "leaderstats"
	local t = Instance.new("IntValue", leaderstats)
	t.Name = "Keys"
	
	-- This is used to tag which keys have been collected
	local collectedKeys = Instance.new("Folder", player)
	collectedKeys.Name = "collectedKeys"
	
	-- Tell the player to render each key
	for _, keyTable in pairs(keys) do
		RenderKeyEvent:FireClient(player, keyTable)
	end
end

function clientWishesToPickupKey(player: Player, keyName: string) -- is ran by a remote event fired to the server from the client
	-- Some validation
	local character = player.Character
	if not character then warn(script.Name..": "..player.Name.." did not have a character!") return end
	local root = character:FindFirstChild("HumanoidRootPart")
	if not root then warn(script.Name..": "..player.Name.." did not have a root part!") return end
	
	local keyTable = keys[keyName]
	local keyPosition = keyTable.position
	
	local playerPosition = root.Position
	
	-- checks the player is close enough to the key AND that they've not already collected that key
	if (keyPosition - playerPosition).Magnitude < 5 and not player.collectedKeys:FindFirstChild(keyName) then
		local tag = Instance.new("StringValue", player.collectedKeys)
		tag.Name = keyName
		
		player.leaderstats.Keys.Value += 1
		
		DestroyKeyEvent:FireClient(player, keyName)
	end
end

-- Start

-- Events
PlayerService.PlayerAdded:Connect(playerJoined)
PickupKeyEvent.OnServerEvent:Connect(clientWishesToPickupKey)

Client Key Renderer

-- Services
local ReplicatedStorage = game:GetService("ReplicatedStorage")

-- Remote Events
local RenderKeyEvent = ReplicatedStorage:WaitForChild("RenderKey")
local DestroyKeyEvent = ReplicatedStorage:WaitForChild("DestroyKey")
local PickupKeyEvent = ReplicatedStorage:WaitForChild("PickupKey")

-- Data
local keyTemplate = ReplicatedStorage:WaitForChild("Key")

-- Functions
function render(keyTable)
	local newKey = keyTemplate:Clone()
	newKey.Position = keyTable.position
	newKey.Name = keyTable.name
	newKey.Parent = workspace
	
	newKey.Touched:Connect(function()
		PickupKeyEvent:FireServer(newKey.Name)
	end)
end

function destroy(keyName)
	local key = workspace:FindFirstChild(keyName)	
	if not key then return end
	key:Destroy()
end

-- Events
RenderKeyEvent.OnClientEvent:Connect(render)
DestroyKeyEvent.OnClientEvent:Connect(destroy)

Your collectables do not need to be client-sided altogether. Instead, try doing something like this

--server script
collectable.Touched:Connect(function(hit)
	local player = game.Players:GetPlayerFromCharacter(hit.Parent)
	if player then
		--give the player a key if they haven't got one already.
		HideCollectable:FireClient(player,collectable) --fire a RemoteEvent to the client to hide the key from the player who touched.
	end
end)
--local script
HideCollectable.OnClientEvent:Connect(function(collectableToHide)
	collectableToHide:Destroy() -- remove the key on the LocalPlayer's client, that way it's still visible on the server.
end)
1 Like