Skip to content

Conversation

@CJ-SPT
Copy link
Contributor

@CJ-SPT CJ-SPT commented Sep 9, 2025

Implements a cache to keep track of custom item ids added by mods. You can get items added by a specific mod by GUID or all added items by all mods. Collections are immutable outside of the service.

@qodo-merge-for-open-source
Copy link

qodo-merge-for-open-source bot commented Sep 9, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Avoid GetCallingAssembly; pass mod identity

Using Assembly.GetCallingAssembly and ReferenceEquals to identify the mod is
brittle (inlining/proxies/load-context differences) and can silently miss
caching. Pass an explicit mod identity (e.g., SptMod or GUID) to the creation
methods, or at least mark callers as NoInlining and compare assemblies by
identity (FullName) instead of reference equality to ensure consistent mapping.

Examples:

Libraries/SPTarkov.Server.Core/Services/Mod/CustomItemService.cs [76]
        modItemCacheService.AddModItem(Assembly.GetCallingAssembly(), newItemId);
Libraries/SPTarkov.Server.Core/Services/Mod/ModItemCache.cs [72-84]
    private SptMod? GetModFromAssembly(Assembly caller)
    {
        SptMod? sptMod = null;
        foreach (var mod in loadedMods)
        {
            if (mod.Assemblies.Any(modAssembly => ReferenceEquals(caller, modAssembly)))
            {
                sptMod = mod;
            }
        }

 ... (clipped 3 lines)

Solution Walkthrough:

Before:

// In CustomItemService.cs
public CreateItemResult CreateItem(...)
{
    // ...
    modItemCacheService.AddModItem(Assembly.GetCallingAssembly(), newItem.Id);
    return result;
}

// In ModItemCacheService.cs
internal void AddModItem(Assembly caller, MongoId modId)
{
    var mod = GetModFromAssembly(caller); // Relies on fragile assembly matching
    if (mod is null) { /* error and return */ }
    _cachedItems[mod.ModMetadata.ModGuid].Add(modId);
}

private SptMod? GetModFromAssembly(Assembly caller)
{
    // ... loops through mods and uses ReferenceEquals(caller, modAssembly)
}

After:

// In CustomItemService.cs
// The public API would need to change to accept mod identity
public CreateItemResult CreateItem(NewItemDetails newItemDetails, string modGuid)
{
    // ...
    modItemCacheService.AddModItem(modGuid, newItem.Id);
    return result;
}

// In ModItemCacheService.cs
internal void AddModItem(string guid, MongoId modId)
{
    if (!_cachedItems.TryGetValue(guid, out _))
    {
        _cachedItems.Add(guid, []);
    }
    _cachedItems[guid].Add(modId);
}
Suggestion importance[1-10]: 9

__

Why: This suggestion identifies a critical design flaw where using Assembly.GetCallingAssembly() and ReferenceEquals makes the caching mechanism unreliable and prone to silent failures, which is a significant issue for the PR's core functionality.

High
Possible issue
Return early when match found

The method continues iterating through all mods even after finding a match,
potentially overwriting the result. Return immediately when a match is found to
improve performance and prevent incorrect behaviour.

Libraries/SPTarkov.Server.Core/Services/Mod/ModItemCache.cs [72-84]

 private SptMod? GetModFromAssembly(Assembly caller)
 {
-    SptMod? sptMod = null;
     foreach (var mod in loadedMods)
     {
         if (mod.Assemblies.Any(modAssembly => ReferenceEquals(caller, modAssembly)))
         {
-            sptMod = mod;
+            return mod;
         }
     }
 
-    return sptMod;
+    return null;
 }
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that the loop continues after a match is found; returning early improves performance and prevents a potential bug where the result could be overwritten.

Low
General
Eliminate redundant dictionary lookup
Suggestion Impact:The commit changed the pattern to use TryGetValue with an out variable, initializing and adding to a local collection instead of performing a second dictionary lookup before adding the item.

code diff:

-        var guid = mod.ModMetadata.ModGuid;
-        if (!_cachedItems.TryGetValue(guid, out _))
-        {
-            _cachedItems.Add(guid, []);
-        }
-
-        _cachedItems[guid].Add(modId);
-

Use TryGetValue with the actual value parameter to avoid double dictionary
lookup. This improves performance by eliminating the redundant
_cachedItems[guid] access.

Libraries/SPTarkov.Server.Core/Services/Mod/ModItemCache.cs [53-59]

 var guid = mod.ModMetadata.ModGuid;
-if (!_cachedItems.TryGetValue(guid, out _))
+if (!_cachedItems.TryGetValue(guid, out var items))
 {
-    _cachedItems.Add(guid, []);
+    items = [];
+    _cachedItems.Add(guid, items);
 }
 
-_cachedItems[guid].Add(modId);
+items.Add(modId);
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly points out a double dictionary lookup and proposes a more performant pattern by using the out parameter of TryGetValue to avoid the second lookup.

Low
  • Update

@chompDev chompDev merged commit 6ae1d6f into sp-tarkov:develop Sep 9, 2025
5 checks passed
@CJ-SPT CJ-SPT deleted the mod-item-id-cache branch September 10, 2025 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants