Modular Character Controller (v4.4-4.6)

[dragonforge-dev]

@panthera

I’ve moved the action collision logic from the manager to action collision so now nodes are in charge of how they interact with each other and the manager’s only concern is proper selection of action nodes to call on per request.

This seems like the appropriate amount of change.

@panthera

I’ve also looked at your state machine and found its the same as my action manager. Because of this I was very confused about your comment calling my thinking “not OOP” and how managers were anti-patterns, which was a shock to me since I had never heard this

To be clear, the main difference is you call yours a “Manager” and I call mine a “State Machine”. The anti=pattern is using the name Manager itself. Because, as I said, it causes you to think about things in a way where the “Manager” needs to know details about those below it to do its job. In other words, it micro-manages things it should delegate.

The State Pattern was introduced by The Gang of Four in their book Design Patterns. I am implementing the StateMachine object as a State Pattern using the Pull Method. This means that instead of the StateMachine deciding when a state should be entered, the States make that decision. Architecturally, I have separated out each State into its own object, but they operate only inside of a State Machine object, and are therefore all conceptually part of the same object and pattern. Hence the naming convention of StateMachine and State. Technically, you could also look at the State objects as implementations of the Decorator Pattern.

@panthera

I agree now that “action manager” could use a name change to be more clear but I just don’t know what would be more clear for both the function it servers and its use in the scene tree. Maybe “Action Coordinator” or something like that.

You saw the effects of the Manager Anti-Pattern in your code. Because once you looked at it, you moved the collision logic into the Action nodes, and out of the Manager. This is a perfect example of how knowing that the word “Manager” is an anti-pattern can prevent you from falling into the trap. There was a great C+++ conference talk on this years ago. Sadly I can no longer find it on YouTube. In it, the speaker talked about the use of the word “Manager” and how to eliminate it.

If you take a look at the Active Record Documentation (which is the data implementation convention for Ruby on Rails), you will see that data objects are singular PascalCase, and the tables containing them are plural snake_case. So a Book is contained in a table named books. This same logic can be used in general programming, and allows you to think about things in a new way.

PlayerStatistics Detour

Let’s take the example of player statistics. Things like number of times a player jumps, coins collected, etc. I want to track any number of statistics across the game. My brain says, “I need a way to manage this.” So I take a step back and say what does one statistic look like?

A statistic must:

1. Have a unique name.
2. Be stored by game level. (For example, I want to be able to know the number of times the player jumped in the jungle level vs the floating island level.)
3. Store its value between game sessions. (Needs to be stored on disk.)
4. Only go up. (Stats never go down, if there are values that can go down, they are not statistics.)

So, I could create a StatisticManager. But following the Active Record Method, if one is a PlayerStatistic, then a collection would be player_statistics - however, since I’m not storing them in a database table, but an object, the collection becomes PlayerStatistics.

So let’s look at an implementation for that class I made for a game I’m working on.

class_name PlayerStatistics extends Resource

var stats: Dictionary


## Adds the passed value to the statistic named, based on the current level.
## If the statistic or level doesn't exist, a new entry is created.
func add(statistic_name: String, new_value: int) -> void:
	var category: Dictionary = stats.get_or_add(statistic_name, {Game.level_name: 0})
	var value: int = category.get_or_add(Game.level_name, 0)
	value += new_value
	category[Game.level_name] = value
	print(stats)


## Gets a list of all the categories in which statistics are stored.
func get_list() -> Array:
	var list: Array[String]
	for key: String in stats:
		list.append(key)
	return list


## Retrieves the aggregate value for a statistic across all levels played.
func retrieve(statistic_name: String) -> int:
	var category: Dictionary = stats.get(statistic_name)
	var total: int = 0
	for key: String in category:
		total += category[key]
	return total

Basically, it’s just a Dictionary of Dictionaries. You’ll note I don’t do any saving to disk here. Instead, that happens in the UserProfile object. The UserProfile handles all of the saving and loading of statistics because it has a PlayerStatistics object that it saves and loads. And the Player object has a PlayerStatistics object that is just a reference to the UserProfile object’s PlayerStatistics object. This distinction wouldn’t be necessary in most games, but since this is a multiplayer game, I will have multiple Player objects in a game and only want to track the stats of the User in the game.

