Disconnect a local function?

NinjoOnline · October 9, 2020, 7:38am

Title says it all. My problem is, I have a module script, that gets required when I enter a car and deleted when I leave said car. And then that keeps repeating. However, if I enter, leave, enter, leave, continously, it stacks all these connections together.

As you can see, I enter the car, and have the remote event fire 3 times (once for each action)

Then I continously leave and re-enter the car, then when I press these keys again, I get a dozen prints for each action.

local function Create(player, vehicle)
	local Character = player.Character
	if not Character then return end
		
	-- Find the model to clone
	local VehicleModel = Vehicles:FindFirstChild(vehicle)
	if not VehicleModel then return end
	
	local VehicleClone = VehicleModel:Clone()
	VehicleClone.Name = player.Name
	VehicleClone:SetPrimaryPartCFrame(Character.PrimaryPart.CFrame * CFrame.new(Vector3.new(0, 0, -VehicleModel.PrimaryPart.Size.Z)))
	
	VehicleClone.Parent = VehicleHolder
	
	-- Create server script
	local CarHandlerClone = CarHandler:Clone()
	CarHandlerClone.Parent = VehicleClone
	require(CarHandlerClone)
end

-- CarHandler module (not all of the code)
local CarHandler = {}

local function Control(player, control)
	if not Seat.Occupant then return end -- Make sure someone is sitting in the car
	
	if control == 'Lights' then
		print("TURN ON LIGHTS")
	elseif control == 'Flip' then
		print("FLIP THE CAR")
	end
end

VehicleControls.OnServerEvent:Connect(Control)

return CarHandler

So how can I stop the previous modules code from running? I only want the most up to date required module to actually run.

Elocore · October 9, 2020, 8:02am

Disconnect the event when it’s no longer needed. First assign the event to a variable like so

local event = VehicleControls.OnServerEvent:Connect(Control)

When you don’t want the event to fire any more then call the Disconnect() function

event:Disconnect()

NinjoOnline · October 9, 2020, 8:21am

I need to disconnect it when the module is deleted tho? And there’s no way to detect from the script, that it’s been deleted

Scarious · October 9, 2020, 8:24am

Deleting a ModuleScript does nothing if you have already required it. What you can do is add a method inside the ModuleScript which disconnects everything, and then call it when you delete it. The required thread of the ModuleScript runs wherever you call the ModuleScript from, rather than in the ModuleScript.

Think of it as you just deleted the Class of an Object, but didn’t delete the Object… if you understand Object Oriented Programming this will make sense… if not, ignore it.

Oh, did I mention… ModuleScript?

NinjoOnline · October 9, 2020, 8:26am

I don’t know what you mean by ‘adding a method’ which disconnects everything

Scarious · October 9, 2020, 8:29am

It means you add a method to the ModuleScript code, (a method in lua is a function, so think adding a function), which handles the deletion of the methods… So for instance:

-- CarHandler module (not all of the code)
local CarHandler = {}

local function Control(player, control)
	if not Seat.Occupant then return end -- Make sure someone is sitting in the car
	
	if control == 'Lights' then
		print("TURN ON LIGHTS")
	elseif control == 'Flip' then
		print("FLIP THE CAR")
	end
end

local serverEvent = VehicleControls.OnServerEvent:Connect(Control)

function CarHandler.disconnect()
	serverEvent:disconnect()
end

return CarHandler

You disconnect all your events/etc by calling CarHandler.disconnect()

starmaq · October 9, 2020, 8:45am

This is kind of bad design. Requiring a copy of a module just for the sake of running some code each time isn’t a perfect idea. I noticed that the CarHandler module is responsible for a lot of stuff concerning the car it’s parented to, but I don’t think that’s a bright idea either. Just have one module, and a script under the car which manages things using the module’s functions. Might even take it one more step and make one script handle all cars in the game. To simplify what’s going on, I suggest you connect the event inside of the Control function, perhaps make it return the connection, and hand the duty of disconnecting to yourself.

local function Control(player, control)
	if not Seat.Occupant then return end -- Make sure someone is sitting in the car
	
	if control == 'Lights' then
		print("TURN ON LIGHTS")
	elseif control == 'Flip' then
		print("FLIP THE CAR")
	end
    return VehicleControls.OnServerEvent:Connect(Control)
end

return CarHandler

