Rework active_item_cache to use safe references #31873
Conversation
After CleverRaven#31796 item processing invalidates item_locations, which causes issues with all activities using item_locations. This commit rewrites active_item_cache to store a safe_reference<item> instead of an item_stack::iterator, removing the now unnecessary active_item_set and has() functions. This also fixes a bug with active item reordering after processing introduced by CleverRaven#31406 and a minor bug with the submaps_with_active_items cache.
ifreund
added
[C++]
|
Could definitely use some more eyes on this. I'm pretty sure what I did works, but it would be easy to miss some subtle interaction here. |
| for( std::pair<const int, std::list<item_reference>> &kv : active_items ) { | ||
| for( std::list<item_reference>::iterator it = kv.second.begin(); it != kv.second.end(); ) { | ||
| if( it->safe_ref ) { | ||
| all_cached_items.emplace_back( *it ); |
There was a problem hiding this comment.
It looks like you're not incrementing the iterator in this branch?
(This might be the cause of the unit test failures)
There was a problem hiding this comment.
Certainly seems like it, thanks for catching this
There was a problem hiding this comment.
The tests pass locally for me after this change. Totally neglected to run them before pushing the initial commit since I thought they didn't test active item processing. Turns out they brush up against it a little.
Should pass travis now I hope
src/map.cpp
Outdated
| @@ -5086,43 +5074,44 @@ std::list<std::pair<tripoint, item *> > map::get_rc_items( int x, int y, int z ) | |||
| return rc_pairs; | |||
| } | |||
|
|
|||
| static bool trigger_radio_item( item_stack &items, item_stack::iterator &n, | |||
| static bool trigger_radio_item( item_stack &items, safe_reference<item> &safe_ref, | |||
There was a problem hiding this comment.
It seems unlikely this needs a safe_reference. Can it just take an item* or item&?
There was a problem hiding this comment.
It needs to be a
using map_process_func = bool ( * )( item_stack &, safe_reference<item> &, const tripoint &,
const std::string &, float, temperature_flag );so unfortunately not
In particular increment the iterator properly in active_item_cache::get()
|
I tried building from this merge request and opening an existing savegame: it solved the butchering and corpse hauling problem. On load it gave loong list of those errors: |
clang-tidy told me what to do
Glad to hear it's working to fix those bugs for you! I'm not sure how those errors could be related to this PR in any way. At a guess I'd say that's an issue with your save. |
|
@IvanShestakov I don't think this PR is causing that error. I got something similar when I compiled a recent master having nothing to do with this PR. Out of curiosity/hunch I tried rolling back through PR #31831 (specifically to 615a4ed) and loaded the save, got the error. Went back one more commit (de083b3) and the save load error was gone. |
|
Quadruple kill! |
ghost
mentioned this pull request
Summary
SUMMARY: Bugfixes "Rework active_item_cache to use safe references."
Purpose of change
After #31796 the current item processing invalidates item_locations, which
causes issues with all activities using item_locations.
Fixes #31839
Fixes #31840
Fixes #31846
Fixes #31868
Describe the solution
This PR rewrites active_item_cache to store a safe_reference
instead of an item_stack::iterator, removing the now unnecessary
active_item_set and has() functions.
This also fixes a bug with active item reordering after processing introduced
by #31406 and a minor bug with the submaps_with_active_items cache.
Describe alternatives you've considered
I considered completely removing
active_item_cache::remove()and relying on lazy removal of broken references in the getter functions. I think this would be better, but I didn't see a way to handle updating thesubmaps_with_active_itemscache in that cases. I could do this for vehicles since their active item status isn't currently cached, but wasn't sure if it was desirable or not.