Scripting Helpers is winding down operations and is now read-only. More info→
Ad
Log in to vote
0

What is wrong with this complicated Script?

Asked by 9 years ago

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

Log in to vote
1
Answered by
BlueTaslem 18071 Moderation Voter Administrator Community Moderator Super Administrator
9 years ago

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
0
This answer is so great, it's perfect. Thank you so much! Gwolflover 80 — 9y
Ad

Answer this question