local connection
local function Create(player, vehicle)
    if connection then 
       connection:Disconnect()
    end
	local Character = player.Character
	if not Character then return end
		
	-- Find the model to clone
	local VehicleModel = Vehicles:FindFirstChild(vehicle)
	if not VehicleModel then return end
	
	local VehicleClone = VehicleModel:Clone()
	VehicleClone.Name = player.Name
	VehicleClone:SetPrimaryPartCFrame(Character.PrimaryPart.CFrame * CFrame.new(Vector3.new(0, 0, -VehicleModel.PrimaryPart.Size.Z)))
	
	VehicleClone.Parent = VehicleHolder
	
	connection = CarHandler.Control(something, something)
end

ocula · October 9, 2020, 8:47am

How exactly is your ModuleScript written?

Tables can have metamethods or functions, and a simple way of explaining ModuleScripts is by describing them as tables. @Scarious Wrote a good example of how you can use a table’s properties to further enhance your ModuleScript to disconnect all of your connected events.

However, I want to point out that “requiring and deleting” a ModuleScript sounds to me like a misuse of the functionality of ModuleScripts. You should, in theory, have your system code set up to require pseudo code from your ModuleScripts to then use in specific use cases.

Roughly put: In creating a new car, you could require a Car Class (a modulescript), which would include all the functionality of the car within the class itself, and then run any kind of attached .new() or create method to create the car and attach all necessary connections to the car. Then you’d attach a :Destroy() or :Disconnect() function to the car class and be able to destroy the car or disconnect all events from the car. No destroying/deleting of any ModuleScripts would need to happen here if all of your modulescripts are written with modularity in mind.

In conclusion: you shouldn’t be needing to delete a ModuleScript unless it’s being cloned under a commanding script or localscript. If that’s not the case, you may save yourself a lot of future hassle by renetworking your current system. This would simplify it so that you have less of a chance of running into issues like this!

NinjoOnline · October 9, 2020, 10:22am

How can I have it controlled all from one module without needing to require a cloned module each time? The main reason I did this was:

I only want 1 server script in the entire game
Duplicate scripts = bad

And I’m not sure how I could control all the cars with one module that is just required on start and runs the entire time.

This is the full module

local CarHandler = {}

local PhysicsService = game:GetService('PhysicsService')
local Players = game:GetService('Players')
local ReplicatedStorage = game:GetService('ReplicatedStorage')

local Remotes = ReplicatedStorage:WaitForChild('Remotes')
local Events = Remotes:WaitForChild('Events')

local PlaySound = Events:WaitForChild('PlaySound')
local StopSound = Events:WaitForChild('StopSound')

local VehicleEvents = Events:WaitForChild('VehicleEvents')
local EnterVehicle = VehicleEvents:WaitForChild('EnterVehicle')
local VehicleControls = VehicleEvents:WaitForChild('VehicleControls')

local DefaultCollisionGroup = 'Players' -- Revert back to the collision group set by CollisionManager
local CharacterCollisionGroup = 'Character'

local Car = script.Parent

local CarClient = script:WaitForChild('CarClient')

local Body = Car:WaitForChild('Body')
local Seat = Body:WaitForChild('VehicleSeat')

local OccupiedPlayer = nil
local OccupiedClientScript

local function PlaySoundEffect(player, soundEffect)
	local SoundEffect = Car:FindFirstChild(soundEffect)
	if not SoundEffect then return end
	
	if SoundEffect.IsPlaying then return end -- Don't continue if already playing
	
	SoundEffect:Play()
end

local function StopSoundEffect(player, soundEffect)
	local SoundEffect = Car:FindFirstChild(soundEffect)
	if not SoundEffect then return end
	
	if not SoundEffect.IsPlaying then return end -- Don't continue if it's not playing
	
	SoundEffect:Stop()
end

local function SetCharacterCollide(character, shouldCollide)
	local Group = shouldCollide and DefaultCollisionGroup or CharacterCollisionGroup
	
	for _, part in ipairs(character:GetDescendants()) do
		if part:IsA('BasePart') then
			part.Massless = not shouldCollide
			PhysicsService:SetPartCollisionGroup(part, Group)
		end
	end
end

