Item access overhaul. (paving the way for item processing performance improvements)

[kevingranade]

This big scary guy is overhauling the way we access items on the map.
The heart of it is that map::i_at() now returns a const item vector, which you can't just write to.
Instead you have to call map::add_item() or one of the helper methods to add items to the map, and i_rem() if you need to remove them.
This is important because in order to address the active item processing performance issues that have caused a bit of consternation recently, we need to build a cache of active items, and we can't have code bypassing the blessed interfaces for adding and removing items from the map.

[Shoes01]

Having only read your comment, your function add_item() and it's inverse i_rem() seem like they should be named add_item() and rem_item() or i_add() and i_rem(). If you require I nitpick other negligibly small items, let me know!

[kevingranade]

Eh, neither of them is new, I'm just inforcing their use, don't feel like doing a find/replace everywhere to fix that as pert of this.

[Rivet-the-Zombie]

Is this ready for testing?

[kevingranade]

Yes it's ready, behavior shouldn't change, this just paves the way for
future optimization.

[KA101]

OK, it was marked in-progress and has merge conflicts.

[kevingranade]

Rebased and ready to go again.

[KA101]

Running over caffeine pills with a car segfaults. Stack trace and relevant line: http://pastebin.com/hYw6rtKa

[BevapDin]

Two remarks:

This creates dependencies: one has to supply the map and the coordinates several times (map::get_item(x, y, iterator), i_rem(x, y, iterator)) where the iterator (from that other PR) already contains that redundant data. This also means one can not just forward the item list / iterators into it to another function that does something, instead one has to forward the map object and the coordinates.

Handling access and removal directly via the iterator seems simple for the user and avoids the redundancy.

The get_item function seems strange, it returns the object which the iterator points to. This is usually done through a function of the iterator itself, not outside of it where it requires additional parameters. That function will also scale very poorly with an item list instead of a vector.

Btw. the error KA101 found is in map.cpp:871 item *smashed_item = get_item( x, y, it ); - the item vector that is iterated is from i_at(wheel_x, wheel_y), that's what I mean with dependencies.

[DavidKeaton]

So, I'm way more knowledgeable with C than C++, so I have to ask...

Seeing as how classes can be derived, could one inherit the std::vector (list, map, etc) and override some of the operators from the base class to do read only to prevent bypassing the cache, while also implementing add_item, i_at, etc. to said derived class?

[kevingranade]

The goals of this change are to prevent insertion and removal of items from
the map outside the blessed interface while still allowing modification of
those items. I was really hoping that I could just have an appropriate
const pattern to achieve this, but it seems the std containers do not
support it.
The other PR introduced a large layer to handle this, but it was both
overcomplicated and did not replace the i_at interface that is the source
of the problem. I'll take another look at it now that I'm looking into the
problem in detail, but I don't think it was usable without a total rewrite.

[BevapDin]

Another thing I forgot to mention:

I think making map functions const is quite useless. All the code accesses either the reality bubble map (g->m), which is always a non-const, or it access local tinymaps on the stack, which are non-const as well. The only place where the map object might be const is inside already const map functions.

Const functions in the game class are useless for the same reason, everybody has a non-const reference to the game anyway.

---

DavidKeaton bough up a special class for the items vector. This could be used to implement a wrapper that is returned by map::i_at:

class item_vector {
private:
    std::vector<item> *vector;
    int x, y;
    map *map;
public:
    // Than add all the main functions that `std::vector` has to item_vector:
    size_t size() const { return vector->size(); }
    bool empty() const { return vector->empty(); }
    iterator erase(iterator it) const { return map->i_rem(x, y, it); }
    iterator begin() const { return vector->begin(); }
    iterator end() const { return vector->end(); }
    const item &operator[](size_t index) const { return (*vector)[index]; }
};

item_vector map::i_at(int x, int y) {
    submap *sm = ...;
    return item_vector{ &sm->itm[...], x, y, this };
}

The wrapper would nicely store the coordinates and the map, it can be copied and given to further functions.
A second wrapper (base class) could provide const-only access.

[DavidKeaton]

Not that my say means much:

I like the above wrapper illustrated. It functions similar to most/all basic std containers, and can be customized in a fashion to limit access to only your get/set methods. You could also easily add aliases to methods for contiguous namings sake, while also keeping the same method names for backcompat.

The retval illustrated is a stack local (to the calling function) const "splice" of a portion of the map. Thus keeping its const-ness, yet having a lifetime in the calling function's stack for use in any other function. You could then make the map class a friend, able to access the pos and map, so you don't have to iterate over and over again, you could use indices/coords inside of the map pointer provided to make any changes.

e.g.
map::i_rem(retval);

where i_rem, being part of map (a friend class to item_vector) could:

void i_rem(item_vector iv) {
m = iv.map; //w00t class friends! :-)
x = ...;
}

Giving your god power to those who deserve it. Hell, if the std::vector in item_vector is a copy of the actual map item vector, the indices for specific items could be used in class map for removal, and for addition a simple insertion/append.

This could also be used in a journalistic manner for the cache, where you simply add each i_at returned item_vector in an array type structure, make changes to those references alone (lessening access time via contiguous stack mem), then flush and shift the journal cache constantly based on char movement. Of course storing previous journal for a bit would help with PC retracing footseps.

I apologize if this sounds like nonsense fyi, tired as hell.

[kevingranade]

I think making map functions const is quite useless. All the code accesses either the reality bubble map (g->m), which is always a non-const, or it access local tinymaps on the stack, which are non-const as well. The only place where the map object might be const is inside already const map functions.

Making methods const isn't intended to "protect" the object itself per se, it's intended to document which methods refrain from modifying class internal state, and as a reminder to not change that (possibly unintentionally).

The wrapper is a good idea, and looks like it will clean up some of the messier parts of the refactor, thanks :)

[kevingranade]

Fixed crash, will be using this as a base for the interface change BevapDin suggested, so no reason to hold off on this one if it checks out (and accumulate more merge conflicts).

[narc0tiq]

Except map::add_item is too stupid to be used externally. It's, in fact, leading to http://smf.cataclysmdda.com/index.php?topic=9022.0. Is there a reason you didn't use map::add_item_or_charges?

[narc0tiq]

Oh, nevermind, I see it -- it doesn't bypass NOITEM.