I have this piece of code that basically just looks at what your mouse has hovered over and sees if it should display a billboard GUI over it. I was wondering if there is a more efficient and faster way to check if it should display the GUI then the way I have right now.
UIS.InputChanged:Connect(function(input)
if mouse.Target then
if mouse.Target:FindFirstChild("Pickable") then
local item = mouse.Target
EtoPickup.Text = "E - Pickup"
PickUpGui.Adornee = item
ObjectName.Text = item.Name
PickUpGui.Enabled = true
elseif mouse.Target:FindFirstChild("LootBox") then
local item = mouse.Target
EtoPickup.Text = "E - Pickup"
PickUpGui.Adornee = item
ObjectName.Text = item.Name
PickUpGui.Enabled = true
elseif mouse.Target:FindFirstChild("BankMan") then
local item = mouse.Target
EtoPickup.Text = "E - Interact"
PickUpGui.Adornee = item
ObjectName.Text = "Bank"
PickUpGui.Enabled = true
elseif mouse.Target:FindFirstChild("StoreMan") then
local item = mouse.Target
EtoPickup.Text = "E - Interact"
PickUpGui.Adornee = item
ObjectName.Text = "Bank"
PickUpGui.Enabled = true
elseif mouse.Target:FindFirstChild("BlackSmith") then
local item = mouse.Target
EtoPickup.Text = "E - Interact"
PickUpGui.Adornee = item
ObjectName.Text = "BlackSmith"
PickUpGui.Enabled = true
elseif mouse.Target:FindFirstChild("Buyable") then
local item = mouse.Target
EtoPickup.Text = "E - Buy"
PickUpGui.Adornee = item
ObjectName.Text = item.Name
PickUpGui.Enabled = true
else
EtoPickup.Text = "E - Pickup"
PickUpGui.Adornee = nil
PickUpGui.Enabled = false
end
end
end)
As said above, your script seems totally fine the way it is and really can’t be simplified, however you could make a function for everything that happens inside the if statements.
One thing about improving code is figuring out what you can factor out. A lot of your branches have the same code, so you can take advantage of that and just shove the differences into the table for easy lookup. Additionally, in places where you don’t need some extra logic running past a big chunk of code you can use a guard clause to avoid the extra scope.
Here’s what my example refactor might look like:
local Interactive = {
Pickable = 'E - Pickup',
LootBox = 'E - Pickup',
BankMan = 'E - Interact',
StoreMan = 'E - Interact',
BlackSmith = 'E - Interact',
Buyable = 'E - Buy',
}
-- UIS.InputChanged seems like a weird place to check for mouse.Target
-- This fires for every key up and down
UIS.InputChanged:Connect(function(input)
local item = mouse.Target
if not item then -- guard clause to avoid whole new scope
return
end
local ok_text
for name, text in pairs(Interactive) do
if item:FindFirstChild(name) then
ok_text = text -- finds whichever item from the list is present
break
end
end
-- if an item is present, just do the single branch for it, otherwise disable the pickup gui
if ok_text then
EtoPickup.Text = ok_text
ObjectName.Text = item.Name
PickUpGui.Adornee = item
PickUpGui.Enabled = true
else
PickUpGui.Adornee = nil
PickUpGui.Enabled = false
end
end)
I also would suggest approaching it differently however. Right now you check for each item you could be interacting with, why not use something like a tag? If mouse.Target contains an Item tag, you know it’ll always be something you pick up instead of interact with. In that tag you could also maybe store information about the item. For an NPC, the NPC tag could be present and the value could be the internal name/ID of the NPC for example. This would avoid repeating the same code for BankMan, StoreMan, and such.