local function Enter(player, car)
	if car ~= Car then return end -- Make sure it's this car we are entering
	
	if Seat.Occupant then return end -- Don't continue if someone is already sitting
	
	local Character = player.Character
	if not Character then return end
	
	local Humanoid = Character:FindFirstChildWhichIsA('Humanoid')
	if not Humanoid then return end
	
	Seat:Sit(Humanoid)
	OccupiedPlayer = player
	
	SetCharacterCollide(Character, false) -- Disable collisions on car
	Car.PrimaryPart:SetNetworkOwner(player)
	
	-- Give player the client script
	OccupiedClientScript = CarClient:Clone()
	OccupiedClientScript.Car.Value = Car
	OccupiedClientScript.Parent = player.Backpack
end

local function OccupantChanged()
	if Seat.Occupant then return end
	
	-- Leaving the car	
	if OccupiedClientScript.Parent then
		-- Stop the client script running
		OccupiedClientScript.Stop.Value = true
		
		local Client = OccupiedClientScript
		
		delay(3, function()
			Client:Destroy()
		end)
	end
	
	if Car and Car.Parent then
		-- Reset NetworkOwner
		Car.PrimaryPart:SetNetworkOwnershipAuto()
	end
	
	local OccupiedCharacter = OccupiedPlayer.Character
	
	OccupiedPlayer = nil
	OccupiedClientScript = nil
	
	wait()
	
	if OccupiedCharacter then
		if Car.PrimaryPart then
			-- Teleport player to side
			OccupiedCharacter:SetPrimaryPartCFrame(Car.PrimaryPart.CFrame * CFrame.new(Vector3.new(-10, 0, 0)))
		end
		
		-- Allow collisions again
		SetCharacterCollide(OccupiedCharacter, true)
	end
		
	OccupiedCharacter = nil
end

local function Control(player, control)
	print(1)
	if not Seat.Occupant then return end
	
	if control == 'Lights' then
		for _, v in pairs(Body:GetChildren()) do
			if v.Name == 'Headlight' then
				v.Material = v.Material == Enum.Material.SmoothPlastic and Enum.Material.Neon or Enum.Material.SmoothPlastic
				v.SpotLight.Enabled = not v.SpotLight.Enabled
			end
		end		
	elseif control == 'Horn' then
		PlaySoundEffect(player, 'Horn')
	elseif control == 'Flip' then
		local BodyGyro = Car.Platform:FindFirstChild('BodyGyro')
		if not BodyGyro then return end
		
		BodyGyro.MaxTorque = Vector3.new(1000, 0, 1000)
		
		delay(2, function()
			BodyGyro.MaxTorque = Vector3.new(0, 0, 0)
		end)
	end
end

Seat:GetPropertyChangedSignal('Occupant'):Connect(OccupantChanged)

PlaySound.OnServerEvent:Connect(PlaySoundEffect)
StopSound.OnServerEvent:Connect(StopSoundEffect)

EnterVehicle.OnServerEvent:Connect(Enter)
VehicleControls.OnServerEvent:Connect(Control)

return CarHandler

BanTech · October 9, 2020, 11:44am

Either by making it object-oriented, or by adding a method to assign the vehicle model of the current car,

CarHandler.AssignModel( yourCarModel )

It looks like the modulescript is really generic and would apply to every car you have. By cloning the module and deleting it you remove most of the benefits of using a module.

When you assign the model, hook up the connections to stuff like the seat changed event. And when you destroy the model, disconnect these. Ideally your cars shouldn’t need to have any modules or scripts inside of them at all.

ocula · October 9, 2020, 11:47am

This isn’t modular code. This is the equivalent of cloning and inserting a Script into the Car object. If you would like to look into the differences between ModuleScripts and Scripts, see here. Now lets solve your problem!

Because of the fact that this code is not modular, you shouldn’t be executing it inside of a ModuleScript. It should be inside of a Script. It’s a good move to translate this code into a module in the future, but to solve your problem right now: Move this code into a Script instead of a ModuleScript.

Great! So we have our code in a Script that we clone and put into the car! However, deleting the script won’t destroy any of the RemoteEvent connections or any other connections for that matter– aka, the problem here is still not solved.

Here’s my simple fix:

Paste your current code into a Script. Delete local CarHandler = {} and return CarHandler as we are no longer writing a ModuleScript.
For every single one of your Connection events, store them in a table. The simplest way of doing this is to give variables to every one of your connections, then wrapping those variables into a table:

	local thisIsMyEvent = PlaySound.OnServerEvent:Connect(PlaySoundEffect)
	local heresAnotherEvent = EnterVehicle.OnServerEvent:Connect(Enter)
	-- ... etc etc, and now we wrap them all in a table:
	local Connections = {thisIsMyEvent, heresAnotherEvent};

