Control a block via WASD; any needed code optimization?

Help and Feedback Code Review
,
#1

Hey there! Long story short I made a tiny little game that uses remote events to move a block (that is binded to your character and moved via WASD) across the server.

For now the only “feature” I have implemented is that when your block hits another block, a 5 second timer goes off before you and your block explode along with your opponent.

If you could give any tips on minimizing or reducing any code that I have feel free. I’m a bit rusty to ROBLOX lua :slight_smile:

Control your Character via RBXL.rbxl (26.3 KB)

Main script for Remote Control (tool) 
--// Variables \\--
local CAS, RS, LocalPlayer = game:GetService("ContextActionService"), game:GetService("ReplicatedStorage"), game:GetService("Players").LocalPlayer
local Bot = game.Workspace:WaitForChild(LocalPlayer.Name .. "-bot")
local moveEvent = RS.MovePart
local tool = script.Parent

local MOVEMENT_KEYS = {
	["Forward"] = Enum.KeyCode.W,
	["Backwards"] = Enum.KeyCode.S,
	["Left"] = Enum.KeyCode.A,
	["Right"] = Enum.KeyCode.D
};

local function MoveObject(actionName, inputState, input)
	for i, v in pairs(MOVEMENT_KEYS) do
		if actionName == i and input.KeyCode == v then
			moveEvent:FireServer(actionName, input.KeyCode)
		end
	end
end


tool.Equipped:Connect(function()
	CAS:BindAction("Forward", MoveObject, false, Enum.KeyCode.W)
	CAS:BindAction("Backwards", MoveObject, false, Enum.KeyCode.S)
	CAS:BindAction("Left", MoveObject, false, Enum.KeyCode.A)
	CAS:BindAction("Right", MoveObject, false, Enum.KeyCode.D)
end)

tool.Unequipped:Connect(function()
	CAS:UnbindAction("Forward")
	CAS:UnbindAction("Backwards")
	CAS:UnbindAction("Left")
	CAS:UnbindAction("Right")
end)
Server-sided Movement handler 
--// Variables \\--
local CAS, RS, Players, TweenService = game:GetService("ContextActionService"), game:GetService("ReplicatedStorage"), game:GetService("Players"), game:GetService("TweenService")
local moveEvent = RS.MovePart
local debounce = false

local MOVEMENT_POSITIONS = {
	["Forward"] = Vector3.new(0, 0, 2),
	["Backwards"] = Vector3.new(0, 0, -2),
	["Left"] = Vector3.new(2, 0, 0),
	["Right"] = Vector3.new(-2, 0, 0)
};



--// Events \\--
moveEvent.OnServerEvent:Connect(function(player, direction)
	
	local playerBot = game.Workspace[player.Name .. "-bot"]
	if not playerBot then
		return
	end
	if not debounce then
		---debounce = true
	end
	
	local character = player.Character or player.CharacterAdded:Wait()
	
	if not character:FindFirstChild("Magic Car") then
		player:Kick("Illegally firing remotes.")
	else
		for i, v in pairs(MOVEMENT_POSITIONS) do
			if i == direction then
				local goal = {}
				goal.Position = playerBot.Position + v
				
				local tweenInfo = TweenInfo.new(0.06, Enum.EasingStyle.Linear, Enum.EasingDirection.Out)
				local tween = TweenService:Create(playerBot, tweenInfo, goal)
				
				tween:Play()
				return
			end
		end
	end
end)

Players.PlayerAdded:Connect(function(player)
	player.CharacterAdded:Connect(function()
		local playerBot = RS.Bot:Clone()
		playerBot.Name = player.Name .. "-bot"
		
		local spawnprotected = Instance.new("BoolValue", playerBot)
		spawnprotected.Name = "SpawnProtection"
		spawnprotected.Value = true
		
		playerBot.Color = Color3.new(math.random(0, 255), math.random(0, 255), math.random(0, 255))
		playerBot.Parent = game.Workspace
		playerBot.Position = Vector3.new(math.random(10, 30), math.random(10, 30), math.random(10, 30))
		playerBot.Anchored = false
		playerBot:SetNetworkOwner(player)
		wait(10)
		spawnprotected.Value = false
	end)
	
	player.CharacterRemoving:Connect(function ()
		local bot = game.Workspace:FindFirstChild(player.Name .. "-bot")
		if bot then
			bot:Destroy()
		end
	end)
	
end)

Players.PlayerRemoving:Connect(function(player)
	local bot = game.Workspace:FindFirstChild(player.Name .. "-bot")
	if bot then
		bot:Destroy()
	end
end)
Explosion handler 
countdownStarted = false

--/. Functions \\--
local function blowupSequence(object, objectOwner)
	if object then
		local counter = 1

		for i = 1, 5, 1 do
			print(counter)
			if counter > 4 then
				object.Color = Color3.new(0.666667, 0, 0)
				wait(1)
				object.Color = Color3.new(0, 0, 0)
				Instance.new("Explosion", object)
				Instance.new("Explosion", objectOwner)
				if object and objectOwner then
					object:Destroy()
					objectOwner:FindFirstChild("Humanoid"):TakeDamage(100)
				end
			else
				object.Color = Color3.new(0.666667, 0, 0)
				wait(1)
				object.Color = Color3.new(0.0117647, 0.0117647, 0.0117647)
				wait(1)
				counter = counter + 1
			end
		end
	end
end



local connection = script.Parent.Touched:Connect(function(hit)
	if not hit.Parent:FindFirstChild("Humanoid") and hit.Name ~= "Baseplate" then
		if string.match(hit.Name, "-") then
			repeat game:GetService("RunService").Heartbeat:Wait() until hit.SpawnProtection.Value == false
			
			local playerName = hit.Name:sub(1, hit.Name:find("-bot") - 1)
			local player = game.Workspace[playerName]
				
			local ownerName = script.Parent.Name:sub(1, hit.Name:find("-bot") - 1)
			local owner = game.Workspace[ownerName]
			local Thread = coroutine.wrap(blowupSequence)
			Thread(hit, player)
			blowupSequence(script.Parent, owner)
			countdownStarted = false
		end
	end
end)

Game Link

I included the RBXL so that people who prefer hands on can directly look at my code. I ask that you please don’t steal anything related to the script as I worked on it for a few hours.
Any and all suggestions are appreciated!

4 Likes
#2
Remote control: 
moveEvent:FireServer(actionName, input.KeyCode)

This means you’re sending a string over the network every time the player presses a button. Worst case that’s 9 chars = 9 bytes. You could just as well just send the first character in the action name, or even a number from 1-4 representing which kind of movement it is, which can be as little as a single byte if Roblox does this in a smart way, or if you encode it as base64 using the BitStream module. Not a big deal, but it’s something to keep in mind if you care about reducing network traffic.

    for i, v in pairs(MOVEMENT_KEYS) do
		if actionName == i and input.KeyCode == v then
			...
		end
	end

No need to iterate here. You could do this instead:

local action_key = MOVEMENT_KEYS[actionName]
if action_key and action_key == input.KeyCode then
    ...
else
    --This only happens if something has been programmed incorrectly
    warn( ("Nonexistent action name %s or mismatch between bound action and listed key %s."):format(action_name, tostring(input.KeyCode)) )
end

The warning stuff from before is a bit extra. You could get around that by not having "magic enums later in your script:

CAS:BindAction("Forward", MoveObject, false, MOVEMENT_KEYS["Forward"])

That way there’s no room for accidentally changing the table but not the BindAction call.

Server movement handler 
    local playerBot = game.Workspace[player.Name .. "-bot"]
	if not playerBot then
		return
	end

You’ve got the right idea but it won’t work. You have to use Workspace:FindFirstChild. Doing Workspace[“test”] when no child called “test” exists is going to throw an error, so your script would break if the bot didn’t exist.

    if not debounce then
		---debounce = true
	end

