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

What is wrong with this complicated Script?

This script removes a model named Horse from the workplace when its in a certain range of the horse, but it is also supposed to remove the tool "Corral" from the player right when the "function on_mouse_down(mouse)" occurs, and the pos ~= nil. The problem is, it won't give me the tool named "Horse" after the Horse model that is in the workspace is destroyed. What is wrong with this script?

local TOOL = script.Parent
local NAME = TOOL.Name
local DEEDS = game.Lighting.Buildings
local DEED = DEEDS:findFirstChild(NAME)


if DEED == nil then
	print("DERP!!!")
	TOOL:remove()
end

DEED = DEED:clone()

function get_pc()
	local player = TOOL.Parent.Parent
	local char = player.Character

	return player, char
end

function get_all_parts(list, model)
	local children = model:GetChildren()

	for index, child in pairs(children) do
		if child:IsA("BasePart") then
			table.insert(list, child)
		end

		get_all_parts(list, child)
	end
end

function get_center(model)
	local list = {}
	get_all_parts(list, model)

	local num = 0
	local pos = Vector3.new(0, 0, 0)

	for index, child in pairs(list) do
		pos = pos + child.Position
	end

	return pos / num
end

function get_example_from_model(model)
	model = model:clone()

	do_trans(model)

	model.Name = "EXAMPLE"

	return model
end

function doRefToPlayer(deed)
	local player, char = get_pc()
	local deeds = player.Buildings

	local ref = Instance.new("ObjectValue")
	ref.Name = deed.Name
	ref.Value = deed
	ref.Parent = deeds
end
		
function do_trans(object)
	local children = object:GetChildren()

	for index, child in pairs(children) do
		if child:IsA("BasePart") then
			child.Transparency = 0.5
			child.Anchored = true
		elseif child:IsA("Model") then
			do_trans(child)
		else
			child:remove()
		end
	end
end

function do_rotate(objects, center, new, recurse)
	for _,object in pairs(objects) do
		if object:IsA("BasePart") then
			object.CFrame = new:toWorldSpace(center:toObjectSpace(object.CFrame))
		end

		if recurse then
			do_rotate(object:GetChildren(), center, new, true)
		end
	end
end

function do_remove_old_example()
	local player, char = get_pc()
	local children = char:GetChildren()

	for index, child in pairs(children) do
		if child.Name == "EXAMPLE" then
			child:remove()
		end
	end
end

function do_remove_tool()
	TOOL:remove()
end

function on_mouse_move(mouse)
	local pos = mouse.hit.p
	local player, char = get_pc()

	do_remove_old_example()

	if pos ~= nil then
		local example = get_example_from_model(DEED)
		example.Parent = char
		example:MoveTo(pos)
	end
end

function on_mouse_down(mouse)
	do_remove_old_example()

	local pos = mouse.hit.p

	if pos ~= nil then
		DEED.Parent = game.Workspace
		DEED:MoveTo(pos)
		DEED:MakeJoints()
        
		doRefToPlayer(DEED)
		do_remove_tool()-- This seems to be the problem, where I move this determines 
		--what is wrong with the script. in this place, it removes the tool at the correct time, but it 
		--messes up the rest of the script and it doesn't remove the horse from the workspace like it
		--is supposed to. when i had it in the place that says --here, it would correctly remove
		--the horse, but it wouldn't remove the tool unless there was a horse in range, which is wrong.
		local htool = game.Lighting.Tools.Horse
		
        local function in_Range(part1,part2,range)

    if (part1.Position - part2.Position).magnitude <= range then
	
        return true

        

    end

    return false

 

end


debounce = false

 

while wait(0.5) do


    if in_Range(game.Workspace.Corral.gate,game.Workspace.Horse.Torso,30) == true then

        if debounce == false then debounce = true

            game.Workspace.Horse:Destroy()
            htool:Clone() .Parent = TOOL.Parent
           
            wait(3)


        debounce = false end
 --Here
    end

end

		
	end
end

function on_key_down(key, mouse)
	if key == "r" then
		local list = {}
		get_all_parts(list, DEED)
		local center = DEED:GetModelCFrame()
		local rotated = center * CFrame.Angles(0, math.pi / 2, 0)

		do_rotate(list, center, rotated, false)
		on_mouse_move(mouse)
	end
end

function on_selected(mouse)
	mouse.Move:connect(function()
		on_mouse_move(mouse)
	end)

	mouse.Button1Down:connect(function()
		on_mouse_down(mouse)
	end)

	mouse.KeyDown:connect(function(key)
		on_key_down(key, mouse)
	end)
end

function on_deselected()
	do_remove_old_example()
end

TOOL.Selected:connect(on_selected)
TOOL.Deselected:connect(on_deselected)

1 answer

✓ Accepted answer

The first problem is that this script is complicated. At the very least, it should be formatted well -- no pointless extra lines, and correct tabbing. That would look something like this.


in_Range shouldn't be a local function. It's name isn't consistent with either of the two styles of names you're using. It doesn't depend on its surrounding function at all. It can also be made to be one line long:

function inRange(part1,part2,range)
	return (part1.Position - part2.Position).magnitude <= range
end

Similarly, no need to say == true.


pos can't be nil (mouse.Target can, when you're hovering over the sky, but even then .Hit is still defined). You're supposed to use Hit, not hit.


You don't need get_pc() since the values don't change once the script starts. Just define them at the top:

