Update active items periodically instead of constantly.

[kevingranade]

The short version is this change lets the game spend up to 600 turns (1 hour) between rot checks for food, and up to 100 turns (10 minutes) between raise checks for zombies and similar.
There's also a fix in there for a bug where every comestible was being set to active, this pretty much wrecked my attempts to make this work properly, because all food everywhere was being added to the lists for processing, and worse, many of them were being added to the default "check every turn" list.

The longer version is that I split the active item list into a potentially arbitrary number of sub-lists, each of which guarantees only that the items on it will be processed at least once every n turns. In reality we're only using three at the moment (with ns of 1, 100 and 600)
It does this by building a list of item references from its lists that contains the first list.size() / n items rounded up and returning that list for processing. When being processed, the items are removed from the lists and then pushed onto the back of the list, so after processing the items that were at the start of each list are now at the end of each list.

Another optimization is item_stack::push_back_fast(), if we know an item can and will fit in a square, such as if we just removed it while processing it, we can skip the checks imposed by map::add_item_or_charges and go straight to map::add_item. The check for inserting items into vehicles isn't as expensive, so I didn't bother with it for now.

Fixes #9926 and should fix #10269

[BevapDin]

This might be problematic on invalid iterators (maybe only with debug iterators). I know that the logic here does not actually read/write the invalid item, so it might be fine, but have you considered using plain old pointers?

Also, I thing for safety, the map functions like add_item and i_rem etc. should just forward to the submap and not access submap::itm directly. That way one can ensure that every change of the item list is correctly

[kevingranade]

I'm not sure what you mean by use a pointer, my guess is you mean make active_item_set a std::unordered_set<item *> and insert into it with active_item_set.insert( &*it ).
While that would work, I'm not sure what the benefit would be. Let me know if I'm missing something.

By forwarding to the submap, I assume you mean adding setters/getters etc to the submap and making the actual container private? That's the kind of thing I'm generally working toward, but I'm running out of time right now so I don't want to embark on another set of API changes in this PR unless it's totally necessary.

[BevapDin]

my guess is you mean make active_item_set a std::unordered_set<item *>

Yes.

While that would work, I'm not sure what the benefit would be

I'm pretty sure the standard forbids dereferencing invalid iterators, so the std iterators could have a check in the them which triggers an assertion or similar.

Judging from the latest crash report http://smf.cataclysmdda.com/index.php?topic=8998.0 especially the "attempt to copy-construct an iterator from a singular iterator" indicates this.

[kevingranade]

If we inserted an invalid pointer into the container, it might avoid the assert, but will lead to undefined behavior if the pointer is later dereferenced. We must store the iterator in active_items anyway, right?

[BevapDin]

But the pointer isn't dereferenced. The pointer is stored, but before dereferencing it, has_active_item is called and looks for the item pointer (only pointer comparison) in the active item set, it does not find it and the processing of that item is skipped.

But there was a discussion on stackoverflow that reading a deleted pointer (not the target, just the pointer itself) invokes undefined behavior http://stackoverflow.com/questions/4990462

[kevingranade]

Yea, on balance I think it's better to just use the iterator and ensure that it is always removed from active_item_cache before we remove the item, pointer or iterator that's the only safe thing to do.
There might be something fancy we could do with linked smart pointers or something, but the requirement that we be able to move the object around to different containers with potentially different semantics (player/monster/NPC inventory, possibly things I'm not thinking of) makes me skeptical that we can beat schlepping around the iterators. If you have something I'm all ears though.

[BevapDin]

You will end up with an invalid iterator anyway in map::process_items_in_submap, after the active_items (which contains iterators) is copied.

Any of the iterators in the copied set can become invalid when the matching item is removed from the map before it's actually processed. That's the point where has_active_item would return false. Invoking that function invokes a lookup in the active item set which invokes hashing of the iterator which invokes dereferencing it.

[kevingranade]

Ok, you are correct. The set needs to store raw pointers, item_reference needs a (hidden) raw pointer, and we use that to check validity of an item_reference before we dereference it in any way.
Thanks for showing me the error of my ways ;)

[BevapDin]

I believe this is the reason for the recent crashes. You usually remove an item when it has stopped being active. But in that case the item is not removed from the active item set, it stays there and is processed again in the next turn. Edit: Nope, it isn't.

[kevingranade]

Could be, makes more sense to check if the iterator is in the container rather than if it should be in the list. There might be somewhere flipping the active flag that I didn't notice.

[kevingranade]

This move invalidated the iterators in the active item cache, then the i_clear would crash trying to remove them.
A copy followed by a i_clear() should preserve the expected behavior.

[KA101]

Anything else?

[macrosblackd]

Seems to fix the crashes and freezes I was experiencing.

[DavidKeaton]

Would using std::stack and std::shared_ptr help with a stack infrastructure and invalid pointers?

[kevingranade]

I'm not sure if that's going to make things any simpler, and would involve I think a sizeable overhaul of the map and vehicle code to wedge shared_ptr instances in all over the place instead of items simply living in lists. I'd love it if you could take a weak_ptr to an object owned by a container, but that's not supported, the owner has to be a shared_ptr.
AFAIK the outstanding issues with this have been resolved, and I'm fairly happy with how simple it is.

[DavidKeaton]

Ah fair enough my man. I think auto_ptr makes life easy on ownership, so long as an instance of it lives.

[BevapDin]

Now that I read this, that behavior will mess up the index of the items on the field. The zombie corpse (active) is at index 0, but once it's processed, it will be at the end of the item list, having the wrong index.

[macrosblackd]

That might be what causes tiles with active items to 'rotate' the topmost item.

[KA101]

I still have a revert button on this. Might pull the plug and see if that changes anything.

[DavidKeaton]

Perhaps this is that segfault bug some of us constantly get?

[kevingranade]

Are the crashes happening while you're crafting or doing other long actions
that interact with items? If not it's unlikely the cause of the segfault.
That having been said, changing the index will cause issues with crafting
and such, so we need to figure out something to address it. Unfortunately
there is no solution in the general case due to potential for explosions
destroying items, but in that case I'm guessing invalidating crafting isn't
really a problem.
Hmm, I might actually have a decent solution, instead of popping the active
item off the list, we can just make a copy of it, and then overwrite it
with the copy if it's still around after processing, and remove it if it's
not and it destroys itself. That will render the list stable, and possibly
more perform ant.
Please don't revert unless we decide the entire approach is unworkable,
it's going to be a huge huge mess if you revert this set of changes, and it
gets worse the further back you go.

[KA101]

Crashes are when active items (re)enter the reality bubble. We've had active foodstuffs disappear when losing (fresh), etc tags too.

[kevingranade]

Hmm, the first is probably a problem with the deserialization code, the
second is likely some interaction with the rot code where there's a bit
flipped or something.
I'm poking at making the active item processing code re-insert items at
their former locations, but I have almost no time to test it, so going is
slow.

[narc0tiq]

Ah, @kevingranade, that 'bot you have doesn't understand the difference between mentioning a PR or issue and posting a fix for an issue to make it auto-close.

[KA101]

And you were working on it, Narc, which is why Kevin put you on the unit. Let's not mistake the merger for a code-author.

[narc0tiq]

Sort of. This isn't an issue, so it's not getting auto-closed -- and in #10700 I was only referencing it as being a related PR (for background info). Pretty sure it doesn't make sense to assign me this PR, where assignees for PRs tend to be used as a "I'm about to merge this" flag.