Don’t need it. And in general, try to avoid the “debounce” design pattern, at least as it’s commonly used in Roblox.

local character = player.Character or player.CharacterAdded:Wait()

This means that if it takes 2 seconds for the character to respawn, and the player spams 50 inputs in that time, those 50 inputs are going to happen all at once once they respawn. Is that really what you want? If not, just return early if the character isn’t found.

Use descriptive variable names, you’ll thank yourself in a couple of days when reading your old code.

for direction_name, direction_vector in pairs(MOVEMENT_POSITIONS) do
    if direction_name == input_direction_name then
        ...
    end
end

… but once again, no need to iterate when you can do a lookup, like so:

local direction_vector = MOVEMENT_POSITIONS[input_direction_name]
if direction_vector then
    ...
else
    warn( ("Invalid direction name '%s'."):format(input_direction_name) )
end

game.Workspace:FindFirstChild(player.Name .. "-bot")

you do that a lot. Maybe DRY up your code and turn it into a function? E.g.

function find_player_bot(player)
    return game.Workspace:FindFirstChild(player.Name .. "-bot")
end

It’s easier, and you don’t get bugs from changing how to look for bots in one place but not the other.

player.CharacterAdded:Connect(function()
    ...

    local spawnprotected = Instance.new("BoolValue", playerBot)

    ...
    
    wait(10)
    spawnprotected.Value = false
end)

This technically works because it’s fine to set some properties after an object has been destroyed, but just be aware that it’s possible for spawnprotected to be destroyed when the 10 seconds run out. Maybe do if spawnprotected.Parent then ... to check if it’s still safe to modify it.

Explosion stuff 
if string.match(hit.Name, "-") then ...

This really needs a comment explaining it. It seems to rely on some built-in assumptions about your game that makes it very hard to read. Maybe use a variable to help explain it, like so:

local is_a_bot = string.match(hit.Name, "-") ~= nil
if is_a_bot then ...

repeat game:GetService("RunService").Heartbeat:Wait() until hit.SpawnProtection.Value == false

could also do

hit.SpawnProtection.Changed:Wait()

whichever you prefer.

local playerName = hit.Name:sub(1, hit.Name:find("-bot") - 1)
...
local ownerName = script.Parent.Name:sub(1, hit.Name:find("-bot") - 1)

Instead of string formatting shenanigans (complicated, lots of built-in assumptions that may change) I would just have an ObjectValue inside each bot called “Player” or “Owner” that refers to the Player object you want to associate with it.

Hope this helps! If anything is confusing or you want a different explanation let me know :slight_smile: :+1:

2 Likes
#3

Thanks! You really thoroughly analyzed code and pointed out some good flaws that I myself realize. I understood all of it and I thank you for taking the time to reply!

2 Likes
#4

I think it’s worth mentioning that you can actually exclude the server from this for the most part, you don’t seem to be doing much sanity checking as far as feeding the server movement vectors goes.

Which means, at any point in time; what you could do is set the network ownership of the brick’s primarypart to the player via :SetNetworkOwner() and then have the client control it from then on via body mover replication through ownership, like this:

local keyEnumLookup = {

    [Enum.KeyCode.W] = Vector3.new(0, 0, -1),
    [Enum.KeyCode.S] = Vector3.new(0, 0, 1),
    [Enum.KeyCode.D] = Vector3.new(1, 0, 0),
    [Enum.KeyCode.A] = Vector3.new(-1, 0, 0),
}

game:GetService("ContextActionService"):BindAction("Vroom Vroom", function(_, state, input)
    local forceToApply = keyEnumLookup[input.KeyCode]

    bodyMover.Velocity = state == Enum.UserInputState.Begin and forceToApply or -forceToApply
end, false, Enum.KeyCode.W, Enum.KeyCode.S, Enum.KeyCode.A, Enum.KeyCode.D)

One thing to keep in mind is that this is technically vulnerable to movement exploits via abusing network owner, mostly just food for thought :smiley:

1 Like