• entries
    27
  • comments
    62
  • views
    24998

Code Refactor - Made a bad design choice?

Sign in to follow this  

847 views

Last time I made an entry I said that my next task would be to have enemies drop items, and to create enemy spawners. When I opened my project, I started looking through my code and wound up refactoring quite a bit of it.

By the end I had 3 or 4 new classes, making all of my classes quite a bit leaner and more maintainable. However, one of the solutions I came up with to better handle UI updates still doesn't feel quite right.

My UI basically has 4 main elements: Health, Experience, Weapons and Money. The first three utilize a slider to visualize % of health remaining, % XP to next level and % of weapon capacity currently loaded, respectively. The fourth simply shows the user's current amount of cash (Placeholder icons):

bj8rYxl.png

I had two main ideas for a solution. The first was to create a class for each group of UI elements (one for health, one for XP, etc) and to simply have them update the sliders and text each and very frame using a reference to either my Stats (of which there is only one of in the game, as an instance) or WeaponController (of which there is one for each different weapon) objects. Stats holds player variables like health and XP, while WeaponController (which maybe should be split into Weapon and WeaponController classes) contains weapon variables like capacity, roundsLoaded and roundsOwned.

This, to me, seemed like the optimal design choice. It had the least amount of complexity and seemed like it could cause the fewest number of problems. Essentially, the UI elements would stand alone, and even if they were removed from the game, nothing would break. Also, it would be the easiest to write this way.

My second option, the one I chose, was a little more complex, but, I think, more efficient. However, as I'm writing this, I'm beginning to think I made the wrong choice. The reason I even came up with this option was because I was thinking about efficiency. Having the UI elements just update 'magically' on every frame is easy and clean, but maybe a little wasteful. I decided that it would be more efficient to have the UI elements only update when needed, i.e. when stats change.

At the time, it seemed to me that good design is just fine, but it should be tossed out if a more efficient solution exists. Now, I realize that the complexity of simply updating UI elements isn't going to be very CPU intensive, but I figured every little bit of CPU savings helps, so this is the solution I went with. I still have a class for each UI group, as in my first solution, but these classes do not have references to my instance(s) of Stats or WeaponController. Instead, Stats has a reference to HealthUI, CashUI and XPUI, and WeaponController has a reference to WeaponUI.

Any time a player takes damage, gains cash or gains experience, the related code winds up leading to methods in Stats, which complete all the appropriate actions, including making calls to the UpdateUI() method in the relevent UI Manager object (HealthUI, CashUI, etc), passing itself as an argument. So, for example, Stats' TakeDamage() method looks something like this:public void TakeDamage(){ //do some stuff ... healthUI.UpdateUI(this);}
The same goes for the calls to the other UI Manager objects. They all take a reference to Stats as an argument (except for WeaponUI).


WeaponController functions similarly, passing itself as an argument in its calls to WeaponUI's UpdateUI() method.

Thinking about this now, it seems like I've chosen an overly complicated solution with little to gain from it (only slight efficiency benefits), and it seems like the cons far outweight the pros (for example, removing the UI elements will now break the game). Also, it stinks of circular dependency.

I think I've answered my own question, but what do you think? Which option would you have chosen, and do you have any better solutions I didn't think of?

I was going to post some questions about the design of my game's enemy spawning mechanic as well (yet to be implemented), but this post has gone on long enough, so I guess I'll make a second entry for that this evening.

Sign in to follow this  


3 Comments


Recommended Comments

I remember facing a similiar problem when I tried to update UI elements. I always tried a "only update when needed" approach instead of updating every frame, because otherwise it would be "bad" code.

 

What a pain in the ass. It's a nightmare to code right and it doesn't make any significant gain in performance. Once I realized that drawing stuff on screen is what takes 90% of a frame time, I stopped worrying about things like that.

 

Just update every frame, it will be better for your mental health. If you still want to be a little more efficient, in your "update" method check if the current value is different than the new value, and if so, do the update stuff, otherwise nothing happens.

Share this comment


Link to comment

WARNING: these are just some ideas for giving advice to achieve a flexible software design regarding UI. It may not be helpful at all, and it became pretty lengthy smile.png...
The code itself is really really pseudo-C#-ish stuff, forgive me for possible mistakes.

I think you should not be concerned too much regarding the performance implications of the presentation layer / UI elements.
If you think about it, in a non-UI-heavy game you have like a couple of dozen elements at max, and that is not much smile.png.
Still I would like to propose the design how I approach UI, because I think it has some common ideas which should be followed (except when it is absolutely illogical biggrin.png), and could help you out.
I think of the UI elements as "Views" ala model view controller design, where UI elements themselves do not have state! A little fib here, they are allowed to have state, like where the button is on screen, or how big the slider is, or the back color of a check-box drawn, but if they are used strictly for presentation, it is much much cleaner, that they do not copy or own the presented data.

Here is a simple example tailored to your case:

Your data and logic types, a.k.a "Model":

public class Player {
    public int MaxHealth { get; private set; }
    public int Health { get; private set; }
}

public abstract class Weapon {
    public int CurrentClip { get; private set; }
    public int ClipSize { get; private set; }
    public int Ammo { get; private set; }
}

The abstract "adaptor" to connect/bind your generic UI elements to specific data (another common fancy name is data-binder). This little class defines how a generic UI bar should present specific types of data.