Back to Renaming Action Manager

Back to your class, currently named Action Manager. You posited you could rename it Action Coordinator. This is a semantic name change that does not avoid the Manager Anti-Pattern - because you are still thinking of it as a manager, or coordinator, or director, etc.

I took a look at your ActionManager and ActionContainer code. I can see why you think they need to be separate, because ActionManager contains an ActionContainer instance. And it is a logical OOP breakdown. It is not strictly necessary, because ActionContainer is the data, and ActionManager is the logic, and neither functions without the other. But if you like it, ok.

---

Access Modifiers

A note on your ActionManager code and access modifiers.

You have indicated in your code that private functions are in a section where every function name starts with two underscores instead of one. Then it looks like you are using one underscore to indicate protected functions. I mention this because it is not the Godot convention as set out by the GDScript Style Guide. Since you are making a plugin, it is important to note that making up your own conventions is going to A) obfuscate your code, and B) create hurdles for people contributing to your code.

GDScript does not support access modifiers. Everything in a script is public. Period. The convention is to prepend private functions with an underscore to indicate that they are only to be used inside the class. Likewise, class variable names are prepended to indicate the same thing. Function arguments are prepended with an underscore to indicate they are not used in the function. (This is typically for functions called by signals that pass arguments you do not need.)

Before, you had said that you do not see a difference between my code and yours. You’ll note if you look at State, that it calls a private variable inside itself to reference the StateMachine, but calls a public function.

## Asks the [StateMachine] to switch to this [State]. Should always be used
## instead of calling _enter_state() when a [State] wants to switch to itself.
func switch_state() -> void:
	_state_machine.switch_state(self)


## Asks the [StateMachine] to exit this [State] if it is the current [State].
## Should always be used instead of calling _exit_state() when a [State] wants
## to exit. Will fail if the current [State] is not this one.
func clear_state() -> void:
	if is_current_state():
		_state_machine.clear_state()


## Returns true if this is the current [State].
func is_current_state() -> bool:
	return _state_machine.is_current_state(self)

In some cases, those StateMachine functions are purely helper functions intended to prevent the breaking of encapsulation by providing an abstracted interface.

## Should ideally be called from [method State.is_current_state][br][br]
## Returns true if the passed [State] is the current [State].
func is_current_state(state: State) -> bool:
	return _current_state == state


## Should ideally be called from [method State.clear_state][br][br]
## Exits, then clears the current state and sets [member State.can_transition]
## to true for that state.
func clear_state() -> void:
	if _current_state:
		_current_state.can_transition = true #Reset this so we don't get frozen out of the state if it's set to false
		_current_state._exit_state() # Run the exit code for the current state. (Even if the state says you can't exit it.)
		_current_state = null

Your code introduces a protected access modifier that GDScript has no concept of, either in code or convention. It also replaces the private access modifier convention. I would recommend that you use the private access modifier convention and make your protected functions public. You can still indicate how they should be used in documentation.

@export Variable Note

Also, I notice that you have @export variables that are private. Again, this breaks convention - since an @export variable can be changed in the Inspector, it is by definition public. If you want it to be private, I recommend getting rid of the @export notation, or creating a private version of the variable that gets its value from the @export variable interface.

---

Back to Renaming Action Manager

So looking at your code for Action Manager, it has two public functions play_action() and stop_action(); and five protected functions _play_private_action(), _add_action(), _remove_action(), _get_action(), _get_context().

I list these because a good way of picking a class name is to make the class name and function names self-documenting code. So let’s try renaming Action Manager to Actions as I recommended before.