Now write a function inside of the script that iterates through our Connections and Disconnects them:

	function DisconnectEvents()
		for i,v in pairs(Connections) do
			v:Disconnect()
		end
	end

Now it’s time to decide when/how we’re going to run this function. There are multiple ways. For example, you could have the script listen to a BoolValue’s .Changed event inside of the Car object, and if it’s set to true will run our DisconnectEvents() function. (There’s a maddening stigma against using Value objects, but I say go for it if that works for you!). But what I’m going to do here is edit the inside of your “OccupantChanged” function, as that seems to dictate when the player gets out of the seat:

	if OccupiedClientScript.Parent then
		-- Stop the client script running
		OccupiedClientScript.Stop.Value = true
		
		local Client = OccupiedClientScript
		
		delay(3, function()
			Client:Destroy()
		end)

		DisconnectEvents()
	end

That should work as a temporary fix. I highly highly highly suggest looking into making this code modular. It will save you time and expense in the future.

Good luck!

NinjoOnline · October 9, 2020, 1:30pm

I’m trying to maintain a singular server/client script structure in my game atm tho.

I haven’t tested this, but is that somewhat the right direction??

local CarHandler = {}

function CarHandler.new(car)		
	local Body = car:WaitForChild('Body')
	local Seat = Body:WaitForChild('VehicleSeat')
	
	local OccupiedPlayer = nil
	local OccupiedClientScript
	
	local function PlaySoundEffect(player, soundEffect)
		local SoundEffect = Car:FindFirstChild(soundEffect)
		if not SoundEffect then return end
		
		if SoundEffect.IsPlaying then return end -- Don't continue if already playing
		
		SoundEffect:Play()
	end
	
	local function StopSoundEffect(player, soundEffect)
		local SoundEffect = Car:FindFirstChild(soundEffect)
		if not SoundEffect then return end
		
		if not SoundEffect.IsPlaying then return end -- Don't continue if it's not playing
		
		SoundEffect:Stop()
	end
	
	local function SetCharacterCollide(character, shouldCollide)
		local Group = shouldCollide and DefaultCollisionGroup or CharacterCollisionGroup
		
		for _, part in ipairs(character:GetDescendants()) do
			if part:IsA('BasePart') then
				part.Massless = not shouldCollide
				PhysicsService:SetPartCollisionGroup(part, Group)
			end
		end
	end
	
	local function Enter(player, car)
		if car ~= Car then return end -- Make sure it's this car we are entering
		
		if Seat.Occupant then return end -- Don't continue if someone is already sitting
		
		local Character = player.Character
		if not Character then return end
		
		local Humanoid = Character:FindFirstChildWhichIsA('Humanoid')
		if not Humanoid then return end
		
		Seat:Sit(Humanoid)
		OccupiedPlayer = player
		
		SetCharacterCollide(Character, false) -- Disable collisions on car
		Car.PrimaryPart:SetNetworkOwner(player)
		
		-- Give player the client script
		OccupiedClientScript = CarClient:Clone()
		OccupiedClientScript.Car.Value = Car
		OccupiedClientScript.Parent = player.Backpack
	end
	
	local function OccupantChanged()
		if Seat.Occupant then return end
		
		-- Leaving the car	
		if OccupiedClientScript.Parent then
			-- Stop the client script running
			OccupiedClientScript.Stop.Value = true
			
			local Client = OccupiedClientScript
			
			delay(3, function()
				Client:Destroy()
			end)
		end
		
		if Car and Car.Parent then
			-- Reset NetworkOwner
			Car.PrimaryPart:SetNetworkOwnershipAuto()
		end
		
		local OccupiedCharacter = OccupiedPlayer.Character
		
		OccupiedPlayer = nil
		OccupiedClientScript = nil
		
		wait()
		
		if OccupiedCharacter then
			if Car.PrimaryPart then
				-- Teleport player to side
				OccupiedCharacter:SetPrimaryPartCFrame(Car.PrimaryPart.CFrame * CFrame.new(Vector3.new(-10, 0, 0)))
			end
			
			-- Allow collisions again
			SetCharacterCollide(OccupiedCharacter, true)
		end
		
		OccupiedCharacter = nil
	end
	
	local function Control(player, control)
		print(1)
		if not Seat.Occupant then return end
		
		if control == 'Lights' then
			for _, v in pairs(Body:GetChildren()) do
				if v.Name == 'Headlight' then
					v.Material = v.Material == Enum.Material.SmoothPlastic and Enum.Material.Neon or Enum.Material.SmoothPlastic
					v.SpotLight.Enabled = not v.SpotLight.Enabled
				end
			end		
		elseif control == 'Horn' then
			PlaySoundEffect(player, 'Horn')
		elseif control == 'Flip' then
			local BodyGyro = Car.Platform:FindFirstChild('BodyGyro')
			if not BodyGyro then return end
			
			BodyGyro.MaxTorque = Vector3.new(1000, 0, 1000)
			
			delay(2, function()
				BodyGyro.MaxTorque = Vector3.new(0, 0, 0)
			end)
		end
	end
	
	Seat:GetPropertyChangedSignal('Occupant'):Connect(OccupantChanged)
	
	PlaySound.OnServerEvent:Connect(PlaySoundEffect)
	StopSound.OnServerEvent:Connect(StopSoundEffect)
	
	EnterVehicle.OnServerEvent:Connect(Enter)
	VehicleControls.OnServerEvent:Connect(Control)