public abstract class UIBarAdaptor {
    public abstract float Percentage { get; }
    
    public abstract string DataText { get; }
}

A generic bar element, which draws a rectangular border, a rectangular bar and a text over it.
The way the bar is presented (width %) and the text to draw is provided by a concrete adaptor implementation.

public class UIBar {
    public UIBarAdaptor Adaptor;
    
    public Rectangle FullBar { get; set; }
    
    public Color BorderColor { get; set; }
    public Color BarColor { get; set; }
    public Color TextColor { get; set; }
    
    public void Draw(Graphics g) {
        g.DrawRectangleBorder(this.FullBar, this.BorderColor);
        
        var barRectangle = new Rectangle(this.FullBar);
        barRectangle.Width = this.FullBar.Width * this.Adaptor.Percentage;
        g.DrawRectangle(barRectangle, this.BarColor);
        
        g.DrawString(
            this.FullBar.Position, this.Adaptor.DataText, this.TextColor
        );
    }
}

And finally here they are, two simple adaptor implementations, to present the player health and the ammo of the current weapon on a UI bar.

public class HealthBarAdaptor : UIBarAdaptor {
    public Player Player { get; set; }
    
    public override float Percentage {
        get { return (float)this.Player.Health / (float)this.Player.MaxHealth; }
    }
    
    public override string DataText {
        get {
            return string.Format("{0}/{1} ({2}%)", this.Player.Health, this.Player.MaxHealth, this.Percentage);
        }
    }
}

public class AmmoBarAdaptor : UIBarAdaptor {
    public Weapon Weapon { get; set; }
    
    public override float Percentage {
        get { return (float)this.Weapon.CurrentClip / (float)this.Weapon.ClipSize; }
    }
    
    public override string DataText {
        get {
            return string.Format("{0}/{1} ({2})", this.Weapon.CurrentClip, this.Weapon.ClipSize, this.Weapon.Ammo);
        }
    }
}

What advantages you get this way:

  • You do not need to "Update" and keep in sync most of your UI with your actual game-data.
  • The actual UI elements don't need to know about the data they present, they are just generic UI elements.
    It is easier this way to hold generic layouts in file, or to compose a UI based on looks/designs without even touching any game logic code.
  • Your game-data and logic is decoupled from the UI too. It doesn't have to bother with the UI elements at all, which is logical, since a weapon is a weapon, and not a weapon + a UI element to show the state of the weapon!
  • No separate "Update" is happening for the UI, you only have to present your data with the UI elements each frame, which logic is actually implemented as simple glue code.

If you are concerned with the performance of the presentation part, or profiling actually shows, that it is memory or CPU heavy you can easily cache the data text or whatever you need in the adaptor as an example, with simple lambdas/events, still getting away without a per-frame update call!

public class Player {
    private int health;
    
    public Action HealthChanged { get; set; }
    
    public int MaxHealth { get; private set; }
    public int Health
    {
        get { return this.health; }
        private set
        {
            this.health = value;
            if (this.HealthChanged != null)
            {
                this.HealthChanged();
            }
        }
    }
}

public class HealthBarAdaptor : UIBarAdaptor {
    private string dataText;
    
    public HealthBarAdaptor(Player player) {
        this.Player = player;
        player.HealthChanged += this.RefreshDataText;
        RefreshDataText();
    }
    
    private void RefreshDataText() {
        this.dataText = string.Format("{0}/{1} ({2}%)", this.Player.Health, this.Player.MaxHealth, this.Percentage);
    }
    
    public Player Player { get; private set; }
    
    public override float Percentage {
        get { return (float)this.Player.Health / (float)this.Player.MaxHealth; }
    }
    
    public override string DataText {
        get { return this.dataText; }
    }
}

Another last trick, is that you can still support stateful UI elements with a simple non-binding adaptor. You modify the fields/data of an instance of this class directly, to modify the presented data on a UI element:

public class UIBarData : UIBarAdaptor {
    public int Count;
    public int MaxCount;
    public int AllCount;
    
    public override float Percentage {
        get { return (float)this.Count / (float)this.MaxCount; }
    }
    
    public override string DataText {
        get {
            return string.Format("{0}/{1} ({2})", this.Count, this.MaxCount, this.AllCount);
        }
    }
}

No need take these constructs/plans too seriously and especially no need to become overzealous and start rewriting your UI code for the third time wink.png ! If it already works don't change it, if you need to make it more flexible or robust, draw some ideas from here if something may work for your game wink.pngsmile.png!
 

Share this comment


Link to comment

You probably aren't updating values in as many places as you think.  For example, player health should only be modified in the player class.  That makes it fairly easily to funnel all player health changes through one or two functions.  Either one generic "ChangeHealth(changeAmount)", or TakeDamage(damageAmount) paired with "HealDamage(damageAmount).  Then it's very easy to have a function that notifies the UI, or raises an event, or whatever.  For a simple game like what you have, you might just want to have a [serializefield] HealthSlider that you assign to your player, or you can go with an event system, or delegates, but it's probably overkill currently.

 

The same goes for Money, Ammo and Experience, they should all be updated by only one or two functions.  That makes life so much easier for things like determining when a player needs to level up, or what have you.

Share this comment


Link to comment

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now