Any way I can improve the structure of my StateMachine?

Help and Feedback Code Review
1

I’m noticing that as more States are added my structure will look more and more cluttered. It’s not difficult to read but I’d like the code to be more condensed.

My idea is to place the states under States.Activations and checking if the state is valid through States.Transitions (Example below). I can see this idea has it’s downsides too, any recommendations?

Example 
local Sprint = require(script.Sprint)
local Block = require(script.Block)
local LightAttack = require(script.LightAttack)

local States = {
	Activations = {
		ActivateState = function(StateMachine, State, Activation, ...)
			StateMachine:TransitionState(State, Activation)
		end,
		LightAttack = function(StateMachine, Character, ...)
			LightAttack:Light(StateMachine, Character)
		end,
		EndCombat = function(StateMachine, Character)
			Character.Humanoid.WalkSpeed = Character:GetAttribute("BaseWalkSpeed")
			StateMachine:ChangeState("Idle")
		end,
		Sprint = function(StateMachine, Character, InputState)
			if InputState then
				Sprint:Enable(StateMachine, Character)
			end
		end,
		EndSprint = function(StateMachine, Character,  InputState)
			if not InputState then
				Sprint:Disable(StateMachine, Character)
				StateMachine:ChangeState("Idle")
			end
		end,
  },
},

How it works Currently: StateMachine checks if the state is valid through States.Transitions and finds the function to run through States.Activations[State][Function].

local Sprint = require(script.Sprint)
local Block = require(script.Block)
local LightAttack = require(script.LightAttack)

local States = {
	Activations = {
		Idle = {
			ASprint = function(StateMachine, ...)
				StateMachine:TransitionState("Sprinting", "Sprint", ...)
			end,
			ALightAttack = function(StateMachine, ...)
				StateMachine:TransitionState("Attacking", "LightAttack", ...)
			end,
			ABlock = function(StateMachine, ...)
				StateMachine:TransitionState("Blocking", "Block", ...)
			end,
		},
		Sprinting = {
			Sprint = function(StateMachine, Character, InputState)
				if InputState then
					Sprint:Enable(StateMachine, Character)
				end
			end,
			EndSprint = function(StateMachine, Character,  InputState)
				if not InputState then
					Sprint:Disable(StateMachine, Character)
					StateMachine:ChangeState("Idle")
				end
			end,
			ALightAttack = function(StateMachine, ...)
				Sprint:Disable(StateMachine, ...)
				StateMachine:TransitionState("Attacking", "LightAttack", ...)
			end,
			ABlock = function(StateMachine, ...)
				Sprint:Disable(StateMachine, ...)
				StateMachine:TransitionState("Blocking", "Block", ...)
			end,
		},	
		Attacking = {
			LightAttack = function(StateMachine, Character, ...)
				LightAttack:Light(StateMachine, Character)
			end,
			EndCombat = function(StateMachine, Character)
				Character.Humanoid.WalkSpeed = Character:GetAttribute("BaseWalkSpeed")
				StateMachine:ChangeState("Idle")
			end,
		},
		Blocking = {
			Block = function(StateMachine, Character, ...)
				Block:Enable(StateMachine, Character)
			end,
			EndBlock = function(StateMachine, Character, ...)
				Block:Disable(StateMachine, Character)
			end,
			EndSprint = function(StateMachine, Character, ...)
				Sprint:Disable(StateMachine, Character)
			end,
		},
		Stunned = {
			Stun = function(StateMachine, Character)
				
			end,
			EndStun = function(StateMachine, Character)
				
			end,
		},
	} 
}

States.Transitions = {
	Idle = {
		ASprint = true,
		ALightAttack = true,
		ABlock = true,
		TakeDamage = true,
	},
	Sprinting = {
		ALightAttack = true,
		ABlock = true,
		EndSprint = true,
		TakeDamage = true,
	},	
	Attacking = {
		EndCombat = true,
		TakeDamage = true,
	},
	Blocking =  {
		EndBlock = true,
		TakeDamage = true,
	},
}
1 Like
2

