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

Is this script for automating light sources depending on TimeOfDay efficient?

Mainly a question for the scripting pros, is this an efficient way to do what I'm trying to do? The script works perfectly fine I was just curious.

-- Services --
local Lighting = game:GetService("Lighting")

-- Directories
local Workspace_LightSources = game.Workspace:GetChildren()

-- Tables
local Table_LightSources = {}

-- Basic Values --
local Enabled_Material = Enum.Material.Neon -- Material when Source is on.
local Disabled_Material = Enum.Material.SmoothPlastic -- Material when Source is off.

-- Fill Table 
for _,a in pairs (game.Workspace:GetChildren()) do -- find all Lightsources in workspace and its children
	if a.Name == "LightSource" then
		table.insert(Table_LightSources,a)
	else
		for _,b in pairs (a:GetChildren()) do
			if b.Name == "LightSource" then
				table.insert(Table_LightSources,b)
			else
				for _,c in pairs (b:GetChildren()) do
					if c.Name == "LightSource" then
						table.insert(Table_LightSources,c)
					end
				end
			end
		end
	end
end

-- Light Sources Handler --
function LightChange(Status)
	if Status == true then
		for _,a in ipairs (Table_LightSources) do
			a.Material = Enabled_Material
			local LightChildren = a:GetChildren()
			for _,b in pairs (LightChildren) do
				if b.ClassName == "PointLight" or "SpotLight" or "SurfaceLight" then
					b.Enabled = true
				else
					return
				end
			end
		end
	elseif Status == false then
		for _,a in ipairs (Table_LightSources) do
			a.Material = Disabled_Material
			local LightChildren = a:GetChildren()
			for _,b in pairs (LightChildren) do
				if b.ClassName == "PointLight" or "SpotLight" or "SurfaceLight" then
					b.Enabled = false
				else
					return
				end
			end
		end
	end
end

-- Automatic Lighting --
while true do
	wait(.1)
	local MinutesAfterMidnight = game.Lighting:GetMinutesAfterMidnight()
	game.Lighting:SetMinutesAfterMidnight(MinutesAfterMidnight + 10)
	
	if MinutesAfterMidnight > (18 * 60) or MinutesAfterMidnight < (5.5 * 60) then
		LightChange(true)
	elseif MinutesAfterMidnight < (18 * 60) or MinutesAfterMidnight > (5.5 * 60) then
		LightChange(false)
	end
end

Old Version

7 comments

0

Not really you have a lot of duplicate code and why not make a table with the parts in?

0

I really haven't learned about tables...

0

They would really help for this kind of task.

0

do you have a link to a tutorial about tables?

0

I've updated it with a table. It works but I'm not sure if I did it entirely correctly.

0

That look ok, you next need to update the function LightChange as we should only be adding the parts we want to the liste meaning that this function only needs to loop through the processed table list and set its data.

1 answer

✓ Accepted answer

(Written for v2 of your script)

Good job upgrading your script; there is still room for improvement.

First, I will assume tha tyou want to find all LightSources in the entire workspace. To do this, you use a recursive function -- that is, a function that calls itself. You can do this to look through all objects in the workspace.

local Table_LightSources = {}
function FindLights(where)
	for _, child in pairs(where:GetChildren()) do
		if child.Name == "LightSource" then
			table.insert(Table_LightSources, child)
		else
			FindLights(child)
		end
	end
end
FindLights(workspace)

(With that, you no longer need Workspace_LightSources.)

Now, say that you wanted to only look at a maximum depth of 3. That is, you'd be willing to look at workspace.Object1.Object2.Object3, but not workspace.Object1.Object2.Object3.Object4. Simply add a "depthLeft" variable:

local Table_LightSources = {}
function FindLights(where, depthLeft)
	for _, child in pairs(where:GetChildren()) do
		if child.Name == "LightSource" then
			table.insert(Table_LightSources, child)
		elseif depthLeft > 0 then --only recurse if we have depth left
			FindLights(child, depthLeft - 1)
		end
	end
end
FindLights(workspace, 3)

Next, notice how your LightChange function is mostly identical code put twice? You can do this instead:

function LightChange(Status)
	local Material = Status and Enabled_Material or Disabled_Material
	for _,a in ipairs (Table_LightSources) do
		a.Material = Material
		for _, b in ipairs(a:GetChildren()) --you don't need the LightChildren variable
			if b.ClassName == "PointLight" or b.ClassName == "SpotLight" or b.ClassName == "SurfaceLight" then --NOTE: You MUST repeat "b.ClassName" each time or else you're just saying "if ClassName is a PointLight or if the string 'SpotLight' is not false/nil or if the string 'SurfaceLight' is not false/nil".
				b.Enabled = Status --since Status is true/false already, we can use that here
			--NOTE: "else return" removed, or else the moment you have a non-point-light child of a LightSource, your script will stop updating all the other LightSources.
			end
		end
	end
end

Be sure to read the comments I added.

In lua, everything evaluates to true unless it is false or nil. Thus, if Status == true then is redundant in this case; you could simply put if Status then.

Status and Enabled_Material or Disabled_Material is sort of like a special if. The syntax is "expression and valueIfExpressionIsTrue or valueIfExpressionIsFalse", but valueIfExpressionIsTrue must not be false or nil, or else valueIfExpressionIsFalse will be chosen instead. Since Enabled_Material is clearly not false/nil, it is safe to use this. A similar syntax is value = value or default, useful when accepting optional arguments in functions. Like the "and or" one, if 'value' is false/nil, 'default' will be chosen.

If you didn't want to use the "and or" method, here's the equivalent if:

	local Material
	if Status then
		Material = Enabled_Material
	else
		Material = Disabled_Material
	end

Now there is one last thing I can see: you run LightChange every 0.1 seconds regardless of if you need to or not. You need to have a boolean flag (variable) somewhere that keeps track of the last value you sent to LightChange. You can either do this in the LightChange function (so it will be responsible for not changing the lights a 2nd time unnecessarily), or put the responsibility in the while true do loop. One advantage of putting it in the function is that if you started using the function elsewhere in the script, you wouldn't have to consider variables from the while true do loop, simplifying the code. Here's the required addition:

local LastStatus --starting value of nil will work because LightChange is never called with nil
function LightChange(Status)
	if LastStatus == Status then return end
	LastStatus = Status
	local Material = Status and Enabled_Material or Disabled_Material
	--etc
end

A stylistic note: you've been capitalizing variable names in a fashion I haven't seen before. It's your choice, of course, but it's not a bad idea to use a more common style; usually variables do not have their first letter capitalized (unless it's a property, like "brick.BrickColor" -- BrickColor is capitalized, but it is a property, not a variable, though on the surface it acts the same way. A property is something that, when you retrieve or assign the value, runs a custom function. This is why, when you assign a brick's Position, its CFrame changes automatically as well -- the property is set up to keep them consistent.) You may wish to consider variable_name or variableName.

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