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

What's wrong with this crate script? It's absorbing everything...

Asked by 6 years ago

i have no idea why its not working

local max = 10
local bame = script.Parent.bame.Value
local amt = script.Parent.amt.Value
local sty = script.Parent.styl.Value
    script.Parent.Touched:connect(function(item)
      if bame.Value == "none" `and amt.Value == s and item.Name == "pat" or item.Name == "tbun" or item.Name == "bbun" or item.Name == "bac" or item.Name == "tom" or item.Name == "che" or item.Name == "let" or item.Name == "pla" or item.Name == "mon" or item.Name == "rat" then   
        print("added "..item.Name)
        bame.Value = item.Name
        amt.Value = amt.Value + 1
        item:Destroy()
    elseif item.Name == "pat" and amt.Value~=max and bame.Value == "Patty" then
        amt.Value = amt.Value + 1
        sty = "pat"
        item:Destroy()
    elseif item.Name == "tbun" and amt.Value~=max and bame.Value == "TopBun" then
        amt.Value = amt.Value + 1
        sty = "tbun"
        item:Destroy()
    elseif item.Name == "bbun" and amt.Value~=max and bame.Value == "BottomBun" then
        amt.Value = amt.Value + 1
        sty = "bbun"
        item:Destroy()
    elseif item.Name == "bac" and amt.Value~=max and bame.Value == "Bacon" then
        amt.Value = amt.Value + 1
        sty = "bac"
        item:Destroy()
    elseif item.Name == "tom" and amt.Value~=max and bame.Value == "Tomato" then
        amt.Value = amt.Value + 1
        sty = "tom"
        item:Destroy()
    elseif item.Name == "che" and amt.Value~=max and bame.Value == "Cheese" then
        amt.Value = amt.Value + 1
        sty = "che"
        item:Destroy()
    elseif item.Name == "let" and amt.Value~=max and bame.Value == "Lettuce" then
        amt.Value = amt.Value + 1
        sty = "let"
        item:Destroy()
    elseif item.Name == "pla" and amt.Value~=max and bame.Value == "Plate" then
        amt.Value = amt.Value + 1
        sty = "pla"
        item:Destroy()
    elseif item.Name == "mon" and amt.Value~=max and bame.Value == "Cash" then
        amt.Value = amt.Value + 1
        sty = "mon"
        item:Destroy()
    elseif item.Name == "rat" and amt.Value~=max and bame.Value == "Rat" then
        amt.Value = amt.Value + 1
        sty = "rat"
        item:Destroy()
    elseif item.Name == "unload" then
        for i,v in pairs (game.Lighting.Pick:GetChildren()) do
            for i = 1,amt.Value do
                if v.Name == sty then
                    b = v:Clone()
                    b.Parent = workspace.Pick
                    b.CFrame = CFrame.new(Vector3.new(57.1,5.835,-80))
                    amt.Value = amt.Value - 1
                    wait(1.5)
                end
            end
        end
end end)
while true do
    wait()
    if bame.Value == "tbun" then bame.Value = "TopBun"
    elseif bame.Value == "pat" then bame.Value = "Patty"
    elseif bame.Value == "bbun" then bame.Value = "BottomBun"
    elseif bame.Value == "bac" then bame.Value = "Bacon"
    elseif bame.Value == "tom" then bame.Value = "Tomato"
    elseif bame.Value == "che" then bame.Value = "Cheese"
    elseif bame.Value == "let" then bame.Value = "Lettuce"
    elseif bame.Value == "pla" then bame.Value = "Plate"
    elseif bame.Value == "mon" then bame.Value = "Cash"
    elseif bame.Value == "rat" then bame.Value = "Rat"
    end
    script.Parent.SurfaceGui.amt.Text = ""..amt.Value.."/"..max
    if amt.Value <= 0 then
        amt.Value = 0
        bame.Value = "none"
    end
    script.Parent.SurfaceGui.name.Text = bame.Value
end

the script is supposed to fire when something touches the crate, if the name of that thing that touched the crate is one of the names listed above ("pat", "mon", "tom", etc.) then the thing gets destroyed, the bame value gets the things name and the amt value increases by one. after the bame value has been set to that thing, it cant be set to something else until the amt = 0. that was the desired result, but for some reason the crate overwrites the bame value with every item except for "pat" and i have no idea why, could someone explain this to me?

also removing the and amt.Value == 0makes the crate absorb everything including regular parts not listed in the item.Name == "" above

0
also sorry for my messy as heck coe Apostasla 27 — 6y
0
it seems if i replace `and item.Name == "pat"` with the name of another item, then that item is the item that works with the script. :thinking: Apostasla 27 — 6y

2 answers

Log in to vote
0
Answered by 6 years ago
Edited 6 years ago

Tips:

  • Write clean code! -- specifically, your spacing is off. Also, it would be quite nice if your variable names were more descriptive (though it's good you at least explained it to us in your question. Normally you should comment those so that if you leave the project for a couple weeks and go back to it, you won't have forgotten what they all mean/do.)
  • Learn how to use tables. Whenever you see a lot of very similar code, you should use functions and/or tables to make it more efficient. (I will demonstrate this below.)
  • Don't use while loops to keep values up-to-date. Use functions instead (I will demonstrate below).
  • connect is deprecated, use Connect instead
  • If you end up navigating to the same object multiple times, consider saving it to a local variable and using the local variable (ex you use script.Parent.SurfaceGui on lines 77 and 82).
  • You should open the Script Analysis window -- it tells you a variety of things that may be wrong with your script (ex it can often catch misspelled variable names, or catch times when you should be using a local variable).
  • You can use tables as either a list of something, a dictionary, and/or an object (similar to Roblox's classes, like "Part"). You should use one of the latter two options to convert your "code name" into your "english name" (of objects -- ex, "pat" to "Patty"). In your case, I recommend doing something like itemLookup = {pat="Patty", tbun="TopBun", ...} (replace the '...' with your continuation). You can even expand this later, if each object is going to have special properties:
data = {
    pat = {
        Name = "Patty",
        Value = 2,
    },
    tbun = {
        Name = "TopBun",
        Value = 1,
    },
    --etc
}
print(data.pat.Value)

Superior to both of what I just said would be to only use one name (ex just use "Patty" in all cases, never "pat").

Your if statements need work. Mainly, use parentheses whenever you're mixing and and or, at least if you don't know which order it will evaluate it. Your first if statement actually evaluates to true so long as item.Name is any valid name -- even if bame.Value isn't none. The solution in this case is simply to put parantheses around all the item.Name == "whatever" or part, so that it looks like this:

if bame.Value == "none" and amt.Value == 0 and (item.Name == "pat" or item.Name == "tbun" or item.Name == "bbun" or item.Name == "bac" or item.Name == "tom" or item.Name == "che" or item.Name == "let" or item.Name == "pla" or item.Name == "mon" or item.Name == "rat") then

At first you set sty to some StringValue (I expect), but then later assign strings to sty. If you wanted to update the StringValue with those strings, you need to specify sty.Value - otherwise, sty will just refer to the string you gave it, not the StringValue. (I use sty.Value below.)

Improved script:

local max = 10
local bame = script.Parent.bame.Value
local amt = script.Parent.amt.Value
local sty = script.Parent.styl.Value
local surfaceGui = script.Parent.SurfaceGui
local itemLookup = {
    --Dictionary of names that converts absorbable item.Name to appropriate bame.Value
    tbun="TopBun",
    pat="Patty",
    bbun="BottomBun",
    bac="Bacon",
    tom="Tomato",
    che="Cheese",
    let="Lettuce",
    pla="Plate",
    mon="Cash",
    rat="Rat",
    unload = "none", -- could remove this one if desired, but would need to specifically check for this when determining if something's a valid item or not
}
local function UpdateBameValue(newValue)
    bame.Value = newValue
    script.Parent.SurfaceGui.name.Text = newValue
end
local function UpdateAmtValue(newValue)
    if newValue <= 0 then -- Make sure newValue isn't negative before assigning to amt.Value
        newValue = 0
        UpdateBameValue("none")
    end
    amt.Value = newValue
    surfaceGui.amt.Text = string.format("%s/%s", newValue, max)
end
local function Unload()
    local b
    for i, v in pairs(game.Lighting.Pick:GetChildren()) do
        if v.Name == sty.Value then
            for i = 1, amt.Value do
                b = v:Clone()
                b.Parent = workspace.Pick
                b.CFrame = CFrame.new(Vector3.new(57.1,5.835,-80))
                UpdateAmtValue(amt.Value - 1)
                wait(1.5)
            end
        end
    end
end
local unloading = false
script.Parent.Touched:connect(function(item)
    -- Attempt to absorb the item; return early if it shouldn't be absorbed
    local name = itemLookup[item.Name]
    if not name then return end -- not a valid item
    if item.Name == "unload" then -- special case; deal with this first
        if unloading then return end -- we are already unloading so don't absorb this new item
        unloading = true
        Unload()
        unloading = false
    else
        if bame.Value == "none" and amt.Value == 0 then -- Empty; update bame and sty
            UpdateBameValue(name)
            sty.Value = item.Name
        elseif bame.Value ~= item.Value then -- cannot absorb this item at this time
            return
        end
        -- At this point we have a valid item to be absorbed that increases amt.Value
        UpdateAmtValue(amt.Value + 1)
    end
    -- At this point we have a valid item to be absorbed
    item:Destroy()
end)

(I expect that the code I provided will run correctly, but naturally I haven't tested it.)

Why I changed what I did:

  • UpdateBameValue and UpdateAmtValue are there so that whenever you make a change to amt.Value/bame.Value - so long as you use those two functions - they will also keep up-to-date the GUI like you wanted (and any other relevant variables, as you can see). This way, you don't need a while loop at the bottom of your script at all.
  • In UpdateAmtValue I use string.format, which you can learn more about here. In short, each "%s" is replaced with whatever argument you give it (so long as it can be converted to a string).
  • In the Unload function, I put the if v.Name == sty.Value then before the second for loop because if amt.Value = 5 (for example), it doesn't make sense to run the if statement 5 times -- especially if v.Name is not sty.Value!
  • I added unloading so that multiple 'unload' parts cannot be absorbed at the same time. This is the same concept as debounce.
  • You'll notice that the Touched function is significantly smaller and easier to read. This was as a direct result of using tables and functions. Similarly, it should be trivial to add, remove, or change the absorbable parts involved (you need only change the itemLookup table!)
  • Hopefully you'll agree that the comments also make it easy to follow what's going on

It is my hope that you understand the techniques I've shown. Let me know if there's anything you don't understand.

0
Wow, thanks! I had to edit the script a bit but it worked! :D Apostasla 27 — 6y
Ad
Log in to vote
0
Answered by 6 years ago

Your 'pat' has elseif .. I find it disturbing since you used 'elseif' after 'then', I'd use 'elseif' if the first 'if' is false ..

Answer this question