Signal clean up

[monban]

Back in #255, @TheSecondReal0 introduced a priority system for game state signals. I think this was done to prevent a race condition of some sort. I appreciate the need for it, but I was never quite satisfied because the priorities were always a bit nebulous to me, magic numbers like priority 3 don't mean much, and make the code harder to reason about.

We discussed replacing the priorities with an enum, so for example instead of priority 3 we would have priority Network or something like that. I think the real source of the problem is that GameManager is too important, too much is controlled from there. If the map load needs to happen after a network event, I would rather the map controller listen for an event from the network manager, rather than everybody depend on game controller.

I've created a branch that doesn't use the priorities, with the plan to then go back and create new signals to create the correct dependency chain. Much to my surprise, after removing the priority (leaving everything hanging off the same GameManager signals, which I'm still not super happy with) the game still functions! Is there some kind of race condition that only happens sometimes, or am I missing something? Can we safely remove this functionality?

[TheSecondReal0]

I originally added that because it seemed messy to connect to signals in possibly obscure scripts/nodes. It makes it hard to track when something happens and looks disorganized to me. It also makes it easier if you want to add a step in between. Without priority signals you would signal main.gd to create players right after the map is loaded from maps.gd. If you wanted to add a step in between (for example, role/task assignment) you would need to edit multiple scripts. Another reason is that you could use the priority after a step (like map loading) and just assume everything related to that was done without having to try and find what signal to connect to (and then have to somehow navigate to that script/node).

The current priority system is definitely clunky, every single script/script instance is referenced every single time a priority signal is emitted. And as you said it uses arbitrary numbers instead of something more descriptive.

Using signals from each individual thing has its problems too, as I described before it’s harder to change the order and add steps.

To me the priority signals seem cleaner, and they make it easy to tell at a glance what happens when (assuming proper documentation and/or something like a priority enum).

[nicemicro]

It's been grinding my gears while I was drawing up the network connections and stuff like that for starting the game when I was trying to fit in the distribution of character customization to that place.

I think having the priority signals would definitely make sense in an organizational stand point. It makes much easier to understand if we have a central script (game manager), that is used to emit certain signals, and every other script that's in relation to it, is started from there based on the signals. However, I think in its current form it doesn't really live up to our expectations, which is, that when godot does something in a weird order (which in this case is that the map is only instanced for some reason after all signals have been emitted, even though the switch scene function has been invoked in the middle - maybe the scene is only effectively switched once we get back to the main loop?), it doesn't really make much difference.

What I was thinking about, along @monban 's idea, is what if we actually had signals in each script that are emitted when each state_change_priority function is completed?
So instead of having a loop emitting the however many priority signals in a row, we just emit signal priority 1 (using an enum?), and just... wait. And when the job is done, whatever have picked up signal priority 1, signals back "I'm done", and we check what priority's next and emit that then. Using this method, we can invoke the map switching script, but instead of whenever the function picking up the signal's done, we only emit the next priority signal, when the job has actually been finished (like the scene has been switched to main.tscn).

[monban]

When we talked about this a few hours ago @nicemicro, you were stuck at work and couldn't properly respond. Now I'm at work, and don't have time to type out a proper response. The troubles of a team in many timezones 😁

I'll summarize for now. Based on what @TheSecondReal0 and @nicemicro said, I think we're using signals incorrectly at the moment. Signals (which are basically an implementation of the observer pattern) are great when things need to happen in response to other things, but you want to retain loose coupling between them. Order shouldn't matter.

I think our best way ahead would be to not use signals for events that need to happen in a particular pattern. Instead, when the game moves into the Normal state, or whatever, instead of a bunch of things listening for that event and doing their setup, ONE object (GameController or something?) should notice the event, then call all the synchronous functions in the correct order to get the game set up.

I'll elaborate more after work, in case this isn't clear.

[TheSecondReal0]

Calling directly has the same problems as connecting to signals in each individual script, you have to know exactly where that script is instanced and navigate to a possibly obscure place (obviously not a problem for autoloads, but for arbitrary nodes like the map container in main.tscn). Unless there's a better way than signals to avoid this, it might end up kind of locking us into a specific node structure?

I've been thinking about it as keeping each step decoupled from each other in that the GameManager or GameController and the rest of the system doesn't need to know anything about said step. For instance, the map loading step would be signaled, and after that you can just assume everything was done without ever needing to know specifics about what happens (directly calling functions needs at least some knowledge of how the map system works).

I'm interpreting your last paragraph as having another intermediary step in between GameManager and what actually effects the game. Having only one object that actually does stuff (GameController) seems like it's just shifting the crux point to a different place. If we're directly calling functions, having a controller for each overarching system would probably be better because it's limiting how deep GameManager/GameController's tendrils go and it removes the need for one central script to have deeper knowledge about every system.

Signals seem like the easiest way to make stuff happen without direct knowledge of other systems' internals.

[nicemicro]

I see both of your points, and without the fullest grasp of the "observer pattern", I guess, from the philosophical standpoint, it's about how we look at the role each script plays in the process of state change, isn't it? I mean, if I look at the state change as something that requires the process of some well-defined steps in a well-defined order, than it's not really suitable doing with signals?
So @monban , can you elaborate a little more on this? what would be your suggestion? having statechange related functions in all relevant scripts, and just calling NetworkManager.stateChange() and then MapManager.stateChange() etc.?

[TheSecondReal0]

Signals are functionally equivalent to directly calling the connected functions. When you call emit_signal("do_stuff"), it's as if you're calling the first function connected to that signal, then the second, third, and so on until every connected function has been called. Because of this, I think that signals are perfectly suited for processes that need a specific order.

You don't have very granular control over the order of calls within the same signal emit, but I think that the loss of some control is far outweighed by the benefit of decoupling GameManager from individual systems/scripts. If you really need that kind of control, you could always connect the state_changed signal to MapManager.stateChange() and directly call the related functions from within each system.

[TheSecondReal0]

If we end up keeping the signals, we can improve it by moving away from sending arbitrary numbers in state_changed_priority to emitting individual signals. It would be very similar to a priority enum, but the signal itself would have a descriptive name. You wouldn't have to fuss about with each node referencing an enum in GameManager and connecting to a unique signal is much more user/developer friendly to use (connecting to assign_roles is much easier than connecting to state_changed_priority and checking against an enum) and to read (you know exactly what happens when you see GameManager emit assign_roles and what a function connected to it will do without referencing an enum).

It also means you're not calling a function in literally every node that uses a priority thing (which is what happens currently).

It comes with all the benefits of a priority enum without the inherent jankiness of the current state_changed_priority signal. You could still have custom orders/steps for different transitions (don't need to emit map loading signal when you're transitioning to a meeting), it's easy to reorder, and easier to think about than an arbitrary int. It comes with the drawback of having to declare many signals in GameManager, but the readability and usability might be worth it.

It doesn't really go with the idea that signals notify about stuff that just happened, but why should we constrain ourselves to a single use of such a powerful tool?