Repository navigation
autodump: add 'undestroy' option; fix container/unit-item destruction - #5904
Conversation
'autodump destroy' marked items for destruction with no way to revert, despite the docs claiming re-running the command before unpausing would undo it. 'undestroy' now restores the original flags of all items with destruction pending, reusing the per-frame pending_destroy tracking that autodump-destroy-item already had. The bulk destroy path also marked containers without their contents, leaving contained items pointing at deleted containers, and did not skip unit-held items, which could corrupt equipment refs (observed as a crash on save after destroying an equipped quiver). Items in unit inventories are now skipped like autodump-destroy-item does, and the contents of destroyed containers are marked along with them. fixes DFHack#5896 fixes DFHack#5769
14e2bd1 to
8cb9c36
Compare
| for (auto &entry : pending_destroy) | ||
| { | ||
| if (auto itm = df::item::find(entry.first)) | ||
| { // Item could have already been garbage collected |
There was a problem hiding this comment.
If another plugin forcibly deleted the item while the game was still paused, itm could be NULL and result in a crash
There was a problem hiding this comment.
Isn't that case already covered? itm is only dereferenced inside if (auto itm = df::item::find(entry.first)), so if the item was deleted while paused, find returns NULL and the entry is skipped. Unless you mean a case where find can still return a pointer to a dead item?
There was a problem hiding this comment.
the comment here is confusing - it implies that this block handles the case where the item has been deleted, which is not actually what is going on
There was a problem hiding this comment.
Disregard - I misread the code, not noticing that this was an assignment inside an if statement (which, to be honest, is probably something to be avoided specifically because it leads to confusion like this).
There was a problem hiding this comment.
@ab9rf Reworded it, hopefully clearer now.
There was a problem hiding this comment.
@quietust Fair point, I split the declaration out of the if.
| } | ||
| reset_pending_destroy(); | ||
|
|
||
| if (pending_destroy.count(item->id)) |
There was a problem hiding this comment.
use contains here - you don't actually care what the count is and there is no reason to force the library to count them when all you care about is whether the key is present
| // contained items are not left pointing at a deleted container | ||
| static void mark_for_destroy(df::item *itm) | ||
| { | ||
| if (!pending_destroy.count(itm->id)) |
Use map::contains instead of count for membership tests, spell out the NULL check instead of declaring itm inside the if, and reword the comment so it is clear that find() is what skips already-deleted items.
Summary
Adds an
autodump undestroycommand that reverts items pending destruction, the same undo thatgui/autodumpalready had (fixes #5896).While in there I fixed two related issues with
autodump destroy(fixes #5769):Pending-destruction flags are also restored from saved state on undo rather than blindly cleared, so items that were already marked before the destroy don't get unmarked.
Testing
autodump undestroyrestores bothci/lint.pyclean; plugin builds cleanly