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

Bricks Not Looping Through BrickColor Table?

Asked by 8 years ago

I wanted to try making neon lights on a tower change color based on a table of BrickColors I've set up, as well as rotate down and up the array so that the table doesn't abruptly move from color [5] to [1]. It's not doing anything at all however, so I hope you guys can help me optimize my code.

01local tower = script.Parent
02 
03local lightFolder = tower.Lights
04 
05local allLights = lightFolder:GetChildren()
06 
07local lightValue = nil
08local lightUp = true
09 
10colorTable = {BrickColor.new(1029), BrickColor.new(226), BrickColor.new(334), BrickColor.new(24), BrickColor.new(333)}
11 
12--            "Pastel Yellow        "Cool Yellow"        "Daisy Yellow"       "Bright Yellow"     "Gold"                      
13 
14while wait(2) do
15    if lightValue == nil then
View all 38 lines...

1 answer

Log in to vote
0
Answered by
BlueTaslem 18071 Moderation Voter Administrator Community Moderator Super Administrator
8 years ago
Edited 8 years ago

Advice / Problems

  • not nil doesn't make sense. That' just true. a == not nil does not mean "a is different from nil".
  • Don't use == true or == nil ever. Just if something then or if not something then suffices.
    • Don't use elseif when you mean else. Similarly, don't check conditions are the opposite of what they were in the first. That's the point of else and elseif
  • Don't mix different types of things together. There's no reason that lightValue should ever be nil. Start at 1.
  • Use a name like colors instead of colorTable -- saying "table" doesn't mean anything, it's just a filler word
  • Use the names for the BrickColor values. That lets you get rid of the comment -- the code can be understood on its own!

Cleaned-up

After beginning with lightValue = 1 and renaming colorTable to colors, we get this script:

01local tower = script.Parent
02local lightFolder = tower.Lights
03local allLights = lightFolder:GetChildren()
04 
05local lightValue = 1
06local lightUp = true
07 
08colors = {
09    BrickColor.new("Pastel yellow"),
10    BrickColor.new("Cool yellow"),
11    BrickColor.new("Daisy yellow"),
12    BrickColor.new("Bright yellow"),
13    BrickColor.new("Gold"),
14}
15 
View all 34 lines...

Fixing those two conditions,

01local tower = script.Parent
02local lightFolder = tower.Lights
03local allLights = lightFolder:GetChildren()
04 
05local lightValue = 1
06local lightUp = true
07 
08colors = {
09    BrickColor.new("Pastel yellow"),
10    BrickColor.new("Cool yellow"),
11    BrickColor.new("Daisy yellow"),
12    BrickColor.new("Bright yellow"),
13    BrickColor.new("Gold"),
14}
15 
View all 34 lines...

You have a "magic value" in your code. Why is that 5 as opposed to 4 or 6? In this case, it's the number of colors there are, #colors.

That number is in fact wrong -- you'll be skipping the last. Similarly, == 1 is wrong.

The fixed final version looks like this:

01local tower = script.Parent
02local lightFolder = tower.Lights
03local allLights = lightFolder:GetChildren()
04 
05local lightValue = 0
06local lightUp = true
07 
08colors = {
09    BrickColor.new("Pastel yellow"),
10    BrickColor.new("Cool yellow"),
11    BrickColor.new("Daisy yellow"),
12    BrickColor.new("Bright yellow"),
13    BrickColor.new("Gold"),
14}
15 
View all 34 lines...

A better start

Clean code reads like prose. You should start describing simply what you want.

You wanted to:

  • Show the 1st color
  • Show the 2nd
  • ...
  • Show the last color
  • Show the second to last color
  • Show the third to last color
  • ...
  • Show the second color
  • Show the first color
  • Show the second color
  • ...

So here is the code that does exactly that:

01function setColor(color)
02    for _, light in pairs(allLights) do
03        light.BrickColor = color
04    end
05end
06 
07colors = {
08    BrickColor.new("Pastel yellow"),
09    BrickColor.new("Cool yellow"),
10    BrickColor.new("Daisy yellow"),
11    BrickColor.new("Bright yellow"),
12    BrickColor.new("Gold"),
13}
14 
15while true do
View all 32 lines...

This is shorter than your original code and very clear, but very repetitive. However, it clearly shows the pattern: 1 2 3 4; 5 4 3 2; 1 2 3 4; 5 4 3 2.

This can be easily done with a for loop:

01local allLights = lightFolder:GetChildren()
02 
03function setColor(color)
04    for _, light in pairs(allLights) do
05        light.BrickColor = color
06    end
07end
08 
09colors = {
10    BrickColor.new("Pastel yellow"),
11    BrickColor.new("Cool yellow"),
12    BrickColor.new("Daisy yellow"),
13    BrickColor.new("Bright yellow"),
14    BrickColor.new("Gold"),
15}
View all 26 lines...

This is much simpler, because it's just doing precisely what you wanted. Don't be clever!

0
Wow, thanks for all the effort! It's my first time ever trying to do a long block of code by hand, so I'll definetly keep this in mind in the future. airgabe234 20 — 8y
Ad

Answer this question