end

return CarHandler

return CarHandler

And so in the other script, instead of cloning and requiring, all I do is pass the car clone?

CarHandler.new(VehicleClone)

yungpharma · October 9, 2020, 2:41pm

That’s not object oriented yet, I’d recommend using @Quenty’s Maid and a method like this:

local Maid = require(Path.To.Maid)

local Car = {}
Car.__index = Car

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

	self.body = car:WaitForChild("Body")
	self.seat = self.body:WaitForChild("VehicleSeat")

	self._maid = Maid.new()
	

	local function PlaySoundEffect(...) self:PlaySoundEffect(...) end
	local function StopSoundEffect(...) self:StopSoundEffect(...) end
	local function Enter(...) self:Enter(...) end
	local function OccupantChanged(...) self:OccupantChanged(...) end
	local function Control(...) self:Control(...) end

	self._maid:GiveTask(PlaySound.OnServerEvent:Connect(PlaySoundEffect))
	self._maid:GiveTask(StopSound.OnServerEvent:Connect(StopSoundEffect))
	self._maid:GiveTask(EnterVehicle.OnServerEvent:Connect(Enter))
	self._maid:GiveTask(self.seat:GetPropertyChangedSignal('Occupant'):Connect(OccupantChanged))
	self._maid:GiveTask(Control.OnServerEvent:Connect(Control))

	return self
end

function Car:Enter(player)
	--if self.car ~= Car then return end -- if you compare car with the car class, you're pretty much getting an false statement, so it will never pass this. you can't compare tables in lua.
		
	if self.seat.Occupant then return end -- Don't continue if someone is already sitting
		
	local Character = player.Character
	if not Character then return end
		
	local Humanoid = Character:FindFirstChildWhichIsA('Humanoid')
	if not Humanoid then return end
		
	self.seat:Sit(Humanoid)
	self.OccupiedPlayer = player
		
	self:SetCharacterCollide(Character, false) -- Disable collisions on car
	self.car.PrimaryPart:SetNetworkOwner(player)
		
	-- Give player the client script
	self.OccupiedClientScript = CarClient:Clone()
	self.OccupiedClientScript.Car.Value = Car
	self.OccupiedClientScript.Parent = player.Backpack
end

local function Car:PlaySoundEffect(player, soundEffect)
	local SoundEffect = self.car:FindFirstChild(soundEffect)
	if not SoundEffect then return end
		
	if SoundEffect.IsPlaying then return end -- Don't continue if already playing
		
	SoundEffect:Play()
end
	
local function Car:StopSoundEffect(player, soundEffect)
	local SoundEffect = self.car:FindFirstChild(soundEffect)
	if not SoundEffect then return end
		
	if not SoundEffect.IsPlaying then return end -- Don't continue if it's not playing
		
	SoundEffect:Stop()
end
	
