I’m wondering what’s the best way to handle this design issue
Imagine this scenario:
I have a class MyObject. All the objects of the inventory in my game are of this type.
Now, there are many clases of objects, say MyWeapon, MyArmor, etc., that derive from that base class (MyObject).
Now, when I handle the character’s inventory, I have a list for every inherited type of object: one for the weapons, one for the armors, etc. The purpose of having all in different lists, is that if I need to evaluate what’s the best armor, I just look into that list and that’s it, I don’t have to filter the class from the main list every time. This is useful for when I pick an armor at random, I can order the armors by property, see if a gun meets certain criteria, etc.
But… I also need to do stuff to every object. Things that will afect my inventory as a whole. Like I may need for example to populate the trade balance, or take into account the overall weight, etc. For this I have a super list of type MyObject.
Now, having all of this lists plus a superlist gets tedious, because if I want to add an item, I have to find the proper list where I may add it, and also add it to the super list, and everytime I want to do something I have to write it for every list (i.e. " if (object is MyArmor){myArmorsList.Add(object);}if (object is MyWeapon){myWeaponList.Add(object);}" ), this seems stupid as it creates a lot of code and introduces error.
For the past days I’ve been trying to find the best solution for this type of thing. Using just one list and filter it with linq seems like I’m doing to much usless processing, you’re always filtering stuff you filtered a few seconds ago. Interfaces haven’t been of much help here either. Generics allow to do several things, and I’ve used them, but they’re a bit ugly and is difficult to make them work (there are cases where you’ll need to use some sort of reflection to make them work too). Properties could be a solution, but also seems a bit ugly and prone to error.
So, how would you handle such a thing?
You can have a dictionary of lists with the type as a key.
or, you can extend list class and make two classes
first extended list should have events for add/remove/replace items
and second list should rely on those events to maintain it’s content automatically
class MasterList<T> : List<T>
{
public event Action<T> onAdd;
public void Add(item) {
base.Add(item);
onAdd?.Invoke(item);
}
public class SlaveList<U> : List<U> where U : T {
public SlaveList(MasterList<T> master) {
master.onAdd += HandleAdd;
}
private void HandleAdd(item) {
if(item is U){
base.Add(item as U);
}
}
}}
Well that idea looks very interesting.
I’m a bit confuse about the code though. I guess is pseudo code because it throws several erros when I put it in VS. So, let me ask you a few things on how you override the Add method of the List class.
The first thing that it says is that the paremeter “item” can’t be found. Since it doesn’t have a type, I tried adding a generic type T. After that it says Add() is hiding the baseclass member. I thought that was the idea, so I marked “Add()” as an override and it says that it can’t because the original Add() is not marked as abstract. As far as I know I can’t change the original List class (?).
So I’ve heard many times of exapanding the List class or making a class that inherits form it, but I’m not sure how to do it. There are other errors that throws that I’m not sure how to fix, for example “master.onAdd += HandleAdd;” says that “no overload for HandledAdd matches delegate Action”.
This is just a pseudocode to illustrate the idea and nothing more. I real implementation i suggest you inherit IList, not List, and delegate implementation to it’s private field
class MasterList<T> : IList<T>
{
private readonly List<T> list = new List<T>();
public void Add(T item) {
list.Add(item);
}
}
I’ve been trying to follow what you said as best as I could.
I tried to extend the idea to the rest of the methods of the implementation. Also, not only the master must be able to change the slave lists, but also, the other way around (hopefully without creating an infinte loop).
This is the code. I post it in case anyone can make use of it, or if you or anyone detects any issues or have any advice on how to improve it. Thanks a lot.
using System.Collections;
using System.Collections.Generic;
public class UberList<T> : IList<T>
{
#region propiedades
readonly IList<T> list = new List<T>();
public T this[int index] { get => list[index]; set => list[index] = value; }
public int Count => list.Count;
public bool IsReadOnly => list.IsReadOnly;
#endregion
#region eventos para las clases dependientes
public event System.Action<T> OnAdd;
public event System.Action<T> OnRemove;
public event System.Action OnClear;
#endregion
#region acciones disparadas por las clases dependientes
private void SlaveAdds(T item)
{
list.Add(item);
}
private void SlavesRemoves(T item)
{
list.Remove(item);
}
private void SlavesClear()
{
list.Clear();
}
#endregion
#region implementación
public void Add(T item)
{
list.Add(item);
OnAdd.Invoke(item);
}
public void Clear()
{
list.Clear();
OnClear.Invoke();
}
public bool Contains(T item)
{
return list.Contains(item);
}
public void CopyTo(T[] array, int arrayIndex)
{
list.CopyTo(array, arrayIndex);
}
public IEnumerator<T> GetEnumerator()
{
return list.GetEnumerator();
}
public int IndexOf(T item)
{
return list.IndexOf(item);
}
public void Insert(int index, T item)
{
OnAdd.Invoke(item);
list.Insert(index, item);
}
public bool Remove(T item)
{
OnRemove.Invoke(item);
return list.Remove(item);
}
public void RemoveAt(int index)
{
OnRemove.Invoke(list[index]);
list.RemoveAt(index);
}
IEnumerator IEnumerable.GetEnumerator()
{
return list.GetEnumerator();
}
#endregion
public class SlaveList<U> : IList<U> where U : T
{
readonly IList<U> list = new List<U>();
UberList<T> master;
#region >> propiedades
public U this[int index] { get => list[index]; set => list[index] = value; }
public int Count => list.Count;
public bool IsReadOnly => list.IsReadOnly;
#endregion
#region >> constructor
//Constructor
public SlaveList(UberList<T> master)
{
master.OnAdd += HandleAdd;
master.OnRemove += HandleRemove;
master.OnClear += HandleClear;
this.master = master;
}
#endregion
#region >> cambios ordenados por el master
private void HandleAdd(T item)
{
if (item is U myU)
{
list.Add(myU);
}
}
private void HandleRemove(T item)
{
if (item is U myU)
{
list.Remove(myU);
}
}
private void HandleClear()
{
list.Clear();
}
#endregion
#region >> implementación
public void Add(U item)
{
list.Add(item);
if (item is T obj)
master.SlaveAdds(obj);
}
public void Clear()
{
list.Clear();
master.SlavesClear();
}
public bool Contains(U item)
{
return list.Contains(item);
}
public void CopyTo(U[] array, int arrayIndex)
{
list.CopyTo(array, arrayIndex);
}
public IEnumerator<U> GetEnumerator()
{
return list.GetEnumerator();
}
public int IndexOf(U item)
{
return list.IndexOf(item);
}
public void Insert(int index, U item)
{
list.Insert(index, item);
master.SlaveAdds(item);
}
public bool Remove(U item)
{
if (item is T obj)
{
master.SlavesRemoves(obj);
}
return list.Remove(item);
}
public void RemoveAt(int index)
{
if (list[index] is T obj)
{
master.SlavesRemoves(obj);
}
list.RemoveAt(index);
}
IEnumerator IEnumerable.GetEnumerator()
{
return list.GetEnumerator();
}
#endregion
}
}
I found that I can’t use AddRange() and a few other things with IList.
Would it affect me if instead of using an IList as my private list I usea a regular list? after all is readonly, it shouldn’t matter, right?
You work is very good, but there are still things to do
-
When calling an event, you missed ? sign. This is important. It should be OnAdd?.Invoke(item), without it this line will throw exception if there are no slave lists
-
this[int index] shoud invoke onReplace event because it replaces item in master list with some another item and slave lists must be informed of this change. You need to add this event
-
It is better to implement IReadOnlyList interface in slave lists than IList so slave lists are just for viewing items and no make changes in master. It is always easier to maintain one point of control(changes) than multiple ones.
You may implement this method yourself or create your own list interface with it and use it
IBulkList<T> : IList<T>
{
void AddRange(IEnumerable<T> range);
}
I suggest you do that because if you ever use it, then it will be easier to add another event and process multiple added items at once in all slave lists than handling multiple OnAdd events.
Personally I’d just have a List that holds all my inventory. And if I needed the parts of it use the linq method ‘OfType’.
List<MyObject> Inventory = new List<MyObject>();
//elsewhere
var lightestItem = Inventory.OrderBy(o => o.Weight).FirstOrDefault();
var lightestArmor = Inventory.OfType<MyArmor>().OrderBy(o => o.Weight).FirstOrDefault();
As long as this is something you’re not doing all the time (like several times per frame, all the time)… it’ll work fine.
I wouldn’t bother optimizing this unless I found some bottle-neck down the line.
If you get tired of typing OfType you can make read only collections for the query work
List<MyObject> Inventory = new List<MyObject>();
IEnumerable<MyArmor> Armor => Inventory.OfType<MyArmor>();
IEnumerable<MyWeapon> Weapons => Inventory.OfType<MyWeapon>();
IEnumerable<MyItem> Items => Inventory.OfType<MyItem>();
Or you can flip the idea on its head and have a read only master list
List<MyWeapon> Weapons {get; set;}
List<MyArmor> Armor {get; set;}
List<MyItem> Items {get; set;}
IEnumerable<MyObject> Inventory => Weapons.Concat<MyObject>(Armor).Concat(Items);
If the only thing you would do with the “master list” containing all types is to iterate it, you can easily write a wrapper for iterating all of the sublists:
IEnumerable<Item> All_Items()
{
foreach (var i in weapons) yield return i;
foreach (var i in armor) yield return i;
foreach (var i in potions) yield return i;
// etc.
}
// Used like this:
foreach (Item i in All_Items())
{
// Do stuff with item
}
This is also a good idea as an inverse of eisenpony and I. Where you join the disparate lists, rather than split the total list.
Both are equally effective.
No need to write some large class that has a lot of moving parts that may be hard to test/debug to a stable state.
I don’t like Enumarables and linq because they introduce allocations in every foreach, have costly count operation, and no fast access to content.
I don’t actually use Unity, so I don’t always know the caveats hidden in it… though I thought they had resolved that garbage issue with foreach after getting off that old version of mono.
They have with the foreach.
linq still has garbage issues.
But I find its convenience worth it when it’s for in frequent stuff. Sure doing it every frame over and over can be bad. But “when user clicks a button in the inventory screen, use linq to filter inventory” and the subsequent 60 bytes of garbage it creates aren’t all that bad.
Heck, a display timer that requires regular updating, and thusly regular string creation, is going to create way more garbage. And that doesn’t stop me from having a display timer.
And yeah, I’m with you… most of my work is actually not in Unity, and linq is freaking awesome in my regular work!
You can write wrappers for count and index, too, if you want. (And then you can avoid the IEnumerable by iterating using a regular for loop on the index.)
public int Count { get {
return weapons.Count + armors.Count + potions.Count + ...;
} }
public Item this[int index]
{
get
{
var sub = ConvertMasterIndex(index);
return sub.sublist[sub.index];
}
set
{
var sub = ConvertMasterIndex(index);
sub.sublist[sub.index] = value;
}
}
private struct SublistAndIndex
{
public List<Item> sublist;
public int index;
}
private SublistAndIndex ConvertMasterIndex(int masterIndex)
{
if (masterIndex < weapons.Count)
{
return new SublistAndIndex { sublist = weapons, index = masterIndex };
}
masterIndex -= weapons.Count;
if (masterIndex < armors.Count)
{
return new SublistAndIndex { sublist = armors, index = masterIndex };
}
masterIndex -= armors.Count;
// etc.
}
(If I were being serious about this, I would probably put all of the sublists into some kind of meta-collection, like a List<List> or a Dictionary<ItemCategory, List>, so that it would be possible to iterate over them and eliminate a lot of the copy/paste code.)
You could do something similar the other way around by keeping an explicit master list that is sorted by category and caching the starting-index of each category, but that would make it expensive to add new items in any category other than the last one. (Though if your list isn’t too big, that might not matter much.)
I end up using a base class list and made a few Properties that would get the list of subclases (as suggested). The OfType<>() method is much better than what I was using ( ist.Where (x => x is Subclass).Cast().toList())
The biggest issue with this approach is that I can’t look at the subclasses bit of info in the inspector. Eisenpony’s second approach also seems to have this problem.
You’re not going to be able to see it in the inspector without creating a redundant explicit representation of the data. (Though you could theoretically use a custom inspector where the explicit representation only exists in the inspector and not the class itself.)
But you don’t necessarily want your alternate representations exposed in the inspector–what happens if someone uses the inspector to edit them? You may want to think about alternate ways of accessing that information for debug purposes, such as logging it, or simply using your game’s regular UI to view it (presumably you intend to build a way for the player to look at their inventory anyway, and once you do that’s probably nicer than using the inspector anyway).
By the way, remember not to use ToList() unless you actually need the result in list form. If you’re just iterating the collection, this causes an unnecessary memory allocation.
I always forget caveman tricks and have to force myself to slow down.
You mentioned how adding an object could add to the masterList and also the particular armour/weapon sublist, but that’s lots of code and error-prone? Yes, but write a function Inventory.AddItem(item1). I’ve done the exact thing – I was so worried about some complex issue that I forgot the basics.
Likewise, so many options for storage: keep all lists, or compute sublists as needed, or recompute the master list as needed. But the solution is basic OOP: Inventory.getArmourList(); and so on. Implement it in any piece-of-garbage way and fix it later. I always get all wrapped-up in details and forget interfaces are exactly for this sort of “I don’t want to close off making this thing way better, later on” problem.
And lastly, and I also do this all the time, it’s just a speed issue. And it’s inventory. How many things do you pick up each frame? Doing it in the worst, sloppiest way will probably be fine. I often start out thinking of the best way to organize it to be readable and easy to code, then slide into the ways it could be more “awesome”, when I could have it working good enough and have moved on.