How clean is my placement module?

Help and Feedback Code Review
,
#1

Hello! I am going to write my third placement module, but want to see what’s wrong, so I can improve the code overall, and not just add more functionality. One thing I am doing already with my new one is it will be a more object oriented script, so no need to point that out. Here it is:

-- The API

--[[
		MODULE:new(string name, obj model location, obj plot, obj placed objs location, bool stackable objs, bool rot type)
		MODULE:Finish(obj Remote event)
--]]

-- Settings

-- Grid

local MoveByGrid = true -- If you wan't the model to move by grid specified units.
local GridSize = 2 -- In studs.
local DisplayGrid = true -- If you wan't to display a grid.
local SmartDisplay = true -- If you want the module to try and autoscale the texture to match your grid (may alter resolution of texture).
local GridTexture = "rbxassetid://2415319308" -- Texture used when grid is displayed.

 -- Linear Interpolation (smoothing)

local Interpolate = true -- Enable smoothing (Interpolation)
local InterpolateSpeed = 0.7 -- Speed of movement (0 = instant, 1 = no movement. Defualt = 0.7)

-- Basic Settings - Start

local RotationStep = 90 -- In degrees.
local FloorStep = 10 -- In studs.
local MaxHeight = 30 -- In studs.
local CollisionCheckCooldown = 0.3 -- In seconds

-- Bools (true or false values)

local DetectCollisions = true -- If you wan't to have a collision system.
local IgnoreItems = true -- If you want the mouse to ignore the items currently placed down.
local BuildModeEnabled = true -- If you want continual placement.
local EnableFloors = true -- If you want a floor system.
local TransparentModels = true -- if you want the models to be transparent while placing

-- Basic Settings - End

-- Advanced Settings - Start

local CollisionColor = Color3.fromRGB(255, 55, 55) -- The RGB value for when collision is detected.
local PlacingColor = Color3.fromRGB(55, 255, 55) -- The RGB value for when collision is not detected.

-- Keybinds

local KeyCodeCancel = Enum.KeyCode.X -- Key used to cancel placement.
local KeyCodeRotate = Enum.KeyCode.R -- Key used to rotate the model being placed.
local KeyCodeRaiseFloor = Enum.KeyCode.U -- Key used to raise the floor.
local KeyCodeLowerFloor = Enum.KeyCode.L -- Key used to lower the floor.

-- Advanced Settings - End

local transparentDelta = 0.6
local hitboxTransparency = 0.6
local step = 1.1

-- DO NOT EDIT PAST THIS POINT UNLESS YOU KNOW WHAT YOUR DOING!

local PlacementModuleV2 = {}

local runService = game:GetService("RunService")
local userInputService = game:GetService("UserInputService")

local player = game.Players.LocalPlayer
local character = player.Character or player.CharacterAdded:Wait()
local mouse = player:GetMouse()	

local posX
local posY
local posZ
local startingY
local currentRot = true
local smartRot = false

local lastY
local rot = 0
local c = 0

local modelInfo = {}
local cframe

local grid = math.abs(tonumber(GridSize))

local model
local plot
local objs
local collisionPoints
local collisionPoint

local placingMode
local canPlace
local placing
local colliding
local stacking
local canStart = true

wait(0.1) -- DO NOT REMOVE (It will error if you remove)

local humanoid = character:FindFirstChild("Humanoid")

local function EditColor()
	if PlacementModuleV2:GetCurrentState() == "Collision" and model then
		model.PrimaryPart.Color = CollisionColor
	elseif PlacementModuleV2:GetCurrentState() ~= "Collision" and model then
		model.PrimaryPart.Color = PlacingColor
	end
end

local function CheckHitbox()
	if model then
		colliding = false
		
		collisionPoint = model.PrimaryPart.Touched:Connect(function() end)
		collisionPoints = model.PrimaryPart:GetTouchingParts()
		
		for i = 1, #collisionPoints do
			if not collisionPoints[i]:IsDescendantOf(model) and not collisionPoints[i]:IsDescendantOf(character) then
				colliding = true
				
				break
			end
		end
		
		collisionPoint:Disconnect()
		
		return colliding
	end
end

local function CalcualateNewPosition()
	if MoveByGrid then
		posX = math.floor((mouse.Hit.X / grid) + 0.5) * grid
		posZ = math.floor((mouse.Hit.Z / grid) + 0.5) * grid
	else
		posX = mouse.Hit.X
		posZ = mouse.Hit.Z
	end
	
	if EnableFloors and not stacking then
		if posY > MaxHeight then
			posY = MaxHeight
		elseif posY < startingY then
			posY = startingY
		end
	end
	
	if stacking then
		posY = math.floor(mouse.Hit.Y) + step
		
		if posY > MaxHeight then
			posY = MaxHeight
		elseif posY < startingY then
			posY = startingY
		end
	end
end

