Add lightweight crafting inventory to improve NPC construction time #46286
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.
Summary
SUMMARY: Performance "Add lightweight crafting inventory to improve NPC construction time"
Purpose of change
Fixes #41220 (performance)
Currently, when performing crafting/construction, NPC for each stack of components has to invoke
requirements_map
which, in turn, invokesare_requirements_nearby
.are_requirements_nearby
constructs temporaryinventory
, which includes all items in the 60 tile radius. Current implementation of theinventory
is highly inefficient, namely:Temporary inventory is constructed, checked once against a single requirement and immediately discarded afterwards.
Describe the solution
The gist of the solution is to use lightweight wrapper around
cata::colony<item *>
instead ofinventory
(which is internallylist<list<item>>
).However, to achieve this seemingly simple task, I had to:
visitable<T>
interface (replace it with virtual inheritance).Reason:
visitable<A>
andvisitable<B>
are two different classes, while all descendants of justvisitable
share the common interface, which can (and should) be used as an argument in different crafting check methodsvisitable
intoread_only_visitable
(doesn't allow item removal) andvisitable
(inheritsread_only_visitable
, addsremove_item
andremove_items_with
)inventory
parameter to acceptread_only_visitable
insteadcata::colony<item *>
which inherits fromread_only_visitable
temp_crafting_inventory
) instead ofinventory
inare_requirements_nearby
Note to reviewers: all commits in this PR are logical stages of the implementation (that correspond to the items above) and pass unit tests in isolation, so I suggest to review this PR one commit at a time to reduce the scope.
Describe alternatives you've considered
I probably could've left the template parameter in
visitable
, but then all crafting checks would have to be templated and moved to the header, which doesn't seem ideal. I think, havingvisitable
as a simple interface is a better design choice.Testing
Playtesting.
Also added unit tests for the new
temp_crafting_inventory
and changed the current crafting tests forinventory
to also check thetemp_crafting_inventory
.Additional context
There are multiple places in the codebase where
inventory
is created, queried couple of times and immediately discarded. Also, there isCharacter::crafting_inventory
. In the future, I plan to replace all of that with the newtemp_crafting_inventory
.Demo:
Before:
After:
Profiling results coming soon...