I’ve wrote some code that moves a moves a part via user input of the WASD keys, is this code efficient or does anything need to be changed/improved?
local Controller = game.Workspace.Controller
local KeyDown = false
local Positions = {}
Positions.W = "0,0,-1"
Positions.S = "0,0,1"
Positions.A = "-1,0,0"
Positions.D = "1,0,0"
local function StringToVector(string1)
local partsOfString = string.split(string1, ",")
local NewVector = Vector3.new(tonumber(partsOfString[1]),tonumber(partsOfString[2]),tonumber(partsOfString[3]))
return NewVector
end
function Positions:Move(KeyCode)
local Vector = StringToVector(Positions[KeyCode])
repeat wait(0.1)
local Vector = StringToVector(Positions[KeyCode])
Controller.Position = Controller.Position + Vector
until KeyDown == false
end
UIS.InputBegan:Connect(function(input, GPE)
if GPE then return end
if input.UserInputType == Enum.UserInputType.Keyboard then
local KeyCode = string.gsub(tostring(input.KeyCode), "Enum.KeyCode.", "")
if Positions[KeyCode] then
if KeyDown == false then
KeyDown = true
Positions:Move(KeyCode)
end
end
end
end)
UIS.InputEnded:Connect(function(input, GPE)
if GPE then return end
if input.UserInputType == Enum.UserInputType.Keyboard then
wait(0.2)
KeyDown = false
end
end)
The creation of new Vector3 values is relatively an expensive operation. As such, remove the StringToVector function to deserialize your Vector3 values, instead assigning Position.W to Vector3.new(0, 0, -1), Position.S to Vector3.new(0, 0, 1), and so on.
Also, remove the use of wait, repeat, and while when sensible in this context; instead, opt for event-driven architecture when possible.
If you have any questions as to how this might look and be implemented, let me know and I can draft a possible implementation.
as edgy said vector creation is expensive just set your values to actual vector3 values instead and reference them, for your inputbegan instead of your weird string thing going on you can just do
if Positions[Input.KeyCode.Name] then
I do disagree with edgy said about not using wait since your implementation of wait is completely fine
however when moving the position of the controller you need to be factor in the actual time of speed
so you use wait(0.1) for each step
local Step = wait(0.1)
Controller.Position = Controller.Position + Direction*Speed*Step -- example
Also assuming your using this controller for multiple objects it useful to have all controllers exist inside of an array table and have some sort of main loop to loop through the table and move each one, I do not recommand creating a thread for each controller
Here is a possible implementation for what you seek; I have commented it rather thoroughly so the key points of its design are easy to understand and fairly easy to replicate. Also, as I do with most other reviews I write in #help-and-feedback:code-review where I create a full code snippet: this is by no means a prescriptive solution. Rather, this is a guide to help you better inform your own style and techniques.
I was also able to test this in Roblox Studio (Yay! A lot of the time people ask questions no one but themselves can replicate). It, thankfully, works as anticipated:
local RunService = game:GetService("RunService");
local UserInputService = game:GetService("UserInputService");
-- Notice that any change to `movingPart` will likely not replicate to the
-- server. I assume this is intentional.
local movingPart = workspace.Controller;
-- Measured in studs per second.
local targetVelocity = 10;
local keyCodeToDirectionMap = {
[Enum.KeyCode.W] = Vector3.new(0, 0, -1),
[Enum.KeyCode.A] = Vector3.new(-1, 0, 0),
[Enum.KeyCode.S] = Vector3.new(0, 0, 1),
[Enum.KeyCode.D] = Vector3.new(1, 0, 0),
};
local keyCodeToIsPressedMap = {
[Enum.KeyCode.W] = false,
[Enum.KeyCode.A] = false,
[Enum.KeyCode.S] = false,
[Enum.KeyCode.D] = false,
};
--[[
A utility function that returns `true` if the given `input` is relevant to
this "controller".
@param {InputObject} input
@returns {boolean}
]]
local function isRelevantInput(input)
return (
input.KeyCode == Enum.KeyCode.W
or input.KeyCode == Enum.KeyCode.A
or input.KeyCode == Enum.KeyCode.S
or input.KeyCode == Enum.KeyCode.D
);
end
--[[
A utility function that returns which directions should be processed by the
moving part's manager.
@returns {Vector3[]}
]]
local function getDirectionsToProcess()
local directions = {};
for keyCode, isPressed in pairs(keyCodeToIsPressedMap) do
if isPressed then
local direction = keyCodeToDirectionMap[keyCode];
table.insert(directions, direction);
end
end
return directions;
end
--[[
An event handler that is meant to listen to `UserInputService.InputBegan`.
It modifies `keyCodeToIsPressedMap` accounting for engine context.
@param {InputObject} input
@param {boolean} gameProcessedEvent
@returns {nil}
]]
local function registerKeyPressed(input, gameProcessedEvent)
if gameProcessedEvent or not isRelevantInput(input) then
return;
end
keyCodeToIsPressedMap[input.KeyCode] = true;
end
--[[
An event handler that is meant to listen to `UserInputService.InputEnded`.
It modifies `keyCodeToIsPressedMap` accounting for engine context.
@param {InputObject} input
@param {boolean} gameProcessedEvent
@returns {nil}
]]
local function unregisterKeyPressed(input, gameProcessedEvent)
if gameProcessedEvent or not isRelevantInput(input) then
return;
end
keyCodeToIsPressedMap[input.KeyCode] = false;
end
--[[
An event handler that is meant to listen to `RunService.Heartbeat`. It
manages the part to be moved, taking into consideration the time elapsed
between frames. Refer to `targetVelocity` to adjust how fast the part moves.
@param {number} step
The time (in seconds) that has elapsed since the previous frame.
@returns {nil}
]]
local function managePartMovement(step)
local directions = getDirectionsToProcess();
local targetPosition = movingPart.Position;
for _, direction in ipairs(directions) do
-- We enforce this order of operations explicitly to minimize the amount
-- of new Vector3 values that are created.
local offset = (targetVelocity * step) * direction;
targetPosition = targetPosition + offset;
end
movingPart.Position = targetPosition;
end
UserInputService.InputBegan:Connect(registerKeyPressed);
UserInputService.InputEnded:Connect(unregisterKeyPressed);
RunService.Heartbeat:Connect(managePartMovement);
As always, if you have any questions from style to implementation, feel free to ask!