local function CalcFinalCFrame()
	if currentRot then
		cframe = CFrame.new(posX, posY, posZ) * CFrame.new(model.PrimaryPart.Size.X / 2, 0, model.PrimaryPart.Size.Z / 2)
	else
		cframe = CFrame.new(posX, posY, posZ) * CFrame.new(model.PrimaryPart.Size.Z / 2, 0, model.PrimaryPart.Size.X / 2)
	end
end

local function Calc(tf)
	if currentRot then
		if Interpolate then
			model:SetPrimaryPartCFrame(model.PrimaryPart.CFrame:Lerp(CFrame.new(posX, posY, posZ) * CFrame.new(model.PrimaryPart.Size.X / 2, 0, model.PrimaryPart.Size.Z / 2) * CFrame.Angles(0, math.rad(rot), 0), 1 - InterpolateSpeed))
		else
			model:SetPrimaryPartCFrame(CFrame.new(posX, posY, posZ) * CFrame.new(model.PrimaryPart.Size.X / 2, 0, model.PrimaryPart.Size.Z / 2) * CFrame.Angles(0, math.rad(rot), 0))
		end
	else
		if Interpolate then
			model:SetPrimaryPartCFrame(model.PrimaryPart.CFrame:Lerp(CFrame.new(posX, posY, posZ) * CFrame.new(model.PrimaryPart.Size.Z / 2, 0, model.PrimaryPart.Size.X / 2) * CFrame.Angles(0, math.rad(rot), 0), 1 - InterpolateSpeed))
		else
			model:SetPrimaryPartCFrame((CFrame.new(posX, posY, posZ) * CFrame.new(model.PrimaryPart.Size.Z / 2, 0, model.PrimaryPart.Size.X / 2)) * CFrame.Angles(0, math.rad(rot), 0))
		end
	end
end

local function ModifyCoordinates()
	if placingMode and canPlace and model then
		CalcualateNewPosition()
		Calc(true)
		
		if DetectCollisions then
			CheckHitbox()
			EditColor()
		end
	end
end
local function EditFloor(g)
	if posY <= MaxHeight and posY >= startingY then
		if g == 2 then
			posY = posY + FloorStep
		else
			posY = posY - FloorStep
		end
	end
end

local function CheckRotation()
	if model then
		if currentRot then
			currentRot = false
		else 
			currentRot = true
		end
	end
end

local function RotateModel()
	if smartRot then
		if currentRot then
			rot = rot + RotationStep
		else
			rot = rot - RotationStep
		end
	else
		rot = rot + RotationStep
	end
	
	CheckRotation()
end

local function RemovePlacement(tf)
	placingMode = false
	canPlace = false
	canStart = true
	
	if DisplayGrid then
		for i, v in pairs(plot:GetChildren()) do
			if v then
				if v:IsA("Texture") and v.Name == "GridTexture" then
					v:Destroy()
				end
			end
		end
	end
	
	stacking = false
	
	if not BuildModeEnabled and tf then
		model:Destroy()
		model = nil
	else
		model:Destroy()
		model = nil
	end
end

local function InputDetector(input, gpe)
	if placingMode and model then
		if input.KeyCode == KeyCodeCancel then
			RemovePlacement()
		elseif input.KeyCode == KeyCodeRaiseFloor then
			EditFloor(2)
		elseif input.KeyCode == KeyCodeLowerFloor then
			EditFloor(1)
		elseif input.KeyCode == KeyCodeCancel then
			RemovePlacement(false)
		elseif input.KeyCode == KeyCodeRotate then
			RotateModel()
		end
	end
end

local function DisplayGridOnCanvas()
	if DisplayGrid then
		local gridTex = Instance.new("Texture")
		
		gridTex.Name = "GridTexture"
		gridTex.Texture = GridTexture
		gridTex.Parent = plot
		gridTex.Face = Enum.NormalId.Top
		
		gridTex.StudsPerTileU = 2
		gridTex.StudsPerTileV = 2
		
		if SmartDisplay then
			gridTex.StudsPerTileU = grid
			gridTex.StudsPerTileV = grid	
		end
	end
end

local function GetModel(loc, id, ignoreLocation)
	model = loc:FindFirstChild(id):Clone()
	
	startingY, posY = model.PrimaryPart.CFrame.Y, model.PrimaryPart.CFrame.Y
	model:SetPrimaryPartCFrame(CFrame.new(posX, lastY, posZ))
	
	model.Parent = ignoreLocation
	
	for i, m in pairs(model:GetDescendants()) do
		if m then
			if m:IsA("Part") or m:IsA("UnionOperation") or m:IsA("MeshPart") then
				m.CanCollide = false
				
				if TransparentModels then
					m.Transparency = m.Transparency + transparentDelta
				end
			end
		end
	end
	
	model.PrimaryPart.Transparency = hitboxTransparency
	
	if IgnoreItems and not stacking then
		mouse.TargetFilter = ignoreLocation
	else
		mouse.TargetFilter = model
	end
end

