Scripting HelpersARCHIVE
A little piece of Roblox history. Preserved for everyone, one question at a time.Read about the archive →
✓ ANSWER ACCEPTED

I don't know how to fix this error?

I had this script running okay, then I made some changes which I want to do, and now it just isn't working, and I seriously don't know why. It throws this error:

20:47:03.109 - Players.Player1.PlayerGui.mouse_runner:58: attempt to index upvalue 'valid_target' (a boolean value) 20:47:03.110 - Stack Begin 20:47:03.110 - Script 'Players.Player1.PlayerGui.mouse_runner', Line 58 20:47:03.111 - Stack End

Here's my code:

repeat wait() until game.Players.LocalPlayer.Character and game.Players.LocalPlayer
local player = game.Players.LocalPlayer
local mouse = player:GetMouse()
local selection = Instance.new("SelectionBox",player.PlayerGui)
selection.Color = BrickColor.new("Medium stone grey")

local valid_target = player.valid_target
local current_target = player.current_target

mouse.Icon = "http://www.roblox.com/asset/?id=187412806"
mouse.Move:connect(function()
	local target = mouse.Target
	if target ~= nil then
		local hum = target.Parent:FindFirstChild("Humanoid")
		local enemy = target.Parent:FindFirstChild("enemy")
		local player = target.Parent:FindFirstChild("player")
		local book = target.Parent:FindFirstChild("book")
		local friend = target.Parent:FindFirstChild("friend")
		if hum and target.Parent:IsA("Model") and not player and enemy and current_target.Value ~= target.Parent then
			mouse.Icon = "http://www.roblox.com/asset/?id=187412859"
			selection.Adornee = target.Parent
			valid_target.Value = true
		elseif hum and target.Parent:IsA("Model") and not player and enemy and current_target.Value == target.Parent then
			mouse.Icon = "http://www.roblox.com/asset/?id=187412859"
			selection.Adornee = target.Parent
			valid_target = true
		end
		if hum and target.Parent:IsA("Model") and not player and book and current_target.Value ~= target.Parent then
			mouse.Icon = "http://www.roblox.com/asset/?id=187412830"
			selection.Adornee = target.Parent
			valid_target.Value = true
		elseif hum and target.Parent:IsA("Model") and not player and book and current_target.Value == target.Parent then
			mouse.Icon = "http://www.roblox.com/asset/?id=187412830"
			selection.Adornee = target.Parent
			valid_target.Value = true
		end
		if hum and target.Parent:IsA("Model") and not player and friend and current_target.Value ~= target.Parent then
			mouse.Icon = "http://www.roblox.com/asset/?id=187412846"
			selection.Adornee = target.Parent
			valid_target.Value = true
		elseif hum and target.Parent:IsA("Model") and not player and friend and current_target.Value == target.Parent then
			mouse.Icon = "http://www.roblox.com/asset/?id=187412846"
			selection.Adornee = target.Parent
			valid_target.Value = true
		end
		if hum and target.Parent:IsA("Model") and player and current_target.Value ~= target.Parent then
			mouse.Icon = "http://www.roblox.com/asset/?id=187425092"
			selection.Adornee = target.Parent
			valid_target.Value = true
		elseif hum and target.Parent:IsA("Model") and player and current_target.Value == target.Parent then
			mouse.Icon = "http://www.roblox.com/asset/?id=187425092"
			selection.Adornee = target.Parent
			valid_target.Value = true
		end
		if not hum or current_target.Value ~= target.Parent then
			mouse.Icon = "http://www.roblox.com/asset/?id=187412806"
			selection.Adornee = nil
			valid_target.Value = false
		end
	else
		mouse.Icon = "http://www.roblox.com/asset/?id=187412806"
		selection.Adornee = nil
		valid_target.Value = false
	end
end)

I'm really having trouble on this, so any help is much appreciated! Thanks for reading!

1 answer

✓ Accepted answer

"Upvalue" means "variable" (more or less).

It says that it can't index (using the . on) valid_target because it is a boolean value.

A boolean value is either true or false; we should look wherever we set valid_target to see how it might become true or false.


valid_target = true -- LINE 26

Probably, you meant to use valid_target.Value like in all of the other ones.


This is a lot longer than it needs to be.

Almost all of them use hum as a check. Just make another if to differentiate. These sames ones also require target.Parent:IsA("Model").

Most of those also use not player (the remainder use player) so another if else.

There remains a bunch of redundant checks, too with friend enemy book.

You get segments like this:

					if current_target.Value ~= target.Parent then
						mouse.Icon = "http://www.roblox.com/asset/?id=187412859"
						selection.Adornee = target.Parent
						valid_target.Value = true
					elseif current_target.Value == target.Parent then
						mouse.Icon = "http://www.roblox.com/asset/?id=187412859"
						selection.Adornee = target.Parent
						valid_target = true
					end

