Howdy!
I’ve been really enjoying the “Unity Multiplayer” course so far. I think the way you present networking concepts is excellent, and I think the choice of Mirror was a fantastic one. But I feel like I should give some feedback.
My background: I’ve been a professional software engineer over a decade, and I have one of those expensive pieces of toilet paper called a degree in Computer Science as well.
I feel like the Unity Mulitplayer coures’ architecture could use another pass. Please take this criticism as it is meant: As some friendly advice! 
Also, in the end, I understand that this whole project is a means to an end ( we are learning the basics of Unity and Network programming, not launching a commercial game, after all ) and obsessing about the architecture of this project is probably overkill. I just hope my code review here is educational for some readers!
My concern started when in the first part of the lecture series when we named a bunch of modules “MyNetworkManager” and “MyNetworkPlayer”. These names are obviously bad, adding the word “My” to these names adds no additional information. Names are important! We want them to be descriptive! No big deal. This is a small section and we quickly moved on. However, we repeat the pattern with “myUnits” in the RTSPlayer class. Having good hygiene with our naming is really low hanging fruit and not something we should blaze past in the moment.
More concerning is the road we are going down on the RTS game. Mainly, I feel like our classes are a bit bulky and are doing too much. Additionally, To follow along at home I am reviewing this code: https://gitlab.com/GameDevTV/UnityMultiplayer/RealTimeStrategy/-/tree/bab714ef532352c38d16f24af2ce2d00dc34634d/Assets/Scripts/Units
I want to point out that we are building an RTS game. We shouldn’t be hesitant to introduce new classes and utilities to assist us with common tasks like casting rays from the camera, and checking what is under the mouse cursor. Right now I feel like we have a lot of logic spread around our classes, and a cleaner architecture is possible.
Right now I have a number of classes:
- Targetable
- Targeter
- Unit
- UnitCommandGiver
- UnitMovement
- UnitSelectionHandler
The Targeter has a targetable. This checks out. Targeter also has a function called “CmdSetTarget” to set the target of the Targeter. I think this is quite nice.
This is where things get a little hairy. UnitMovement has a target property. It also has a function called CmdMove that calls the target.ClearTarget() function on that Unit’s target:
[Command]
public void CmdMove(Vector3 position)
{
targeter.ClearTarget();
if (!NavMesh.SamplePosition(position, out NavMeshHit hit, 1f, NavMesh.AllAreas)) { return; }
agent.SetDestination(hit.position);
}
So issue here is that now CmdMove is no longer just in charge of moving. This bubbles up to the UnitMovement class in general. It isn’t the UnitMovement class, it is the UnitMovementAndAlsoClearTarget class. I feel like using an event here would be MUCH cleaner, as it would invert the dependency of the UnitMovement and Targeter classes. That way, if we change the way we target units, we won’t also have to change the way we move units.
To pick on CmdMove a bit further: the NavMesh check here is valuable and I think it is the right place for it. However, we have a LOT of guard clauses around this codebase and I think the code suffers from it. You shouldn’t just check every parameter that comes into every function, that is going to create a mess. Over hundreds of functions and thousands of lines of code, these things add up quickly. Instead you should use other tools. Specifically:
- Create a type that represents the validations you desire. If you make a class that validates that the parameter meets the requirements during construction, you can pass that value around and trust that it is doing what you need it to. This is the one you should be reaching for!
- Create a perimeter around your core business logic past which you can trust your input. This is more of a dynamic language thing. Since we have types in C#, we should use those. But in some cases, where we are calling
if (!hasAuthority) {return;}we should really see if we can just not call the function in the first place.
The UnitCommandGiver class isn’t so bad, but I see some problems with the Update function:
private void Update()
{
if (!Mouse.current.rightButton.wasPressedThisFrame) { return; }
Ray ray = mainCamera.ScreenPointToRay(Mouse.current.position.ReadValue());
if (!Physics.Raycast(ray, out RaycastHit hit, Mathf.Infinity, layerMask)) { return; }
if (hit.collider.TryGetComponent<Targetable>(out Targetable target))
{
if (target.hasAuthority)
{
TryMove(hit.point);
return;
}
TryTarget(target);
return;
}
TryMove(hit.point);
}
Ad-hoc checking inputs like this is going down a bad road. A centralized service to manage our inputs and map them into commands would be better architecture. It would also allow for control remapping in the future. Think about it this way: What if our user wanted to switch the right mouse and left mouse button? We would have to hunt down every usage of leftMouse and rightMouse and update it in dozens of places. This will inevitably be more error prone, and is something software engineers want to avoid at all cost.
Moving on to UnitSelectionHandler: it is doing way too much. It is handling the drawing and dimensions of our selection box, calling out to our individual units, updating or player’s selected units, and deciphering the player’s currently pressed keys to figure out what stage of selection we are in.
I feel like it is more of a UnitSelectionBoxHandler, and we are again doing ad-hoc checks against our keyboard and mouse to see what command we need to execute, instead of wrapping that state in a type and representing it in a readable way. For example, instead of:
if (Mouse.current.leftButton.wasPressedThisFrame) {
if(!Keyboard.current.leftShiftKey.isPressed) {
...do your thing...
}
}
We could have something like:
SelectionState selectionState = InputManager.getSelectionState(Mouse.current, Keyboard.current);
switch (selection) {
case SelectionState.MultipleUnitSelect:
doMultiSelect();
break;
case SelectionState.SingleUnitSelect:
doSingleUnitSelect();
break;
case SelectionState.FinalizeUnitSelection:
finalizeUnitSelection();
break;
}
That way, when we are looking at the UnitSelectionHandler class, we can hide information, so we can concentrate on the logic around selecting the unit, not following a trail of if-statements and try to decipher what our program is doing and when.
I know these points seem simple, but that is the thing about software: You write it line by line, and these “simple” things add up into an unmanageable system.
Lets go over ClearSelectionArea. I will add comments to this code in order to point out things I feel could be improved:
private void ClearSelectionArea()
{
// 1
// 2
unitSelectionArea.gameObject.SetActive(false);
// 3
if (unitSelectionArea.sizeDelta.magnitude == 0)
{
// 4
Ray ray = mainCamera.ScreenPointToRay(Mouse.current.position.ReadValue());
if (!Physics.Raycast(ray, out RaycastHit hit, Mathf.Infinity, layerMask)) { return; }
if (!hit.collider.TryGetComponent<Unit>(out Unit unit)) { return; }
// 5
if (!unit.hasAuthority) { return; }
SelectedUnits.Add(unit);
foreach (Unit selectedUnit in SelectedUnits)
{
selectedUnit.Select();
}
return;
}
// 6
Vector2 min = unitSelectionArea.anchoredPosition - (unitSelectionArea.sizeDelta / 2);
Vector2 max = unitSelectionArea.anchoredPosition + (unitSelectionArea.sizeDelta / 2);
foreach (Unit unit in player.GetMyUnits())
{
if (SelectedUnits.Contains(unit)) { continue; }
Vector3 screenPosition = mainCamera.WorldToScreenPoint(unit.transform.position);
// 7
if (screenPosition.x > min.x &&
screenPosition.x < max.x &&
screenPosition.y > min.y &&
screenPosition.y < max.y)
{
SelectedUnits.Add(unit);
unit.Select();
}
}
}
- Here we are reaching into he unitSelectionArea and deactivating it.
- It is clear to me from all of the ad-hoc operations we are performing on this unitSelectionArea that it is an important concept for our game. So we should encapsulate this logic in a class, and name the operations we are performing on it in a human readable matter.
- The code within this if statement is not clearing the selection area. It is seeing if you are clicking directly on a unit, and selecting it. So why is the function named “ClearSelectionArea”? Answer: Because it shouldn’t be, it needs to be refactored… It will be hard to name this function properly because it is doing so much.
- We have seen this sort of logic of casting rays from the camera to check what we are clicking on before. Don’t repeat yourself! Find a home for this domain!
- This smells to me. How are we this deep into our call-stack and still unsure if we have authority on this unit? When you see shotgun validation like this, it should be a warning sign that something isn’t meshing with your current architecture.
- This logic doesn’t seem complex right now, but wait a week, add a dozen new classes, and add a few thousands lines of code and you will wish this was in a function called
CalculateSelectionAreawith a nice doc string. - Similar to above, this logic would best be contained in it’s own function with a good name. Heck, if you had a
SelectionAreaclass, you could write aisUnitInSelectionAreafunction and hide all of this information
So does this function clear the selection area? Not really. It:
- Deactivates the selection area
- Conditionally selects a single unit if the sizeDelta is 0
- Calculating the selection area
- Checking to see if the unit is in the selection area
- Selecting the unit
One could argue this function clears nothing. And this function contains logic that will probably be used again elsewhere in the game and handles logic for a lot of different domains. I think having a SelectionBoxHandler is the right move, but this class needs to go on a diet!
Finally, I will admit I have not taken the time to draw out the relationships between our classes, but I would not be surprised to we have a two-way dependency somewhere in our tree.
Understandability is an important facet of software! Understandable code is more secure, more reliable, and makes it possible to continue to add features to our systems.
Anyway, this is long enough as it is. I hope you find this feedback helpful!