OOP Objects not Cleaning Themselves Up?

I am making a script that will be used for droppers in an upcoming tycoon game of mine. I got them to fundamentally work, but I am having an issue with getting the bricks to clean themselves up after a certain amount of time; I am doing this to prevent lag.

In addition, I am using a popular maid module to help with cleanup/memory loss, but I’m not exactly sure that it is needed in this case, or if I am using it correctly.

Basically, with my script, you click a button, and a dropper drops a brick. After 3 or so seconds, the brick should be removed (as well as any OOP references to it), as this will insure there won’t be any sort of “clogging” with bricks staying around too long. It works, but only if you spawn one brick at a time, like seen in this GIF.

However, if you were to spawn multiple bricks at once, only the most recently spawned brick would be deleted after 3 seconds - the rest would stay there, like seen here.

I’m not sure why this is happening, and it is quite annoying. Does anyone have a solution? Here is my code:

-- the following is the code for clicking to spawn bricks, if that is needed.
script.Parent.ClickDetector.MouseClick:Connect(function()
	local dropper = require(game.ServerScriptService.dropperHandler)
	dropper.new(100,BrickColor.new("Bright blue"),2,script.Parent.Parent.spawn.Position,"CoolJohnnyboy",workspace)
end)

--- the following is the actual code that spawns the bricks
local drop = {}
local maid = require(game:GetService("ServerScriptService").Maid)

function drop:Demolish()
    self.Block:Destroy()
        ---the maid class didn't delete the actual block, so I had to add the above line. 
	wait(0.1)
	self._maid:Destroy()
end

function drop.new(value: "how much the brick is worth, in $", 
	color: "brick color value", 
	size: "how many times bigger than the default brick", 
	spawnPos: "attachment on dropper",
	owner: "username of the player who spawned it",
	parent: "where the block will be located")
    ---local self = setmetatable({}, MyClass) 
    --- the above was commented out, since referring to everything as "self" 
    --- here caused issues where I couldn't call the ":Demolish()" function.
	drop._maid = maid.new()
	drop.Block = script.brick:Clone()
	drop.Value = value
	drop.Owner = owner
	local block = drop.Block
	block.Size = block.Size * size
	block.BrickColor = color
	block.Position = spawnPos
	block.Parent = parent
	spawn(function()
		wait(3)
		drop:Demolish()
	end)
end

return drop

Instead of local block = drop.Block try doing self.block = drop.Block and referencing the block with self.block instead

I haven’t really used maid myself, but it doesn’t seem to be connecting to anything. So what is it cleaning up?

Also you should use task.wait() and task.spawn()

local Drop = {}
Drop.__index = Drop

function Drop.new(...)
   local self = setmetatable({}, Drop)

   self._Maid = -- Maid here
   self.Block = --  Block here 

   self._Maid:GiveTask(self.Block) -- Not Actually Sure?   

   task.delay(3, function()
      self:Destroy()
   end)
   return self
end

function Drop:Destroy()
   self.Block:Destroy()

   self._Maid:DoCleaning() -- I believe this is the clean up?
end

return Drop
2 Likes

Couldn’t you just use

game:GetService("Debris"):AddItem(block, 3)

The maid connection connects to a module script that contains the following:


---	Manages the cleaning of events and other things.
-- Useful for encapsulating state and make deconstructors easy
-- @classmod Maid
-- @see Signal

local Maid = {}
Maid.ClassName = "Maid"

--- Returns a new Maid object
-- @constructor Maid.new()
-- @treturn Maid
function Maid.new()
	return setmetatable({
		_tasks = {}
	}, Maid)
end

function Maid.isMaid(value)
	return type(value) == "table" and value.ClassName == "Maid"
end

--- Returns Maid[key] if not part of Maid metatable
-- @return Maid[key] value
function Maid:__index(index)
	if Maid[index] then
		return Maid[index]
	else
		return self._tasks[index]
	end
end

--- Add a task to clean up. Tasks given to a maid will be cleaned when
--  maid[index] is set to a different value.
-- @usage
-- Maid[key] = (function)         Adds a task to perform
-- Maid[key] = (event connection) Manages an event connection
-- Maid[key] = (Maid)             Maids can act as an event connection, allowing a Maid to have other maids to clean up.
-- Maid[key] = (Object)           Maids can cleanup objects with a `Destroy` method
-- Maid[key] = nil                Removes a named task. If the task is an event, it is disconnected. If it is an object,
--                                it is destroyed.
function Maid:__newindex(index, newTask)
	if Maid[index] ~= nil then
		error(("'%s' is reserved"):format(tostring(index)), 2)
	end

	local tasks = self._tasks
	local oldTask = tasks[index]

	if oldTask == newTask then
		return
	end

	tasks[index] = newTask

	if oldTask then
		if type(oldTask) == "function" then
			oldTask()
		elseif typeof(oldTask) == "RBXScriptConnection" then
			oldTask:Disconnect()
		elseif oldTask.Destroy then
			oldTask:Destroy()
		end
	end
end

--- Same as indexing, but uses an incremented number as a key.
-- @param task An item to clean
-- @treturn number taskId
function Maid:GiveTask(task)
	if not task then
		error("Task cannot be false or nil", 2)
	end

	local taskId = #self._tasks+1
	self[taskId] = task

	if type(task) == "table" and (not task.Destroy) then
		warn("[Maid.GiveTask] - Gave table task without .Destroy\n\n" .. debug.traceback())
	end

	return taskId
end

function Maid:GivePromise(promise)
	if not promise:IsPending() then
		return promise
	end

	local newPromise = promise.resolved(promise)
	local id = self:GiveTask(newPromise)

	-- Ensure GC
	newPromise:Finally(function()
		self[id] = nil
	end)

	return newPromise
end

--- Cleans up all tasks.
-- @alias Destroy
function Maid:DoCleaning()
	local tasks = self._tasks

	-- Disconnect all events first as we know this is safe
	for index, task in pairs(tasks) do
		if typeof(task) == "RBXScriptConnection" then
			tasks[index] = nil
			task:Disconnect()
		end
	end

	-- Clear out tasks table completely, even if clean up tasks add more tasks to the maid
	local index, task = next(tasks)
	while task ~= nil do
		tasks[index] = nil
		if type(task) == "function" then
			task()
		elseif typeof(task) == "RBXScriptConnection" then
			task:Disconnect()
		elseif task.Destroy then
			task:Destroy()
		end
		index, task = next(tasks)
	end
end

--- Alias for DoCleaning()
-- @function Destroy
Maid.Destroy = Maid.DoCleaning

return Maid

If you click the first hyperlink I had in my original post, it takes you to the documentation and code for it.

@Kaid3n22
That doesn’t solve the issue that the maid disconnecting and deleting any memory regarding the block is only done for the last generated block.

After reading the documentation, your code simply does nothing with the maid, you create a new Maid and then clean the maid, though it has nothing to clean up since no tasks were given to it. I edited my old reply hopefully that should help.

I don’t really see a need to use a maid here, it just seems like an unnecessary addition of complexity.

Simply destroying the brick after a duration is by far the easier implementation here. The actual object will be disposed of automatically, as soon as it is no longer referenced anywhere. And on that note, detaching any reference to it is easy to do as well without really requiring a maid. (for the case you’re storing these objects in a table cache.)

What about adding more events, such as when the block is touched? Would a maid be necessary then?

Yep, that’s what clean up modules help with. Like if the drop had a .Touched connection and you wanted to clean that up you would assign that connection to the cleanup module and then once your done it would clean it up.

You still don’t need a maid for that specific connection. I’m not saying you shouldn’t or can’t use a maid, but to me given what you’re using it for it just seems unnecessary to me.

Specifically, the Touched event is a member of the actual part instance. So that said, when Destroy() is called on the part, that event listener is cleaned up for you automatically. It doesn’t linger on in memory.

You forgot to make your class work as metatable (that is drop.__index = drop). That was the cause of not being able to call :Demolish() in the constructor.
it is necessary to use self, because it represents the object itself that has just been created and must always be returned by the constructor (otherwise it ceases to be a constructor).
With these corrections it should work.

local drop = {}
drop.__index = drop

local maid = require(game:GetService("ServerScriptService").Maid)

function drop:Demolish()
    self.Block:Destroy()
    ---the maid class didn't delete the actual block, so I had to add the above line. 
    wait(0.1)
    self._maid:Destroy()
end

function drop.new(value: "how much the brick is worth, in $", 
    color: "brick color value", 
    size: "how many times bigger than the default brick", 
    spawnPos: "attachment on dropper",
    owner: "username of the player who spawned it",
    parent: "where the block will be located")
    local self = setmetatable({}, drop) 
    self._maid = maid.new()
    self.Block = script.brick:Clone()
    self.Value = value
    self.Owner = owner
    local block = self.Block
    block.Size = block.Size * size
    block.BrickColor = color
    block.Position = spawnPos
    block.Parent = parent
    spawn(function()
        wait(3)
        self:Demolish()
    end)
    return self
end

return drop

As for Maid there is no problem in using it that way. Although personally I don’t use it when I do OOP as it is the destructors that are meant to do that job.