Am I overengineering my 1v1 system?

So I’m working on a pretty advance and complicated game (Training - Roblox Studio 2022-01-17 21-51-46) which IMO requires some advanced coding like I’ve had to implement FIFO queues, finite state machines, behavior trees etc, however, I think I’ve at some points over-done it a bit and just started to make things unnecessarily complicated and so I’d like to get some help on what I could do differently.

From the clip, you can see my game is 1 vs 1 so I tried to make a system to easily set up and reference player1 and player2. On the client, I have a module that uses OOP that has this function which gets ran at the start of every new round (probably don’t need to be doing that)

function Player.new(obj,opponent)
	instance.CommandBuffer = FIFO.new() 
	instance.xVel = 0
	instance.yVel = 0
	instance.jumpsLeft = 1
	instance.direction = -1
	instance.state = "idle"
	instance.hits = 0
	instance.jumpPressed = 0
	instance.opponent = opponent
	instance.animations = {}
	instance.currentAnimation = nil

	instance.grounded = true
	instance.crouching = false
	instance.freefall = false
	instance.blocking = false
	instance.canAttack = false
	instance.jumping = false
	instance.attacking = false
	
	--instance.Player = Players:GetPlayerFromCharacter(obj.Character)
	instance.Name = obj.Name
	instance.Humanoid = obj.Humanoid
	instance.Character = obj.Character
	instance.HumanoidRootPart = obj.HumanoidRootPart
	instance.HRP = obj.HumanoidRootPart
	instance.PlayerGui = playerGui
	instance.ScreenGui = screenGui
	
	--Setup Animations
	for _,animation in pairs(RepStorage.Assets.Animations:GetDescendants()) do
		if animation:IsA("Animation") then
			if animation.AnimationId ~= "" then
				--animation.Name ..= tostring(animation.Parent.Name)
				instance.animations[animation.Name..tostring(animation.Parent.Name)] = instance.Humanoid:LoadAnimation(animation)
				--warn("[CLIENT]: Loaded animation: "..animation.Name)
			end
		end
	end
	
	obj.Humanoid.Died:Connect(function()
		--warn(obj.Name.." Died!")
		RunService:UnbindFromRenderStep("Update")
	end)

	--workspace.ChildAdded:Connect(function(child)
	--	if child.Name == obj.Name then
	--		--warn(obj.Name.." Respawned!")
	--	end
	--end)

	return setmetatable(instance,Player)
end

I also have a second module which is almost an identical version just slightly modified because it’s for my AI, but throughout both modules, I send the instance to other scripts as an argument for functions mainly so I can easily reference their opponent and some other things, but I feel like this is kind of clunky to work with at times. It does work, but I feel like there is a much neater and more organized method.

Can view the entire code here if you’re interested local Player = {}Player.__index = Playerlocal RunService = game:GetService - Pastebin.com
Sorry for the large post probably confusing.

Hm, I find myself in this situation ALOT. My solution is often to chunk it up into more bits… Haha…

No I feel like thats fine, however I would create things like an AnimationHandler and PlayerState objects to minimalize how much you have there.

1 Like

4 (maybe) things that could be changed to make the code clear are:

  • Instead of setting instance.state to a string value use a separate module script that contains the states (security and in-general cleaner) ex:
local customPlayerStatesEnum = {
	IDLE = "idle",
	RUNNING_FROM_SCARY_MONSTER = "child's play"
}

return customPlayerStatesEnum
local customPlayerStatesEnum = require(script.CustomPlayerStatesEnum)
instance.state = customPlayerStatesEnum.IDLE

implementing that way also gives you automatic string returns

  • Renaming the instance variable (you don’t want to try and create a part only to realize your using the wrong instance)

  • Renaming the function (same reason as above)

  • I don’t see why you need the command bar queue because from skimming over the entire code I could only see one command being used at a time. I’m probably wrong though ¯\_(ツ)_/¯

Otherwise great job