local function Car:SetCharacterCollide(character, shouldCollide)
	local Group = shouldCollide and DefaultCollisionGroup or CharacterCollisionGroup
		
	for _, part in ipairs(character:GetDescendants()) do
		if part:IsA('BasePart') then
			part.Massless = not shouldCollide
			PhysicsService:SetPartCollisionGroup(part, Group)
		end
	end
end
	
local function Car:OccupantChanged()
	if self.seat.Occupant then return end
		
	-- Leaving the car	
	if self.OccupiedClientScript.Parent then
	-- Stop the client script running
	OccupiedClientScript.Stop.Value = true
			
	local Client = self.OccupiedClientScript
			
	delay(3, function()
		Client:Destroy()
	end)
		
	if self.car and self.car.Parent then
		-- Reset NetworkOwner
		self.car.PrimaryPart:SetNetworkOwnershipAuto()
	end
		
	self.OccupiedCharacter = self.OccupiedPlayer.Character
		
	self.OccupiedPlayer = nil
	self.OccupiedClientScript = nil
		
	wait()
		
	if self.OccupiedCharacter then
		if self.car.PrimaryPart then
			-- Teleport player to side
			self.OccupiedCharacter:SetPrimaryPartCFrame(self.car.PrimaryPart.CFrame * CFrame.new(Vector3.new(-10, 0, 0)))

			-- Allow collisions again
		end

	 	 self:SetCharacterCollide(self.OccupiedCharacter, true)
	 end
		
	self.OccupiedCharacter = nil
end
	
function Car:Control(player, control)
	print(1)
	if not self.seat.Occupant then return end
		
	if control == 'Lights' then
		for _, v in pairs(self.body:GetChildren()) do
			if v.Name == 'Headlight' then
				v.Material = v.Material == Enum.Material.SmoothPlastic and Enum.Material.Neon or Enum.Material.SmoothPlastic
				v.SpotLight.Enabled = not v.SpotLight.Enabled
			end
		end		
	elseif control == 'Horn' then
		self:PlaySoundEffect(player, 'Horn')
	elseif control == 'Flip' then
		local BodyGyro = self.car.Platform:FindFirstChild('BodyGyro')
		if not BodyGyro then return end
			
		BodyGyro.MaxTorque = Vector3.new(1000, 0, 1000)
			
		delay(2, function()
			BodyGyro.MaxTorque = Vector3.new(0, 0, 0)
		end)
	end
end

function Car:Destroy() -- you only need to call this if something goes wrong, don't create a new instance of car every single time someone hops in
	self._maid:DoCleaning()
	self._car:Destroy()
end
	
return Car

TestyLike3 · October 9, 2020, 8:13pm

Is debounce not possible?
If not, you could try to make code where it deletes any previous modules right before it starts.

NinjoOnline · October 10, 2020, 12:43am

I wasn’t really going for OO as I feel it’s too advanced for what I need

NinjoOnline · October 10, 2020, 3:02am

Upon further testing with this, I still get a multitude of problems. Main one being if I spawn a car, then spawn another car, I get this error
attempt to index nil with ‘SetNetworkOwner’

with this piece of code

function Vehicle:Enter(player)	
	if self.seat.Occupant then return end -- Don't continue if someone is already sitting
	
	local Character = player.Character
	if not Character then return end
	
	local Humanoid = Character:FindFirstChildWhichIsA('Humanoid')
	if not Humanoid then return end
	
	self.seat:Sit(Humanoid)
	self.occupiedPlayer = player
	
	self:SetCharacterCollide(Character, false) -- Disable collisions on car
	self.vehicle.PrimaryPart:SetNetworkOwner(player) -- ERROR HERE
	
	-- Give player the client script
	self.occupiedClientScript = CarClient:Clone()
	self.occupiedClientScript.Car.Value = self.vehicle
	self.occupiedClientScript.Parent = player.Backpack
end

local Vehicle = {}
Vehicle.__index = Vehicle

local Maid = require(game.ReplicatedStorage.SharedModules.Maid)

local PhysicsService = game:GetService('PhysicsService')
local Players = game:GetService('Players')
local ReplicatedStorage = game:GetService('ReplicatedStorage')

local CarClient = script:WaitForChild('CarClient')

local Remotes = ReplicatedStorage:WaitForChild('Remotes')
local Events = Remotes:WaitForChild('Events')

local PlaySound = Events:WaitForChild('PlaySound')
local StopSound = Events:WaitForChild('StopSound')

