Moved to generics

[Doprez]

PR Details

This PR shows a POC that would reduce the amount of required casts by users. I don't want to merge this but I was curious on what the thoughts would be on something like this as using object causes a ton of casting and required checks to be usable. Ideally I would redo this PR but with the Obsolete attribute. The only variable that gets in the way is CollectionChanged as I dont know what the non-obsolete variable would be called.

I also still need to verify that there wont be weird checks in the GameStudio layers as I assume they may not like generics as much as objects. Works fine.

Related Issue

No related issues. I ran into this today trying to use tracking collections for a modular system I am experimenting with as seen in the code snippet below:

    private void WornItemsChanged(object? sender, TrackingCollectionChangedEventArgs e)
    {
        if (e.Action == NotifyCollectionChangedAction.Add)
        {
            var newItem = e.Item;
            if (e.Item is GameItem item && !item.HasDescriptor<WearableDescriptor>())
            {
                _logger.Error($"Only wearable items can be worn. Item: {item.Name}");

                // Remove the item if it is not wearable
                var key = e.Key as string;
                var oldItem = e.OldItem == null ? null : e.OldItem as GameItem;
                WornItems[key] = oldItem;
            }
        }
    }

Types of changes

• Docs change / refactoring / dependency upgrade
• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• My change requires a change to the documentation.
• I have added tests to cover my changes.
• All new and existing tests passed.
• I have built and run the editor to try this change out.

[Doprez]

No issues with a simple example.

public class TestComponent : StartupScript
{
    public TrackingCollection<int> Numbers { get; set; } = [];
    public TrackingHashSet<int> UniqueNumbers { get; set; } = new TrackingHashSet<int>();
    public TrackingDictionary<int, string> NumberNames { get; set; } = new TrackingDictionary<int, string>();
}

[Doprez]

For context on if I were to make this real I would just change the existing TrackingCollectionChangedEventArgs to inherit from TrackingCollectionChangedEventArgs<T,T> like TrackingCollectionChangedEventArgs<object, object> and set it as obsolete for the people who already use this functionality so they can migrate over gradually.

[Kryptos-FR]

I'm no against it if there are no regression.

I don't think we can do a transition with an duplicated obsolete and non-obsolete property. That would be messy. So it has to be a breaking change IMO.

[Doprez]

Im ok with it being a breaking change. I think it would be fairly easy to tell people how to migrate as they just need to define their types but I just dont want to wait for this to release 😆

I was also thinking of making a seperate event args for Lists since they shouldnt need to define 2 types and would simplify the Collection and HashSet event definitions.

[Doprez]

I went ahead with the implementation that separates the definition between collections and keyed collections. This was a lot nicer to work with at a higher level and just looked cleaner overall.

Would this be able to be pushed in to main as a revision breaking change or would it have to wait for the major/minor release?

[Eideren]

Looks good, just minor stuff. Have you looked at the samples solution if there is any remnant over there as well ?

[Eideren]

I'll change this to draft for organizational purposes, feel free to change it back when ready :)

[Doprez]

I should have resolved all of the issues brought up before. I also have some of my time back for the next couple of weeks so I should be a bit more active than I was.

[Kryptos-FR]

Given that we just released 4.3, I think it would make sense to start a future (or 5.0) branch which includes breaking changes. We need to maintain 4.3 for minor changes.

Alternatively, we could have a master-4.3, but we would need to make that one the default branch.

[Doprez]

Im ok with that, this doesnt "fix" an issue and is just a QOL change so its not urgent.

I think we talked about a future branch before for the IOC changes as well. Did a branch get created for that? I definitely would consider this a breaking change for anyone using these event based lists so at least a 4.4 would probably make sense.