local player = TOOL.Parent.Parent
-- or game.Players.LocalPlayer from a LocalScript
local character = player.Character or player.CharacterAdded:wait()

get_center isn't used.


do_trans is far more complicated than it needs to be (it's also named horribly).

You already have get_all_parts:

function setTransparency(model, transparency)
	local parts = {}
	get_all_parts(parts, model)
	for _, part in pairs( parts ) do
		part.Transparency = transparency
	end
end

Coincidentally, we can change get_all_parts to simplify how we use it. First, swap the argument order: model, list. Next, make list optional by using list = list or {}. Finally, return list:

function get_all_parts(model, list)
	list = list or {}
	for _, child in pairs(model:GetChildren()) do
		if child:IsA("BasePart") then
			table.insert(list, child)
		end
		get_all_parts(child, list)
	end
	return list
end

It's now even briefer:

function setTransparency(model, transparency)
	for _, part in pairs( get_all_parts(model) ) do
		part.Transparency = transparency
	end
end

do_remove_old_example can be simplified. Since you should only have one example anyway, you can use a simple :FindFirstChild. (You can use a loop to be certain, but you have a mistake elsewhere if you have multiple example models)

There's no reason to have do at the beginning of a name. Methods are verbs for a reason -- they say what they're doing.

function remove_old_example()
	if char:FindFirstChild("EXAMPLE") then
		char.EXAMPLE:Destroy()
	end
end

Use workspace instead of game.Workspace.

You should use ReplicatedStorage instead of Lighting to store things.


If you destroy the tool, then the script will stop executing. That is not what you want to do when you have more work to do.

I don't understand the purpose of the while loop, or the purpose of the debounce. If you want this tool to go away, there's no reason to have a period of three seconds where it exists but isn't supposed to do anything.

There's really no reason that I can understand to have the while loop start once you clicked. Why not just check that in addition to being in range, you have a DEED, and run constantly?


This seems inherently unfriendly to multiplayer, since there's only one workspace.Horse. I'm not sure if you mean to be using DEED in those cases or not.

Here is what I have after all of the above suggestions being applied:

local TOOL = script.Parent
local NAME = TOOL.Name
-- Use ReplicatedStorage
local DEEDS = game.Lighting.Buildings
local DEED = DEEDS:findFirstChild(NAME)

assert(DEED, "DEED did not exist)")

DEED = DEED:clone()

local player = TOOL.Parent.Parent
-- or game.Players.LocalPlayer from a LocalScript
local character = player.Character or player.CharacterAdded:wait()

function get_all_parts(model, list)
	list = list or {}
	for _, child in pairs(model:GetChildren()) do
		if child:IsA("BasePart") then
			table.insert(list, child)
		end
		get_all_parts(child, list)
	end
	return list
end

function get_example_from_model(model)
	model = model:clone()
	setTransparency(model, 0.5)
	model.Name = "EXAMPLE"
	return model
end

function doRefToPlayer(deed)
	local deeds = player.Buildings

	local ref = Instance.new("ObjectValue")
	ref.Name = deed.Name
	ref.Value = deed
	ref.Parent = deeds
end

function setTransparency(model, transparency)
	for _, part in pairs( get_all_parts(model) ) do
		part.Transparency = transparency
	end
end

function rotate(objects, center, new, recurse)
	for _, object in pairs(objects) do
		if object:IsA("BasePart") then
			object.CFrame = new:toWorldSpace(center:toObjectSpace(object.CFrame))
		end

		if recurse then
			rotate(object:GetChildren(), center, new, true)
		end
	end
end

function remove_old_example()
	if char:FindFirstChild("EXAMPLE") then
		char.EXAMPLE:Destroy()
	end
end

function remove_tool()
	TOOL:remove()
end

function on_mouse_move(mouse)
	local pos = mouse.Hit.p

	remove_old_example()

	local example = get_example_from_model(DEED)
	example.Parent = char
	example:MoveTo(pos)
end

function inRange(part1, part2, range)
	return (part1.Position - part2.Position).magnitude <= range
end

function on_mouse_down(mouse)
	remove_old_example()

	local pos = mouse.Hit.p

	DEED.Parent = workspace
	DEED:MoveTo(pos)
	DEED:MakeJoints()

	doRefToPlayer(DEED)
end

function on_key_down(key, mouse)
	if key == "r" then
		local list = get_all_parts(DEED)
		local center = DEED:GetModelCFrame()
		local rotated = center * CFrame.Angles(0, math.pi / 2, 0)

		rotate(list, center, rotated, false)
		on_mouse_move(mouse)
	end
end

function on_selected(mouse)
	mouse.Move:connect(function()
		on_mouse_move(mouse)
	end)

	mouse.Button1Down:connect(function()
		on_mouse_down(mouse)
	end)

	mouse.KeyDown:connect(function(key)
		on_key_down(key, mouse)
	end)
end

function on_deselected()
	dremove_old_example()
end

TOOL.Selected:connect(on_selected)
TOOL.Deselected:connect(on_deselected)

-- Use ReplicatedStorage
local htool = game.Lighting.Tools.Horse

while wait(0.5) do
	if DEED.Parent and inRange(workspace.Corral.gate, workspace.Horse.Torso, 30) then
		workspace.Horse:Destroy()
		htool:Clone().Parent = TOOL.Parent
		remove_tool()
	end
end

1 comment

0

This answer is so great, it's perfect. Thank you so much!

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