local VehicleEvents = Events:WaitForChild('VehicleEvents')
local EnterVehicle = VehicleEvents:WaitForChild('EnterVehicle')
local VehicleControls = VehicleEvents:WaitForChild('VehicleControls')

local DefaultCollisionGroup = 'Players' -- Revert back to the collision group set by CollisionManager
local CharacterCollisionGroup = 'Character'

function Vehicle.new(vehicle)		
	local self = setmetatable({}, Vehicle)
	
	self.vehicle = vehicle
	self.body = vehicle:WaitForChild('Body')
	self.seat = self.body:WaitForChild('VehicleSeat')
	
	self._maid = Maid.new()
	
	local function PlaySoundEffect(...) self:PlaySoundEffect(...) end
	local function StopSoundEffect(...) self:StopSoundEffect(...) end
	local function Enter(...) self:Enter(...) end
	local function OccupantChanged(...) self:OccupantChanged(...) end
	local function Control(...) self:Control(...) end
	
	self._maid:GiveTask(PlaySound.OnServerEvent:Connect(PlaySoundEffect))
	self._maid:GiveTask(StopSound.OnServerEvent:Connect(StopSoundEffect))
	
	self._maid:GiveTask(self.seat:GetPropertyChangedSignal('Occupant'):Connect(OccupantChanged))
	
	self._maid:GiveTask(EnterVehicle.OnServerEvent:Connect(Enter))
	self._maid:GiveTask(VehicleControls.OnServerEvent:Connect(Control))
	
	return self
end

function Vehicle:Enter(player)	
	if self.seat.Occupant then return end -- Don't continue if someone is already sitting
	
	local Character = player.Character
	if not Character then return end
	
	local Humanoid = Character:FindFirstChildWhichIsA('Humanoid')
	if not Humanoid then return end
	
	self.seat:Sit(Humanoid)
	self.occupiedPlayer = player
	
	self:SetCharacterCollide(Character, false) -- Disable collisions on car
	self.vehicle.PrimaryPart:SetNetworkOwner(player)
	
	-- Give player the client script
	self.occupiedClientScript = CarClient:Clone()
	self.occupiedClientScript.Car.Value = self.vehicle
	self.occupiedClientScript.Parent = player.Backpack
end

function Vehicle:PlaySoundEffect(player, soundEffect)
	local SoundEffect = self.vehicle:FindFirstChild(soundEffect)
	if not SoundEffect then return end
	
	if SoundEffect.IsPlaying then return end -- Don't continue if already playing
	
	SoundEffect:Play()
end

function Vehicle:StopSoundEffect(player, soundEffect)
	local SoundEffect = self.vehicle:FindFirstChild(soundEffect)
	if not SoundEffect then return end
	
	if not SoundEffect.IsPlaying then return end -- Don't continue if it's not playing
	
	SoundEffect:Stop()
end

function Vehicle:SetCharacterCollide(character, shouldCollide)
	local Group = shouldCollide and DefaultCollisionGroup or CharacterCollisionGroup
	
	for _, part in ipairs(character:GetDescendants()) do
		if part:IsA('BasePart') then
			part.Massless = not shouldCollide
			PhysicsService:SetPartCollisionGroup(part, Group)
		end
	end
end

function Vehicle:OccupantChanged()
	if self.seat.Occupant then return end

	-- Leaving the car	
	if self.occupiedClientScript.Parent then
		-- Stop the client script running
		self.occupiedClientScript.Stop.Value = true
		
		local Client = self.occupiedClientScript
		
		delay(3, function()
			Client:Destroy()
		end)
		
		if self.vehicle and self.vehicle.Parent then
			-- Reset NetworkOwner
			self.vehicle.PrimaryPart:SetNetworkOwnershipAuto()
		end
		
		self.occupiedCharacter = self.occupiedPlayer.Character
		
		self.occupiedPlayer = nil
		self.occupiedClientScript = nil
		
		wait()
		
		if self.occupiedCharacter then
			if self.vehicle.PrimaryPart then
				-- Teleport player to side
				self.occupiedCharacter:SetPrimaryPartCFrame(self.vehicle.PrimaryPart.CFrame * CFrame.new(Vector3.new(-10, 0, 0)))
			end
			
			-- Allow collisions again
			self:SetCharacterCollide(self.occupiedCharacter, true)
		end
		
		self.occupiedCharacter = nil
	end
