RemoteCrafting optimisation#142
Conversation
|
|
|
||
| if (__instance.localTileEntity == null) | ||
| { | ||
| Log.Out("[DropBox] OnClose: localTileEntity is null, skipping."); |
There was a problem hiding this comment.
if (!buildsettings.Development)
Log.Out
?
| var composite = __instance.localTileEntity as TEFeatureStorage; | ||
| if (composite == null) | ||
| { | ||
| Log.Out("[DropBox] OnClose: localTileEntity is not TEFeatureStorage, skipping."); |
There was a problem hiding this comment.
if (!buildsettings.Development)
Log.Out
?
| StringParsers.TryParseBool(dropBoxStr, out var dropBoxBool) && dropBoxBool; | ||
| if (block is not BlockDropBoxContainer && !hasDropBoxProperty) | ||
| { | ||
| Log.Out($"[DropBox] OnClose: block at {pos} ({block.GetBlockName()}) is not a drop box, skipping."); |
There was a problem hiding this comment.
if (!buildsettings.Development)
Log.Out
?
| distance = StringParsers.ParseFloat(strDistance); | ||
|
|
||
| var primaryPlayer = __instance.xui.playerUI.entityPlayer; | ||
| Log.Out($"[DropBox] OnClose: distributing from {block.GetBlockName()} at {pos}, player={primaryPlayer?.EntityName ?? "null"}, distance={distance}, IsServer={SingletonMonoBehaviour<ConnectionManager>.Instance.IsServer}"); |
There was a problem hiding this comment.
if (!buildsettings.Development)
Log.Out
?
|
|
||
| if (__instance.localTileEntity.items == null) | ||
| { | ||
| Log.Out("[DropBox] OnClose: items array is null, skipping."); |
There was a problem hiding this comment.
if (!buildsettings.Development)
Log.Out
?
| } | ||
|
|
||
| var items = __instance.localTileEntity.items; | ||
| var tileEntities = RemoteCraftingUtils.GetTileEntities(primaryPlayer, distance, false); |
There was a problem hiding this comment.
Get tileEntities once before loop instead of repeatedly for each item.
| { | ||
| if (items[i].IsEmpty()) continue; | ||
| var itemName = items[i].itemValue?.ItemClass?.GetItemName() ?? "unknown"; | ||
| Log.Out($"[DropBox] OnClose: trying to distribute slot {i}: {itemName} x{items[i].count}"); |
There was a problem hiding this comment.
if (!buildsettings.Development)
Log.Out
?
| var itemName = items[i].itemValue?.ItemClass?.GetItemName() ?? "unknown"; | ||
| Log.Out($"[DropBox] OnClose: trying to distribute slot {i}: {itemName} x{items[i].count}"); | ||
|
|
||
| if (RemoteCraftingUtils.AddToNearbyContainer(primaryPlayer, items[i], distance)) |
There was a problem hiding this comment.
Use new function overload to use cached tileEntitites instead of recalculating them for each item.
| if (RemoteCraftingUtils.AddToNearbyContainer(primaryPlayer, items[i], distance)) | ||
| if (RemoteCraftingUtils.AddToNearbyContainer(primaryPlayer, items[i], tileEntities)) | ||
| { | ||
| Log.Out($"[DropBox] OnClose: distributed {itemName} successfully, clearing slot {i}."); |
There was a problem hiding this comment.
if (!buildsettings.Development)
Log.Out
?
| continue; | ||
| } | ||
|
|
||
| Log.Out($"[DropBox] OnClose: no container accepted {itemName}, leaving in slot {i}."); |
There was a problem hiding this comment.
if (!buildsettings.Development)
Log.Out
?
|
|
||
| private static NearbyContainerCacheEntry _nearbyContainerCache; | ||
|
|
||
| private sealed class NearbyContainerCacheEntry |
There was a problem hiding this comment.
Cache nearby containers to reduce frequency of recalculating them.
Invalidates on item change, or player moving out of the positionBucket.
Refactors other code touched to integrate cleaner, and be more readable. Code already had comments about needing to be refactored. Public signatures that have uses inside the project were not altered, but instead overloads/wrappers were added.
| } | ||
| } | ||
|
|
||
| private sealed class NearbyContainerScanContext |
There was a problem hiding this comment.
Put configuration in one context.
Mainly because previously this was re-read from files inside a hot loop.
But this also helps with separating business logic, from previously "magic" looking string manipulation.
| var disabledsender = Configuration.GetPropertyValue(AdvFeatureClass, "disablesender").Split(','); | ||
| var nottoWorkstation = Configuration.GetPropertyValue(AdvFeatureClass, "nottoWorkstation"); | ||
| var bindtoWorkstation = Configuration.GetPropertyValue(AdvFeatureClass, "bindtoWorkstation"); | ||
| var scanContext = BuildNearbyContainerScanContext(player, landClaimContainersOnly, landClaimPlayerOnly); |
There was a problem hiding this comment.
Use context and cache instead of magic and Configuration.GetPropertyValue.
| continue; | ||
| } | ||
| if (!tileEntity.TryGetSelfOrFeature<ITileEntityLootable>(out var lootTileEntity)) | ||
| if (!ShouldIncludeTileEntity(scanContext, tileEntity)) |
There was a problem hiding this comment.
Wall of guard clauses into one boolean.
| continue; | ||
| } | ||
| } | ||
| tileEntities.Add(tileEntity); |
| currentWorkstation = GetCurrentWorkstation(playerLocal); | ||
| } | ||
| else | ||
| catch |
There was a problem hiding this comment.
This workstation has an issue. Haven't encountered this in game, but worth adding logging for a test build.
| return value?.Split(',') ?? Array.Empty<string>(); | ||
| } | ||
|
|
||
| private static List<WorkstationLootBinding> ParseWorkstationBindings(string value, bool trimLootLists) |
There was a problem hiding this comment.
All the magic, mostly in one function.
| return values; | ||
| } | ||
|
|
||
| private static bool ShouldIncludeTileEntity(NearbyContainerScanContext scanContext, TileEntity tileEntity) |
| return string.IsNullOrEmpty(scanContext.BindToWorkstationRaw) || BindToWorkstation(scanContext, tileEntity); | ||
| } | ||
|
|
||
| public static bool DisableSender(IEnumerable<string> value, ITileEntity tileEntity) |
| var disableSenderValues = value as string[] ?? value.ToArray(); | ||
| var invertdisable = bool.Parse(Configuration.GetPropertyValue(AdvFeatureClass, "Invertdisable")); | ||
| if (!invertdisable) | ||
| return DisableSender(disableSenderValues, invertdisable, tileEntity); |
There was a problem hiding this comment.
Once per scan: read config, parse bool, normalize list
for every tile entity: only run the actual check.
Cached in scanContext
| if (player is not EntityPlayerLocal playerLocal) return false; | ||
| if (!scanContext.HasLocalWorkstationContext) return false; | ||
| if (!tileEntity.TryGetSelfOrFeature<ITileEntityLootable>(out var lootTileEntity)) return false; | ||
|
|
There was a problem hiding this comment.
Refactored to de-complexify the complex LinQ
| { | ||
| var result = false; | ||
| if (player is not EntityPlayerLocal playerLocal) return false; | ||
| if (!scanContext.HasLocalWorkstationContext) return false; |
There was a problem hiding this comment.
Nice clarity benefit of refactor.
| if (!tileEntity.TryGetSelfOrFeature<ITileEntityLootable>(out var lootTileEntity)) return false; | ||
|
|
||
| foreach (var bind in value.Split(';')) | ||
| foreach (var binding in scanContext.NotToWorkstationBindings) |
There was a problem hiding this comment.
We know what we are doing with what now.
| { | ||
| var tileEntities = GetTileEntities(player, distance, false); | ||
| var itemName = itemStack.itemValue?.ItemClass?.GetItemName() ?? "unknown"; | ||
| return AddToNearbyContainer(player, itemStack, tileEntities); |
| return AddToNearbyContainer(player, itemStack, tileEntities); | ||
| } | ||
|
|
||
| public static bool AddToNearbyContainer(EntityAlive player, ItemStack itemStack, IEnumerable<TileEntity> tileEntities) |
| break; | ||
| } | ||
| } | ||
| // Remove only the amount this container can actually satisfy, then continue to the next source. |
There was a problem hiding this comment.
O(n)->O(1)
Clarity
No off by one bugs
1 participant
Hi, I really think this mod is essential, but was experiencing some stuttering on my (linux + steam proton) system, so me and AI agent had a look and fixed the safest parts.
Not tested in multiplayer (no frens), but caching is done per player, so it should work, while leaving room for more server optimizations.