Im not that experienced with state machines, but as far as I know they should be flexible, there might be a lot of manual work and patterns you could compact more in your code.

And for organization, it might be good to split the code both tables and functions inside them in modules, easier to read and more scalable, but its your option, always make sure your time is worth it, if you wont be changing the code in a while just stay it like that, if its gonna grow you should change it.

2 Likes
3

Scaling your system is inefficient. If you add states to the Activations table, you also have to update the Transitions table aswell. You could just make the transitions table by code though but I’m not too fond of the idea of having a table of bools.

I’m not sure what you mean by checking “if the state is valid” and why you have functions within the states, but it seems on the surface that your system is quite complex.

I would omit sprinting and idle from the states entirely or put them as another state machine class and just have the combat states “attacking” and “blocking”. That way, everything is simpler.

The way I make state machines is to have each state as a ModuleScript with enter() and exit() functions. When Transition() is called, it calls enter() for the new state, and if there’s a current state in place, call exit() on it.

1 Like
4

I’m not sure what you mean by checking “if the state is valid” and why you have functions within the states, but it seems on the surface that your system is quite complex.

I use the transitions table to check if I can run the function. After, I run the function if it exists.

RemoteEvents.Sprint.OnServerEvent:Connect(function(Player, Input)
	if typeof(Player) ~= "Instance" or typeof(Input) ~= "boolean" then return end

	if StateMachine:RequestStatus(Player)	then
		if Input then
			StateMachine:ActivateState("Sprint", Player.Character, Input)
		elseif not Input then
			StateMachine:ActivateState("EndSprint", Player.Character, Input)
		end
	end
end)

function self:ActivateState(ActivateState : string, ...)
			local CurrentState = self.Persist.State

			if States.Activations[ActivateState] and States.Transitions[CurrentState][ActivateState] then
				States.Activations[ActivateState](self, ...)
			end
		end

I’ve settled on just throwing the functions into the activations table, and added comment lines for cleanliness. The transitions table remained roughly the same. Do you think what you recommended might be better in the long run?

Current Activation Table 
local States = {
	Activations = {
		--// Activate States
		Activate = function(StateMachine, State, Activation, ...)
			StateMachine:TransitionState(State, Activation, ...)
		end,
		
		--// Sprinting 
		Sprint = function(StateMachine, Character, InputState)
			if InputState then
				Sprint:Enable(StateMachine, Character)
			end
		end,
		EndSprint = function(StateMachine, Character, InputState)
			Sprint:Disable(StateMachine, Character)
		end,
		
		--// Attacking 
		Light = function(StateMachine, Character, ...)
			Light:Light(StateMachine, Character)
		end,
		EndCombat = function(StateMachine, Character)
			if StateMachine.Persist.Sprinting then
				Sprint:Enable(StateMachine, Character)
			else
				Character.Humanoid.WalkSpeed = Character:GetAttribute("BaseWalkSpeed")
				StateMachine:ChangeState("Idle")
			end
		end,
		
		--// Blocking 
		Block = function(StateMachine, Character, ...)
			Block:Enable(StateMachine, Character)
		end,
		EndBlock = function(StateMachine, Character, ...)
			Block:Disable(StateMachine, Character)
		end,
	}
end
Current Transitions Table

States.Transitions = {
Idle = {
Sprint = true,
Light = true,
Block = true,
Stun = true,
EndSprint = true,
TakeDamage = true,
},
Sprinting = {
EndSprint = true,
Block = true,
Light = true,
TakeDamage = true,
},
Attacking = {
EndCombat = true,
EndSprint = true,
TakeDamage = true,
},
Blocking = {
EndBlock = true,
TakeDamage = true,
},
Stunned = {
EndStun = true,
TakeDamage = true,
},
DeathState = {
Die = true,
},
}

5

Keeping everything flat under one activations table works fine for now, but you’re gonna regret it once you start stacking more states. Add a bit of metadata like validstates to each action so the code knows what belongs where without that giant transitions table.

Long term, the cleanest route imo is splitting each state into its own module and letting the statemachine auto load them.

2 Likes
6

This topic was automatically closed 14 days after the last reply. New replies are no longer allowed.