Initialising function for VFX Manager

[colbert2677 (Dreamseeker)]

I’ve created a VFX Manager in order to modularise a bunch of loose-hanging code responsible for applying effects in my game. It looks good but I’m dissatisfied almost exclusively with the way I’ve structured my init(ialisation) function. Something about it looks horrible.

The content of my init function looks like this:

local function init()
	-- Collect common modules
	cacheVFXModules(Common, script.Common)
	
	-- Prepare CollectionService visualisers
	cacheVFXModules(ObjectVFX, script.Objects)
	CollectionService:GetInstanceAddedSignal(VFX_TAG):Connect(linkObjectVFX)
	CollectionService:GetInstanceRemovedSignal(VFX_TAG):Connect(unlinkObjectVFX)
	Common.FastSpawn(function ()
		for _, object in ipairs(CollectionService:GetTagged(VFX_TAG)) do
			linkObjectVFX(object)
		end
	end)
	
	-- Prepare ambient visualisers
	cacheVFXModules(AmbientVFX, script.Ambient)
	Common.FastSpawn(function ()
		for _, visualiser in ipairs(AmbientVFX) do
			visualiser.Start()
		end
	end)
end

cacheVFXModules is a function that requires, wraps and places the immediate modules of a folder into a dictionary. It is split into two types: ambient (these are run automatically via their Start function and are typically for grouped, static or other types of items) and objects (these are explicitly linkable with CollectionService and usually apply to a single object directly via the Link function).

I feel like there’s a far better way to be handling these modules which is why I’ve brought over my code to Code Review. I’ve referenced a couple of frameworks and other code that also does caching and deploying like this, but they look way cleaner. Could just be my perfectionist voice speaking and this could be completely fine.

A third party to reassure me that this is okay or that it needs changes would be appreciated. Looking for substantiated, knowledgeable answers - no speculation, no unnecessary changes and no questions that can be researched on your own time. I will not be replying to any posts that aren’t related to reviewing code or obtaining relevant information. Please keep it focused. Thank you!

The full source is available below. Feel free to use it in your projects if it tickles your fancy as well, but you probably won’t find any good use for it:

--- Manages visual effects for certain items.
-- @author colbert2677

local CollectionService = game:GetService("CollectionService")

local Common = {}
local AmbientVFX = {}
local ObjectVFX = {}

local VFX_TAG = "Apply VFX"

local commonMeta = {__index = Common}

--- Store the immediate modules of a folder into a table
-- @param tbl the table to add the modules to
-- @param storage the folder containing the modules
local function cacheVFXModules(tbl, storage)
	for _, module in ipairs(storage:GetChildren()) do
		local content = require(module)
		if module.Parent ~= script.Common then
			setmetatable(content, commonMeta)
		end
		tbl[module.Name] = content
	end
end

--- Link effects to an object
-- @param object a newly tagged object
local function linkObjectVFX(object)
	local effects = ObjectVFX[object.Name]
	if effects then
		effects.Link(object)
	end
end

--- Unlink effects from an object
-- @param object an object that was removed from a collection
local function unlinkObjectVFX(object)
	local effects = ObjectVFX[object.Name]
	if effects and effects.Unlink then
		effects.Unlink(object)
	end
end

--- Run the manager
local function init()
	-- Collect common modules
	cacheVFXModules(Common, script.Common)
	
	-- Prepare CollectionService visualisers
	cacheVFXModules(ObjectVFX, script.Objects)
	CollectionService:GetInstanceAddedSignal(VFX_TAG):Connect(linkObjectVFX)
	CollectionService:GetInstanceRemovedSignal(VFX_TAG):Connect(unlinkObjectVFX)
	Common.FastSpawn(function ()
		for _, object in ipairs(CollectionService:GetTagged(VFX_TAG)) do
			linkObjectVFX(object)
		end
	end)
	
	-- Prepare ambient visualisers
	cacheVFXModules(AmbientVFX, script.Ambient)
	Common.FastSpawn(function ()
		for _, visualiser in ipairs(AmbientVFX) do
			visualiser.Start()
		end
	end)
end

init()

[Azatiel (Aza)]

As one perfectionist to another, I will emphasize the understanding that perfection is a long and untenable road. There are one million and one ways to write an optimal system, and I have done that more times than I want to admit.

Speaking with 8 years of RBX Lua under my belt: All programmers learn differently, and most result in a unique style. The only way to hone your ability is to constantly be trying to improve what you have, even if what you have works more than sufficiently. It takes a special kind of mentality to commit yourself to a constant personal challenge. So I applaud you for pursuing it.

So with that out of the way and taking a look at your source. If I was to write a script that functioned as closely as you’ve written it here, It would look like this:

local CollectionService = game:GetService('CollectionService');
local vfxLibraries, metaLibraries = {'Common', 'Ambient', 'Objects'}, {['Common'] = true};
--
local vfxTag = 'Apply VFX';
--
for _, library in ipairs(vfxLibraries) do -- Compile modules in a single scalable table, applying inheritance where needed
	vfxLibraries[library] = {};
	--
	for module in ipairs(script:WaitForChild(library):GetChildren()) do
		if module:IsA('ModuleScript') then
			if metaLibraries[library] then
				vfxLibraries[library][module.Name] = setmetatable(require(module), {__index = library});
			else
				vfxLibraries[library][module.Name] = require(module);
			end
		end
	end