• ActionManager.play_action() becomes Actions.play(). Because we know that Actions if we play it, is going to play an action.
• ActionManager.stop_action() becomes Actions.stop(). Because we know that Actions if we stop it, is going to stop an action.
• ActionManager._play_private_action() becomes Actions._play(). Because we know that an underscore indicates a private function.
• ActionManager._add_action() becomes Actions.add(). Because we know that adding to Actions adds an action. And it doesn’t need to be private because theoretically anyone can add an action and it should not derail the object.
• ActionManager._remove_action() becomes Actions.remove(). Because we know that the only thing we can remove from Actions is an action. And it doesn’t need to be private because anyone removing an action will not cause it to break.
• ActionManager._get_context() becomes Actions.get_permissions(). Because it seems this function is primarily used to see what permissions are available and so it’s a more descriptive name. It does not need to be private, because we are just returning information. It’s a getter. Anyone can do what they want with that information.

---

Perhaps Actions seems like a slightly odd name, so what does Action Manager really do? Well, it plays and stops Actions So ActionPlayer would follow the naming convention of things like AudioStreamPlayer and AnimationPlayer.

• ActionPlayer.play()
• ActionPlayer.stop()
• ActionPlayer._play()
• ActionPlayer.add()
• ActionPlayer.remove()
• ActionPlayer.get_permissions()

In this case, maybe it makes sense to rename ActionContainer to Actions because it’s just a group of actions with some accessors.

• ActionContainer._to_string() becomes Actions.to_string(). Because there’s no reason to make conversion methods prtivate.
• ActionContainer.add_action() becomes Actions.add().
• ActionContainer.remove_action() becomes Actions.remove().
• ActionContainer.get_actions_by_type() becomes Actions.get_by_type().
• ActionContainer.get_action_by_name() becomes Actions.get_by_name().
• ActionContainer.has_name() becomes Actions.has_name().
• ActionContainer.has_type() becomes Actions.has_type().
• ActionContainer._sort_action() becomes Actions._sort().

But if you want Actions to replace ActionManager or you want to follow Godot conventions and are using ActionPlayer, you could also call this ActionLibrary.

• ActionContainer._to_string() becomes ActionLibrary.to_string().
• ActionContainer.add_action() becomes ActionLibrary.add().
• ActionContainer.remove_action() becomes ActionLibrary.remove().
• ActionContainer.get_actions_by_type() becomes ActionLibrary.get_by_type().
• ActionContainer.get_action_by_name() becomes ActionLibrary.get_by_name().
• ActionContainer.has_name() becomes ActionLibrary.has_name().
• ActionContainer.has_type() becomes ActionLibrary.has_type().
• ActionContainer._sort_action() becomes ActionLibrary._sort().

Alternately you could leave it ActionContainer but consider removing the redundant part of function names. (Namely the overuse of the word “action”.)

---

@panthera

Now about combining action manager and action container, I can’t. I’ve removed what I believe I can from action manager, as mentioned in the beginning, and the last big step before it becomes just a container would be removing the filtering using permissions. This just wouldn’t work because its effectively the “state” of the system.

The fact that you’re using the term “state” seems to indicates that underneath the naming convention, you’re following the State Pattern. Which your code seems to back up.

Your ActionContainer code is literally a single Array and a bunch of access methods for that Array. It could easily be incorporated into the ActionManager if you wanted. It doesn’t have to be though. If you combined them and just called them Actions, it would look like this:

• Actions.play()
• Actions.stop()
• Actions._play()
• Actions.add() (Combine the two, or make the one from ActionContainer private and call it.)
• Actions.remove() (Combine the two, or make the one from ActionContainer private and call it.)
• Actions.get_permissions()
• Actions.to_string()
• Actions.get_by_type().
• Actions.get_by_name().
• Actions.has_name().
• Actions.has_type().
• Actions._sort().

@panthera

I’ve explored some options for moving this filtering responsibility to action nodes themselves but the system would just not work as it does now and lose a key benefit, which is only one action activates per request.

I don’t think you need to do this. It is in fact a state machine that determines what state(s) are active. Nothing wrong with that.

@panthera

Anyway, I probably put too much work into this response but learning about the manager anti-pattern was interesting and I appreciate the push for me to more deeply analyze a parts of my system with something to compare it too. I am now putting more thought into what objects should be responsible for what behaviors.

You get out of your post what you put into them.