Post by andrew-raphael-lukasik in Space Architect comments

Viewing post in Space Architect comments

andrew-raphael-lukasik5 years ago (5 edits)

Few words of friendly comments:

void Update ()
{
    // *** - ToString allocates at least 2 bytes for every character
    // **** - adding strings together with '+' operator is more or less equivalet to ***
    ItemContainer container = SingletonMB<StationContent>.Instance.GetContainer();
    TextMeshProUGUI title = this.title;// this does nothing - remove line
    float num = container.CapacityUsed;
    string str1 = num.ToString("0");// ***
    num = container.Capacity;// reusing local variables is not needed for anything
    string str2 = num.ToString("0");// ***
    string str3 = "Items " + str1 + " / " + str2;// ****
    title.text = str3;
    this.sectionLines.RemoveAllLines();
    foreach( ItemStack itemStack in container.Items )// unfortunately, starting 'foreach' allocates memory - in Update methods try to use simpler old 'for' iterator instead of 'foreach'
    {
        PanelSectionLines sectionLines = this.sectionLines;// this does nothing - remove line
        ItemType type = itemStack.Type;
        string _identifier = type.ToString();// ***
        type = itemStack.Type;// this does nothing - remove line
        string _text = type.ToString() + " " + itemStack.Amount.ToString("##");// ****
        sectionLines.Set(_identifier, _text);
    }
}

Downsides of ToString() calls are not that simple to get rid of in context of UI. But it's better that you're aware of it and step-by-step will be trying to avoid them in the future (strings in general).

andrew-raphael-lukasik5 years ago (5 edits)

After cleaning code up just a bit it can be like this (cleaner to read/understand imho):

void Update ()
{
    ItemContainer container = SingletonMB<StationContent>.Instance.GetContainer();
    title.text = string.Format( "Items {0:0} / {1:0}" , container.CapacityUsed , container.Capacity );
    sectionLines.RemoveAllLines();
    int numContainerItems = container.Items.Count;
    for( int i=0 ; i<numContainerItems ; i++ )
    {
        ItemStack itemStack = container.Items[i];
        string identifier = itemStack.Type.ToString();
        string text = string.Format( "{0} {1:##}" , identifier , itemStack.Amount );
        sectionLines.Set( identifier , text );
    }
}

Note that 'string.Format()' is slightly better than '"strA"+"strB"'

Also System.Text.StringBuilder would be better (string.Format uses it internally).

andrew-raphael-lukasik5 years ago (3 edits)

To give you an example of how to get rid of string allocations think about creating lookup tables for your values (enum is easy and natural candidate for that). This is how you create an lookup table for enums:

public static readonly Dictionary<ItemType,string> ItemTypeStrings = new Dictionary<ItemType,string>{
            { ItemType.Metal, "Metal" } ,
            { ItemType.Food, "Food" } ,
            { ItemType.Water, "Water" } ,
            { ItemType.Mineral, "Mineral" } ,
            { ItemType.Dirt, "Dirt" } ,
            { ItemType.WasteWater, "Waste Water" } ,
            { ItemType.LOX, "LOX" } ,
            { ItemType.LH2, "LH2" } ,
            { ItemType.AsteroidaldWater, "Asteroidald Water" } ,
            { ItemType.MetalOre, "Metal Ore" } ,
            { ItemType.Component , "Component" } ,
            { ItemType.None , "None" } ,
        };

And that will give you an ability to replace every:

string myTypeString = itemStack.Type.ToString();//allocation

with:

string myTypeString = ItemTypeStrings[ itemStack.Type ];

which will allocate no memory at all (==what we want)

Tazoru5 years ago (4 edits) (+1)

This part of code is just my prototyping/lazy programming left for later :D I've wanted to base it on event instead of leaving it in update, but forgot about it when preparing actual event :/

Adding your suggestions (with exception for foreach, just for readability sake) and moving it to be based on event it now looks and perform a lot better :)

void Start()
{
    sectionLines = GetComponent<PanelSectionLines>();
    title = transform.Find("Title").GetComponentInChildren<TextMeshProUGUI>();
    StationContent.Instance.OnContentChanged += HandleStationContentChanged;
}
private void HandleStationContentChanged(StationContent _stationContent)
{
    ItemContainer container = _stationContent.GetContainer();
    title.text = string.Format("Items {0:0} / {1:0}", container.CapacityUsed, container.Capacity); 
    sectionLines.RemoveAllLines();
    foreach (ItemStack stack in container.Items)
    {
       sectionLines.Set(Lang.ITEM_TYPE[stack.Type], string.Format("{0} {1:##}", Lang.ITEM_TYPE[stack.Type], stack.Amount));
    }
}

andrew-raphael-lukasik5 years ago (5 edits)

That's great!

PS: Please don't over-rely on events (outside UI ofc). Those are 10x times slower and complicate future debugging considerably. If something can be just a method call - let it be just that, you will thank yourself for that later on (less time spent debugging==more productivity==happier you).

itch.io

Viewing post in Space Architect comments