Which are pointless, since they do the same thing and at least one will always trigger.

We get this (I might have made a typo -- but at least make your code look like this! It's much more understandable)

....

mouse.Move:connect(function()
	local target = mouse.Target
	if target ~= nil then
		local hum = target.Parent:FindFirstChild("Humanoid")
		local enemy = target.Parent:FindFirstChild("enemy")
		local player = target.Parent:FindFirstChild("player")
		local book = target.Parent:FindFirstChild("book")
		local friend = target.Parent:FindFirstChild("friend")
		if hum and target.Parent:IsA("Model") then
			if player then
				mouse.Icon = "http://www.roblox.com/asset/?id=187425092"
				selection.Adornee = target.Parent
				valid_target.Value = true
			else
				if enemy then
					mouse.Icon = "http://www.roblox.com/asset/?id=187412859"
					selection.Adornee = target.Parent
					valid_target.Value = true
				end
				if book then
					mouse.Icon = "http://www.roblox.com/asset/?id=187412830"
					selection.Adornee = target.Parent
					valid_target.Value = true
				end
				if friend then
					mouse.Icon = "http://www.roblox.com/asset/?id=187412846"
					selection.Adornee = target.Parent
					valid_target.Value = true
				end
			end
		elseif current_target.Value ~= target.Parent then
			mouse.Icon = "http://www.roblox.com/asset/?id=187412806"
			selection.Adornee = nil
			valid_target.Value = false
		end
	else
		mouse.Icon = "http://www.roblox.com/asset/?id=187412806"
		selection.Adornee = nil
		valid_target.Value = false
	end
end)

You repeatedly refer to target.Parent (and in fact never target itself) after the check that it's not nil (although if target then would suffice and be cleaner), so let's make a variable for it.

We can collapse the weird ifs next to each other into one big elseif (in case they aren't mutually exclusive, I reversed the order to preserve the same effect)

....

mouse.Move:connect(function()
	local target = mouse.Target
	if target then
		local model = target.Parent
		local hum = model:FindFirstChild("Humanoid")
		local enemy = model:FindFirstChild("enemy")
		local player = model:FindFirstChild("player")
		local book = model:FindFirstChild("book")
		local friend = model:FindFirstChild("friend")
		if hum and model:IsA("Model") then
			if friend then
				mouse.Icon = "http://www.roblox.com/asset/?id=187412846"
				selection.Adornee = model
				valid_target.Value = true
			elseif book then
				mouse.Icon = "http://www.roblox.com/asset/?id=187412830"
				selection.Adornee = model
				valid_target.Value = true
			elseif enemy then
				mouse.Icon = "http://www.roblox.com/asset/?id=187412859"
				selection.Adornee = model
				valid_target.Value = true
			elseif player then
				mouse.Icon = "http://www.roblox.com/asset/?id=187425092"
				selection.Adornee = model
				valid_target.Value = true
			end
		elseif current_target.Value ~= model then
			mouse.Icon = "http://www.roblox.com/asset/?id=187412806"
			selection.Adornee = nil
			valid_target.Value = false
		end
	else
		mouse.Icon = "http://www.roblox.com/asset/?id=187412806"
		selection.Adornee = nil
		valid_target.Value = false
	end
end)

You have some code repeated at the end. Instead of using else, you should just set it at the beginning and let other things override it, to be much briefer.

You also repeat a bunch of code for each "type" of thing, we can simplify that with a variable, too:

mouse.Move:connect(function()
	local target = mouse.Target
	-- No target:
	mouse.Icon = "http://www.roblox.com/asset/?id=187412806"
	selection.Adornee = nil
	valid_target.Value = false
	-- Maybe a target:
	if target then
		local model = target.Parent
		local hum = model:FindFirstChild("Humanoid")
		local enemy = model:FindFirstChild("enemy")
		local player = model:FindFirstChild("player")
		local book = model:FindFirstChild("book")
		local friend = model:FindFirstChild("friend")
		if hum and model:IsA("Model") then
			local icon = nil
			if friend then
				icon = 187412846
			elseif book then
				icon = 187412830
			elseif enemy then
				icon = 187412859
			elseif player then
				icon = 187425092
			end
			if icon then
				selection.Adornee = model
				valid_target.Value = true
				mouse.Icon = "rbxassetid://" .. icon
			end
		end
	end
end)

This Move function is a lot cleaner this way.

It cut keywords from 39 to 30 (23% smaller)

This has cut lines down from 55 lines to 33 (40% smaller)

This has cut characters from 2576 to 847 (67% smaller)



Shorter code is more understandable and easier to work with. Big code means big problems and big confusion. Keep it tiny!

1 comment

0

This helps a lot! I REALLY appreciate this, I will apply this whenever I'm scripting. I'm sorry for my code, it usually becomes a lot longer than it really needs to.

This discussion is preserved as it was. New answers, comments, and votes are closed.