Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix: autocrafting patterns registered twice after world reload
Fixes part of #1100, specifically the reproduction and spark profiles in this comment.
Cause
PatternRepositoryImplkeeps aSet<Pattern>, butadddiscarded the result ofpatterns.add(...)and pushed a holder onto the per-outputPriorityQueueunconditionally, so adding the same pattern twice indexed it twice.updatehad the mirror of that problem: it inserted a holder for a pattern that was never added, whenever another registered pattern shared an output.The calculator tries every candidate pattern for a resource and only stops early on success, so each duplicate is retried at every level of the tree. For a 5 level chain like a 64k storage part that multiplies the search by 2^5.
Why only after a reload
On world load the Autocrafter fills its pattern array before the network node joins a container, so
onAddedIntoContainerandonActiveChanged(true)each push the whole array. On fresh placement that array is still empty when both callbacks run and patterns arrive one at a time afterwards, so nothing is pushed twice.That is also why breaking and restoring the cable clears it, as described in the issue: the container is rebuilt and the patterns arrive one at a time again.
Fix
addon an already known pattern updates its priority instead of registering a second holder, andupdateon an unknown pattern is a no-op.PatternProviderNetworkNodeis unchanged, so this also covers relays and third party pattern providers.I debated where to fix the calling of the
addetc in thePatternProviderNetworkNode, that is still an option but more complex and stills holds the door open for other implementations to make a similar mistake (or add-ons that would make this mod look like the bad guy).This is the simplest solution I could think of that would always make the result correct.
Result
Same world and same request, measured with spark:
A reloaded world now costs the same as a fresh one.
Testing
Two tests at the repository level and two through
AutocraftingNetworkComponentreproducing the world-load pathOther things I found during testing, not in this PR
The pattern priority is not necessarily ordered
getByOutputonly guarantees the highest priority pattern comes first. It streams aPriorityQueue, and a binary heap is ordered only at the root, so the rest come out in array order. With three or more patterns for one output (priorities 1, 2, 3 added in that order come back as 3, 1, 2) the calculator, which stops at the first pattern it can craft, falls back to a lower priority pattern when the top one is missing resources, skipping a higher priority alternative. The Autocrafter priority tooltip promises highest-first, so this is real, though narrow: the top choice is always correct, only the fallback order is wrong. Two patterns can never misorder, which is why the current tests do not catch it. Fix is a sorted list per output instead of a queue.Duplicate patterns are still expensive
Duplicate pattern layouts are still expensive. This PR makes a reloaded world cost the same as a fresh one, but the fresh cost of encoding the same recipe several times is itself high, and the 632 ms above is that cost. The calculator has no memoization, so a
PatternLayoutthat already failed for a resource is re-explored for every sibling candidate.As I mentioned above "The calculator tries every candidate pattern for a resource and only stops early on success". A pattern that has already failed will still be retried even though there is no hope for it to succeed. In my repro world that is why the 4 copies of each pattern with fuzzy stained glass costs roughly 5100 calculation nodes, against roughly 26 when already failed layouts are skipped. Users who encode duplicates can still hit the timeout with a larger setup this way.
This is quite an easy fix and am happy to make a PR but I don't want to spam PR's before you get a chance to review the open ones