bbradson / Performance-Fish

Performance Mod for RimWorld
Mozilla Public License 2.0
423 stars 34 forks source link

The CopyAllowancesFrom caching easily breaks intended behaviour. #21

Closed Epicguru closed 6 months ago

Epicguru commented 6 months ago

I found this when asked to investigate the cause of #20.

In a mod I have the following code:

filter.CopyAllowancesFrom(item.filter);

The expected behaviour is that the filter will now contain the items copied from item.filter, which should be returned when enumerating filter.AllowedThingDefs.

However due to the caching, the internal cache list is not synchronized immediately and so enumerating the AllowedThingDefs will return an empty enumeration which in the case of the Custom Ammo mod, broke the mod.

The current patch is as follows:

public class CopyAllowancesFrom_Patch : FirstPriorityFishPatch
{
    // ...

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public static void Prefix(ThingFilter __instance)
        => AllowedDefsListCache.GetAndCheck<AllowedDefsListCacheValue>(__instance).Defs.Clear();
}

I do not fully understand why the cache list is being cleared here, but I assume there is a good reason... Based on the other patches, I think that this would be fixed by adding a forced cache update in a postfix:

public class CopyAllowancesFrom_Patch : FirstPriorityFishPatch
{
    // ...

    // NEW:
    [HarmonyPriority(Priority.Last)]
    public static void Postfix(ThingFilter __instance) => ForceSynchronizeCache(__instance);
}

Currently I have suggested disabling the AllowedThingDefs patch (which fixes the empty-enumeration issue) until a fix can be made on this side.

Thanks.

bbradson commented 6 months ago

https://github.com/bbradson/Performance-Fish/commit/0247ad59a18154eb7f8cabcff727b5b8461c7ecc