local function Setup(stk, location, id, loc, sr)
	placingMode = true
	canPlace = true
	canStart = false
	stacking = stk
	smartRot = sr
	
	if BuildModeEnabled then
		canStart = true
	end
	
	GetModel(loc, id, location)
	DisplayGridOnCanvas()
end

function PlacementModuleV2:Finish(e)
	if placingMode and model and not BuildModeEnabled then
		wait(CollisionCheckCooldown)
		
		Calc(currentRot)
		CalcualateNewPosition()
		CalcFinalCFrame()
		CheckHitbox()
		
		if not colliding then
			e:FireServer(self:GetModelInfo(), model.Parent, cframe)
			
			RemovePlacement(false)
		end
	elseif BuildModeEnabled and canPlace and placingMode and not colliding and model then
		lastY = model.PrimaryPart.CFrame.Y
		
		wait(CollisionCheckCooldown)
		
		Calc(currentRot)
		CalcualateNewPosition()
		CalcFinalCFrame()
		CheckHitbox()
		
		if not colliding then
			e:FireServer(self:GetModelInfo(), model.Parent, cframe)
		end
	end
end

function PlacementModuleV2:CancelPlacementManual()
	RemovePlacement(false)
end

function PlacementModuleV2:GetModelInfo()
	if model then
		modelInfo = {
			["Name"] = model.Name;
			["CollisionState"] = colliding;
			
			
			["CFrame"] = {
				["X"] = model.PrimaryPart.CFrame.X,
				["Y"] =  model.PrimaryPart.CFrame.Y,
				["Z"] =  model.PrimaryPart.CFrame.Z,
				["Rotation"] = model.PrimaryPart.Orientation.Y
			}
		}
		
		return modelInfo
	end
end

-- these state functions were going to do stuff but they are mostly useless at this point
function PlacementModuleV2:GetCurrentState()
	local state
	
	if placingMode and not colliding then
		state = "Movement"
	elseif placing and not colliding then
		state = "Placing"
	elseif colliding then
		state = "Collision"
	elseif canStart and not placingMode then
		state = "Waiting"
	else
		state = nil
	end
	
	return state
end

function PlacementModuleV2:ResumeState(msg)
	if self:GetCurrentState() == "Movement" then
		placingMode = true
		canPlace = true
		
		return msg
	elseif self:GetCurrentState() == "Placing" then
		return "Not developed yet"
	elseif self:GetCurrentState() == "Collision" then
		colliding = nil
		placingMode = true
		canPlace = true
		
		return msg
	elseif self:GetCurrentState() == "Waiting" then
		return "Not developed yet"
	end
end

function PlacementModuleV2:PauseState(msg)
	if self:GetCurrentState() == "Movement" then
		placingMode = nil
		canPlace = nil
		
		return msg
	elseif self:GetCurrentState() == "Placing" then
		return "Not developed yet"
	elseif self:GetCurrentState() == "Collision" then
		placingMode = nil
		canPlace = nil
		colliding = true
		
		return msg
	elseif self:GetCurrentState() == "Waiting" then
		return "Not developed yet"
	end
end

function PlacementModuleV2:new(id, location, plt, loc, stack, smtr)
	if canStart and not placingMode and not BuildModeEnabled then
		plot = plt
		
		Setup(stack, loc, id, location, smtr)
	elseif BuildModeEnabled then
		plot = plt
		
		if model then
			model:Destroy()
		end
		
		Setup(stack, loc, id, location, smtr)
	else
		return self:GetCurrentState()
	end
end

humanoid.Died:Connect(function()
	RemovePlacement(false)
end)

runService:BindToRenderStep("Input", Enum.RenderPriority.Input.Value, ModifyCoordinates)

userInputService.InputBegan:Connect(InputDetector)

return PlacementModuleV2

Anything I can improve on other than making it object oriented is greatly appreciated!

2 Likes
#2

Since it’s not object oriented you could better convey that meaning by defining your functions with dot syntax instead of colon syntax. : syntax is only used for object oriented purposes.

1 Like
#3

Looks good to me. The only thing that stood out:

wait(0.1) -- DO NOT REMOVE (It will error if you remove)

This isn’t really common/“good” practice. Are you doing this because you’re worried the Humanoid won’t be present yet? Try using character:WaitForChild("Humanoid"). Random yields to prevent “things breaking” are generally the sign of doing something hacky or prone to breaking, although in this case it looks like it can be fixed easily. This is pedantic advice, but such things are frowned upon (usually because they’re associated with headaches).

I don’t know how much making this object-oriented would be an improvement. I would advise against over-engineering things when it’s not necessary to do so; too much abstraction or compartmentalization is known as over-engineering, and the cost of maintaining such a codebase is high, especially if you want to rewrite your system if the game is going to work differently.

Here are some nice articles I’ve picked up on implementing OOP effectively:

Best of luck.

2 Likes
#4

for the wait() function I really dont remember why it’s there as I wrote the module over 6 months ago. I think it must have something to do with the humanoid. Thanks!