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)
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