Problems with the Original

This page is part of the 3D Game Kit example.

The character implemented in this example is directly based on the player character from the 3D Game Kit, but it is not an exact copy because the original had several problems which we want to avoid.

Locomotion

The character's Locomotion has several issues that make it feel unresponsive:

  • Lack of Full Movement Control means the character will move in a direction the player doesn't want while turning towards the desired direction.
  • The left and right quick turn animations have different import settings (one loops, the other doesn't) and different transition settings. It is unknown whether there is actually a good reason for this or if it was just an oversight.
  • You have limited Acceleration. This isn't necessarily a "problem" as such, but the implementation is not physically consistent and it is unusual for a character using Unity's CharacterController component instead of a Rigidbody.
  • Sometimes when you jump, the character stays in the Idle animation instead of entering the Airborne Blend Tree properly so it looks like they are just floating upwards for no reason (watch the legs):

Groundedness

The CharacterController component is a capsule shape and it is possible to get the edge of the bottom curve of your capsule on top of a corner, causing it to react as if you were properly on the ground even though the center of the character is actually standing in mid air. If you get close enough to the edge, it can even be far enough down the curve that you can't even move onto the platform because it is too steep, meaning it is treating that edge as both the ground and a wall at the same time.

Since this is a purely physics problem, it is not addressed by this example, but it is worth noting when considering how to implement a character for your own game.

Input

There are several issues with the way player input is handled:

  • The PlayerController only reacts to jump input in FixedUpdate, meaning that quick keypresses can be missed if multiple Updates occur between them (if a key is pressed on the first Update, then released on the second, the following FixedUpdate will not detect that keypress). This might seem obvious, but randomly ignoring legitimate player input is not good.
  • You can't attack during the quick turn animations. This is another arbitrary lack of control. You can almost always attack when on the ground, except when you happen to turn sharp enough while moving quickly enough to trigger one of the quick turn animations, in which case you suddenly can't attack for a short time.
  • You also can't attack during the exit transition of a previous attack. There's a very small window time right before that where pressing the button again will execute the next attack in the combo, but then the attack ends and any attack inputs are ignored until you finish returning to Idle.
  • The logical flow from pressing a button to executing the desired action (such as jumping or attacking) is extremely disorganised and hard to follow.

Splitting PlayerInput away from the rest of the PlayerController is good for separation of concerns, however some of the concerns ended up in the wrong place:

  • Converting the player's desired movement direction into world space should be done in PlayerInput. That way if a different input system is used - such as for example, recording events and then replaying them later on or using AI to control the character - then that system gets to decide exactly how it wants to move the player instead of having the player interpret everything it says to be relative to the camera.
  • The LocomotionSM has an arbitrary input "dead zone" in the conditions of its transitions where any speed below 0.1 is treated as not moving. Again, this is something that should be handled as part of the input system so it can be easily customised, not hidden as a bunch of magic numbers in half a dozen different Animator Controller transitions.
  • It contains a public static PlayerInput Instance singleton property that it assigns in its own Awake method so other scripts can access it through that property. But for some unknown reason, some scripts use Object.FindObjectOfType to find it themselves and the PlayerController uses GetComponent to find it. It's possible that there's a genuine reason for these differences, but it's unlikely and without comments there's no easy way to tell.

Audio

The original character uses a RandomAudioPlayer script to take care of playing randomly selected sounds and also provide a different selection depending on the Material of the object the character is standing on. In particular, this is used to have footstep sounds that eash sound slightly different and have a different set of sounds for different surfaces (stone, grass, etc.).

The concept was sound, but the implementation had one notable flaw which was corrected when copying it over to the Animancer character: the playing and canPlay fields:

[HideInInspector]
public bool playing;
[HideInInspector]
public bool canPlay;
  • The [HideInInspector] attribute does what it says: it prevents the field from showing up in the Inspector. But it does not actually prevent the value from being saved as part of the scene. This means a script could set the value in the Unity Editor and you would never know why it was in the wrong state on startup (and it would waste performance loading that value). Instead, the [NonSerialized] attribute should have been used to prevent them from being saved at all.
    • Alternatively, they could have been auto-properties which do not get serialized anyway: public bool Playing { get; set; }.
  • But the real issue is that they should not even be in this class in the first place. The RandomAudioPlayer class doesn't use them itself and of the several dozen places throughout the project that use it, the only place that actually uses these two fields is in the PlayerController.PlayAudio method for the footstep sounds so there is no reason for them not to be in that class instead.

Naming Conventions

There are also a number of issues with the quality of the code and project structure in the 3D Game Kit. These barely even qualify as minor problems, but they are worth noting in a product that was made by Unity Technologies and should thus be held to a high standard.

It doesn't matter so much whether you use m_Name for private fields or camelCase for properties or whatever specific naming convention you use. The most important things are to use a meaningful convention and to apply it consistently.

For a naming convention to be meaningful, it should provide or emphasize useful information that isn't already easily accessible:

  • In the player's Animator Controller, the main sub state machines have an SM suffix which differentiates them from other states (IdleSM, LocomotionSM, etc.). This is totally unnecessary since sub state machines are already clearly differentiated by having a hexagon shape instead of being rectangular.
    • StateMachineBehaviours have a similarly pointless SMB suffix.
  • Conversely, the Blend Trees do not have any particular suffix to differentiate them even though they look the same as regular states. This would have been a good situation for a naming rule to cover for a deficiency in the user interface.

Applying rules consistently is also important because variations imply a difference of intent:

  • The PlayerInput script in particular has several notable issues:
    • The protected field m_Movement is exposed publicly by the MoveInput property. Shortening from Movement to Move just seems lazy, but the Input suffix is unnecessary in a class which is already called PlayerInput.
    • MoveInput, CameraInput, and JumpInput all have that suffix, but then Attack and Pause don't. This implies something different about those groups of properties, but upon investigation that doesn't seem to be the case.
    • The CameraInput property is never used (camera rotation seems to be handled by Cinemachine). Unused code should generally be removed to avoid wasting people's time when they are trying to figure out how things work.
  • The MeleeWeapon.PARTICLE_COUNT constant also deserves a mention because other constants use the k_PascalCase convention.
  • There are plenty of places in the code and the Animator Controller that specifically mention the character's name (Ellen), which is silly because the name of the character has no bearing on their logic.
    • EllenRunForwardLandingFast is fine as the name of an AnimationClip because it's specific to that character model and giving her a name makes it easier to remember than something like "Character 3". But it should not also be used for the name of the Animator Controller state that it is used in. That state should have a general name like "Hard Landing" to describe what it is actually used for.