end
--
local function linkObjectVFX(object) -- Simple but safe link execution
	if vfxLibraries.Objects and vfxLibraries.Objects[object.Name] and vfxLibraries.Objects[object.Name].Link then
		vfxLibraries.Objects[object.Name].Link(object)
	end
end
local function unlinkObjectVFX(object) -- Simple but safe unlink execution
	if vfxLibraries.Objects and vfxLibraries.Objects[object.Name] and vfxLibraries.Objects[object.Name].Unlink then
		vfxLibraries.Objects[object.Name].Unlink(object)
	end
end
--
CollectionService:GetInstanceAddedSignal(vfxTag):Connect(linkObjectVFX);
CollectionService:GetInstanceRemovedSignal(vfxTag):Connect(unlinkObjectVFX);
--
for _, object in ipairs(CollectionService:GetTagged(vfxTag) or {}) do
	coroutine.wrap(linkObjectVFX)(object) -- This is fired asyncrounously just incase you yield in your link execution, we don't want to hold up the line.
end
--
for _, visualiser in ipairs(vfxLibraries.Ambient) do
	if visualiser.Start then
		coroutine.wrap(visualiser.Start)() -- THis is also async for similar reasons as the other coroutine.
	end
end

You don’t need a complicated framework 99% of the time. Just a simple system that is straight to the point and performant enough to the extent of your needs.

This seems simple enough that it should suffice, and still be manageable if you wanted to add more modules.

If you find my approach unnecessary then all’s fair, but if you do manage to take something away from it, I’ll be happy.

[colbert2677 (Dreamseeker)]

I appreciate the response but this isn’t quite related to what I asked on the thread. I didn’t ask for a rewrite, I asked for a review on a function and whether it is problematic or technically poor or if it is fine the way it is. The complexity is actually not that high of a level and this doesn’t provide me any new information on my current implementation.

The code sample that you’ve given me also has a few problems in it that take away from what my system originally did, meaning that this is actually an anti-solution. You need to be able to understand what exactly the system is doing before you’re able to make a review. I understand the sample you provided is how you would write it and I appreciate it, but that’s not necessarily what Code Review is for.

My problems with this:

• Sorry but the general readability is worse. I don’t want to sacrifice that readability if I can for something that looks “simpler” because simpler does not necessarily mean better. Also, there are points in which this is less performant or desirable than the original source.

• Naming convention is gone and replaces everything with camelCase. Fairly trivial issue if I do say so myself, but I am very big on readability. I follow a loose version of Roblox’s style guide.

• There will always only be two types of modules: Ambient and Object. There is, therefore, no need to make my fetching completely arbitrary. The way I have it is mostly for readability’s sake: one function which then gets run on respective folders.

• The ipairs here is used incorrectly and will fail in production. It is only using a single variable which is the key and GetChildren returns an array. It will attempt to perform a lookup on that table with a numerical key and no such module is numerically named.

• I’m not too sure what your setmetatable is doing or why inheritance is relevant here, because it isn’t. The point of Common, as well as the setmetatable call, is so that other required modules are able to access Common modules in the package without explicitly writing out requires repeatedly at the top of each VFX module.

• The number of table lookups is more expensive than what I’m currently doing. I’ve been able to cut down on table lookups the way I currently have it. Same goes with the functions below. You can cut down by making the “library” (they aren’t) tables accessible through a local variable

• Objects must always have a linker, but the unlinker is optional. That’s the point of the way the original functions are set up.

• The FastSpawn takes care of the coroutine bit.

I am looking for, as the thread states, feedback on my code, not for a new system or way of doing things unless it’s related to improving the init function if such improvements are actually warranted. This would be better suited for non-specific Code Review requests (which don’t belong here anyway) or support threads, none of which my thread are. There’s a very specific part I’m looking feedback on, the init function and the code is extra context if needed to help in that review, such as changing that in order to clean up or improve the init function.

[Azatiel (Aza)]

Your view is fair, but I have a couple counter points (in as best order I can provide to your own)

• In my experience, simplicity provides more readability than naming conventions and externalized functions. I find it much more straight forward to read a block of code in a couple seconds and understand how it executes versus jumping around from uniquely labeled variables and functions (especially if they’re spread around, though this isn’t the case in your source).

• camelCase is just personal habit, and I find is easier to read. This is more subjective, and bias to my own view on simplicity over readability.

• The approach I took was from the view of scalability. I always try to approach systems or frameworks from a position of scalability because I never know if I’m going to want to make an addition or change. If I do, I don’t want to have to add more calls nested deep within the main body (which could be apart of a dense system).

