Hi, I’m new to Unity, currently learning from tutorials and projects and almost everywhere I see something like this
public class Enemy : MonoBehaviour
{
public int health = 100;
void Update ()
{ // Why we check hp for every object in every update?
if(health <= 0)
{
Death ();
}
}
public void TakeDamage (int amount)
{
health -= amount;
}
}
Why don’t we check hp only when object take damage?
public class Enemy : MonoBehaviour, IDamageable
{
public int health = 100;
public void TakeDamage (int amount)
{
health -= amount;
if(health <= 0 && !isDead)
{
Death ();
}
}
}
I saw that on enemies, crates, etc. but I don’t understand why we have to check for example some crate that might never get damaged.
I don’t know how exactly Unity works so maybe both examples can be used in different situations, but I would like to know when to use first and when the other.
It’s very sloppy coding. You’re right, it isn’t necessary and usually you can get away with it, but on mobile those are the things you look for to optimize your scripts as much as possible. Generally speaking, you should try to avoid using Update as much as possible as it is a constant performance drag (unless it’s absolutely necessary or you’re using a bool to use it only when you need it).
There is one advantage to the Update check that’s worth mentioning. If you are changing health in a lot of different places, then it can be easier to control death through a central check. In some designs, checking once in Update might be more efficient then checking multiple times throughout the frame.
Not a super common scenario, but worth pointing out.
I notice that “health” here is public. Thus, anybody and their brother could be mucking with it. In such an unfriendly world of code, checking for death on every frame is probably a good idea.
But much better would be to make health private, and then have a public TakeDamage method like you showed (applies the damage and checks for death). If other scripts really have a need to know how much health you have, you can provide a public read-only property for that.
A heads up: While the Unity tutorials are pretty decent at teaching Unity, they assume that you don’t know how to program. So the code is very… coding beginner friendly. Which is to say it’s often full of horrible shortcuts that are easier to understand when you haven’t seen code before, but are bad ideas in the long run.
having something’s health be a public variable is an excellent example of that.
Essentially, your tutorial is wrong. Even for a pet example, the code should be:
public int maxHealth;
private in currentHealth;
void Start() {
currentHealth = maxHealth;
}
I made it public for easy changes in Unity while coding, so thanks for [SerializeField].
So… if I understand it right, this would be usually ok:
When I directly work with hp from other objects (like taking damage and healing) or more methods use it, death should be check in hp property setter
When I use private hp only by TakeDamage() (through interface) it’s best to check death there
But when object can take damage from multiple sources I can still use TakeDamage(), Heal() (through interfaces) but it would be better to check death in Update()
So for example I should use TakeDamage() when directly damaging object (crate, enemy) property when target can get hit, damage over time, area damage, heal,… but when there are too many of those I should check it in Update (usually for player I guess)
Never check death by using a properties setter. A setter is meant for just that, setting the value. The only logic that should be in a setter is a sanity check to make sure the value entered is allowed to, and can, be set. You can use a PropertyChangedEvent though. But best is to have a seperate TakeDamage() function.
Would it not be useful then as an extra to do that check in LateUpdate()? As you can be sure most if not all damage dealing processes are done at that time.
So could this be a good base class for any player, enemy or destroyable object?
public class Character : MonoBehaviour, IDamageable
{
[SerializeField]
protected int maxHP = 100;
[SerializeField]
private int _HP;
protected virtual int HP
{
get { return _HP; }
set
{
if (value != _HP)
{
_HP = value < maxHP ?
value : maxHP;
}
}
}
protected virtual void Awake()
{
HP = maxHP;
}
public virtual void TakeDamage(int amount)
{
HP -= amount;
if (_HP <= 0)
{
Death();
}
}
protected virtual void Death()
{
Destroy(gameObject);
}
}
And does that mean I shouldn’t run anything in property setter like this? Because PropertyChangedEvent looks easy on WPF binding but I don’t understand how would I use it in Unity.
public class Player : Character, IDamageable, IHealable {
protected override int HP
{
get { return base.HP; }
set
{
if (value != base.HP)
{
base.HP = value < maxHP ?
value : maxHP;
UpdateHealthBar(); //here, instead of calling after every hp change?
}
}
}
public int Health
{
get { return base.HP; }
}
protected override void Awake()
{
base.Awake();
UpdateHealthBar(); //like here
}
public void Heal(int amount)
{
HP += amount;
UpdateHealthBar(); // here
}
public override void TakeDamage(int damage)
{
HP -= damage;
UpdateHealthBar(); // here and after every other hp change
if (HP <= 0 && !isDead)
{
Death();
}
}
private void UpdateHealthBar()
{
//do some magic
}
}
You shouldn’t be serializing your current HP - or any value you’re setting in Awake. If you want to show it in the inspector for debugging purposes, made the getter public, and write a custom inspector to show that instead.
The setter is also a lot prettier with a Clamp:
set
{
_HP = Mathf.Clamp(value, 0, maxHP);
}
Not quite sure why you’re doing the if-check?
@Omni_Owl is also correct. Instead of subclassing Character for the Player, you could attach a Character to the same object as the Player, and use events to communicate the Character’s state to the Player. You’d also probably name it “Health” instead of “Character”. So something like:
public class Health : MonoBehaviour {
//Assigning an empty delegate here to avoid needing null-checks later.
public event Action OnHealthChanged = delegate { };
[SerializeField]
protected int maxHP = 100;
private int _HP;
public int HP {
get { return _HP; }
protected set {
var old = _HP;
_HP = Mathf.Clamp(value, 0, maxHP);
if (old != _HP) {
OnHealthChanged();
}
}
}
}
...
public class Player : MonoBehaviour {
...
void Start() {
var health = GetComponent<Health>();
health.OnHealthChanged += UpdateHealthBar;
}
}
Similarly, you could fire an OnDead event in TakeDamage and make Player react to that.
Yes Clamp looks prettier and delegates looks pretty neat, I had no idea that something like this is possible. So thank you for that code, it really helped me understand.