end

function Vehicle:Control(player, control)
	print(1)
	if not self.seat.Occupant then return end
	
	if control == 'Lights' then
		for _, v in pairs(self.body:GetChildren()) do
			if v.Name == 'Headlight' then
				v.Material = v.Material == Enum.Material.SmoothPlastic and Enum.Material.Neon or Enum.Material.SmoothPlastic
				v.SpotLight.Enabled = not v.SpotLight.Enabled
			end
		end		
	elseif control == 'Horn' then
		self:PlaySoundEffect(player, 'Horn')
	elseif control == 'Flip' then
		local BodyGyro = self.vehicle.Platform:FindFirstChild('BodyGyro')
		if not BodyGyro then return end
		
		BodyGyro.MaxTorque = Vector3.new(1000, 0, 1000)
		
		delay(2, function()
			BodyGyro.MaxTorque = Vector3.new(0, 0, 0)
		end)
	end
end

function Vehicle:Destroy() -- you only need to call this if something goes wrong, don't create a new instance of car every single time someone hops in
	self._maid:DoCleaning()
	self._vehicle:Destroy()
end

return Vehicle

yungpharma · October 10, 2020, 4:59am

Just replace vehicle with car, I used your code.

NinjoOnline · October 10, 2020, 5:01am

No, cause I changed all ‘Car’ reference to vehicle. It’s obviously not deleting the car when I remove it. As it works great with first vehicle, but second and on I get that error (car still drives)

So it’s obviously still referencing the vehicle that’s been deleted, which is why I didn’t wanna use a single module before. EDIT It’s definately still referencing cars that have been destroyed. If I spam click the spawn button and have 100 cars load in, then enter the car, my output gets spammed with that error. So it’s obviously still referencing all these cars, that have since been destroyed

All I do is destroy the vehicle model, not sure if I need to do something inside the OO module, like Vehicle:Destroy() (even tho you say don’t use?)

local function Create(player, vehicle)
	local Character = player.Character
	if not Character then return end
	
	VehicleManager.Remove(player) -- Remove the previous vehicle
	
	-- Find the model to clone
	local VehicleModel = Vehicles:FindFirstChild(vehicle)
	if not VehicleModel then return end
	
	local VehicleClone = VehicleModel:Clone()
	VehicleClone.Name = player.Name
	VehicleClone:SetPrimaryPartCFrame(Character.PrimaryPart.CFrame * CFrame.new(Vector3.new(0, 0, -VehicleModel.PrimaryPart.Size.Z)))
	
	VehicleClone.Parent = VehicleHolder
	
	-- New vehicle construstor
	Vehicle.new(VehicleClone)
end

function VehicleManager.Remove(player)
	local PlayersVehicle = VehicleHolder:FindFirstChild(player.Name)
	if PlayersVehicle then
		-- Destroy their current vehicle
		PlayersVehicle:Destroy()
	end
end

RoBoPoJu · October 10, 2020, 9:44am

Yes, you should use vehicle:Destroy(). You can store the vehicles to a table with either the players or their names as the keys. Here’s an edited version of your script.

local vehicles = {}

local function Create(player, vehicle)
	local Character = player.Character
	if not Character then return end
	
	VehicleManager.Remove(player) -- Remove the previous vehicle
	
	-- Find the model to clone
	local VehicleModel = Vehicles:FindFirstChild(vehicle)
	if not VehicleModel then return end
	
	local VehicleClone = VehicleModel:Clone()
	VehicleClone.Name = player.Name
	VehicleClone:SetPrimaryPartCFrame(Character.PrimaryPart.CFrame * CFrame.new(Vector3.new(0, 0, -VehicleModel.PrimaryPart.Size.Z)))
	
	VehicleClone.Parent = VehicleHolder
	
	-- New vehicle construstor
	vehicles[player] = Vehicle.new(VehicleClone)
end

function VehicleManager.Remove(player)
	local PlayersVehicle = vehicles[player]
	if PlayersVehicle then
		-- Destroy their current vehicle
		PlayersVehicle:Destroy()
	end
end

NinjoOnline · October 10, 2020, 12:02pm

They told me not to use the :Destroy() tho?

function Vehicle:Destroy() -- you only need to call this if something goes wrong, don't create a new instance of car every single time someone hops in
	self._maid:DoCleaning()
	self._vehicle:Destroy()
end