• The second ipairs is a typo, It was fairly late, and I missed a simple "_, ". With the correction, it will function as intended. The ipairs function when provided an array will reference a number index for a key, and the value. In this case it would have unassigned the key, and only looked at the value. It does in fact work, fairly easy to test. Just use for _, Value in ipairs(Any Array) do as an example.

• The inheritance was modeled after your own from my read of your source. While I did apply self inheritance to common, the result is still identical. (I saw some issues, including how you intended to call Common.FastSpawn, when Common was an empty table)

• You are correct, the number of table look ups is more expensive than simply referencing static modules. However, with the approach of scalability, one should usually write some safeties to ensure that one typo does not throw off the entire system (given a larger scale system, again just a habit).

• The Linker and Unlinker should operate no differently from what I can see.

• The use of fast spawn seems odd, and unnecessary. Assuming it’s identical to spawn(); You may have to explain why calling a seemingly non existent function with a created anonymous function is more ideal than simply wrapping an existing function in a coroutine and firing it. I might just be missing something.

Overall, as you say this isn’t a script support topic, so I won’t argue back and fourth about which is the best approach or why. My aim was to provide some simple points of improvement, and It seemed more straight forward to give you my example and highlight the differences.

To try my best to answer your specific focus, your init function seems perfectly fine. The only issue that jumps out at me is scalability and the FastSpawn call for the reason listed above.

Even then it’s not a complex block of code (though I am only viewing a small portion of it, not the body of the link, unlink, or visualizers modules for reference), so you can only optimize it so much.

Hopefully this clears up any particulars:+1: I look forward to your thoughts.

[colbert2677 (Dreamseeker)]

The entire point of me replying is that your feedback is an anti-solution to what I’ve requested on the thread and isn’t relevant. The comments I’ve offered are afterthoughts, but I’m not here to debate on whether or not your approach is what I want to take or the semantics of your code.

I do not like your approach at all. The counterpoints haven’t shed any new information on the workability of my init function and I disagree with them still, but that’s not a discussion I want to be having here. I’m only interested in the improvement of my own code.

I appreciate the response but it’s not what I’m looking for. I’m looking for actual signal that’s based on what I’ve put out in the original post, not noise.

[colbert2677 (Dreamseeker)]

In the end I performed a bit of my own Code Review and touching up, with some references from outside resources on how they set up their own similarly-structured managers. I was able to make a few touch ups and in cases where it wasn’t a big deal, could ignore organisation nitpicks.

I’ve also made a couple of other edits. My full audit:

• Applied clearer variable names reflecting the purpose of each variable.

• Microoptimisation on collectModules (formerly cacheVFXModules) that does a parent check only once instead of for each module, since its parent is predictable.

• Added some functions local to the scope of init just to help with readability and in-lining (or I think that’s what this practice is called?).

Updated code:

--- Manages visual effects for certain items.
-- @author colbert2677

local CollectionService = game:GetService("CollectionService")

local FastSpawn = require(script.Common.FastSpawn)

local CommonModules = {}
local AmbientVFX = {}
local ObjectVFX = {}

local VFX_TAG = "Apply VFX"

local metatable = {__index = CommonModules}

--- Store the immediate modules of a folder into a table
-- @param tbl the table to add the modules to
-- @param folder the folder containing the modules
local function collectModules(tbl, folder)
	local shouldAttachCommonModules = folder.Name ~= "Common"
	
	for _, module in ipairs(folder:GetChildren()) do
		local content = require(module)
		if shouldAttachCommonModules then
			setmetatable(content, metatable)
		end
		tbl[module.Name] = content
	end
end

--- Link effects to an object
-- @param object a newly tagged object
local function linkObjectVFX(object)
	local effects = ObjectVFX[object.Name]
	if effects then
		effects.Link(object)
	end
end

--- Unlink effects from an object
-- @param object an object that was removed from a collection
local function unlinkObjectVFX(object)
	local effects = ObjectVFX[object.Name]
	if effects and effects.Unlink then
		effects.Unlink(object)
	end
end

--- Run the manager
local function init()
	local function linkExistingObjects()
		for _, object in ipairs(CollectionService:GetTagged(VFX_TAG)) do
			FastSpawn(linkObjectVFX, object)
		end
	end
	
	local function startAllAmbientVFX()
		for _, visualiser in ipairs(AmbientVFX) do
			FastSpawn(visualiser.Start)
		end
	end
	
	-- Collect common modules
	collectModules(CommonModules, script.Common)
	
	-- Hook CollectionService visualisers
	collectModules(ObjectVFX, script.Objects)
	CollectionService:GetInstanceAddedSignal(VFX_TAG):Connect(linkObjectVFX)
	CollectionService:GetInstanceRemovedSignal(VFX_TAG):Connect(unlinkObjectVFX)
	linkExistingObjects()
	
	-- Start ambient visualisers
	collectModules(AmbientVFX, script.Ambient)
	startAllAmbientVFX()
end

init()

Used AeroGameFramework as reference for the init function. I think it should be fine this way otherwise. It doesn’t have to be super pretty and I need to get over this annoying habit of not being productive simply because the code looks a certain way, even though it’s probably good as it is.