refactor!: give items identity

[joveeater]

Summary

SUMMARY: Infrastructure "Support managed memory and safe references for items"

Purpose of change

Give items identity. This is the start of a big refactor. I want to pay off some technical debt, untangle the spaghetti and get all our ducks in a row. The real aim is ease of development through consistency and safety. I've spoken about this a bit before but eventually this will mean moving more things to actors allowing the rest of the engine to focus on specific clear tasks and not have to worry about unexpected invalidations. Actors themselves need to be safe and so the first step is safe references. I want to call this new system game objects or GOs. To begin with that will mean items but creatures and vehicles are the next targets. Maybe it could stretch to something like fields but not much further. The main aim at the moment is just to get nice consistent interfaces that we can build out behind later.

GOs know their own location and allow you to do things like describe that location or remove them from it. This is kept up to date automatically as the object moves around the game world. GOs can't be constructed normally (private constructors), instead they use static ::spawn methods. The main ::spawn will give you a GO without a location and it's your duty to give it one or call ->destroy() before the end of the turn. ::spawn_template and _temporary give you a GO with a special location that will debugmsg if you try to put it in the game world. Temporaries are destroyed at the end of turn. Note that a GO is "loaded" if it's inside the reality bubble as such templates/temporaries are never loaded.

GOs cannot have two locations at once and they can't be destroyed while still having a location. Attempting to do either will debugmsg and then call detach() to remove them from their location first. They debugmsg to warn you to be careful of iterator invalidation. GO containers should provide methods that update both iterators and locations correctly. If iterators aren't a problem just call detach yourself first. Note that detaching something infers the same responsibility to put it somewhere or destroy it that creating something new does. Failing to do this will cause debugmsgs in debug builds and memory leaks in release. This level of strictness around iterators might not be appropriate for monsters/vehicles etc and we can just cut that debugmsg but I think it's warranted for items.

Normal references and pointers to GOs are always valid until the end of the turn, even through movement/destruction/unloading. This means that when appropriate it's important to check their destroyed/loaded status, however with good actor separation that should only be when you're directly engaged in destroying stuff etc. Conversely they should never be considered valid across turns so they're suitable for the temporary variables/structures that get passed around as the engine does its work. If you want a reference that goes across turns you need one of two depending. A cache_reference is safe, it will deny access to unloaded or destroyed things and it's fast, but it can't be stored in save files and once something leaves the reality bubble it loses track of it permanently so it's best for caches. A safe_reference is slower but can be serialized and allows explicit const access to things that were destroyed/unloaded this turn and doesn't lose track of its target. They can also tell the difference between destroyed and unloaded targets, which cache references can't.

And that's all you really need to know to use GOs and for the most part they all work the same, but this is a cleanup effort. More important than what's added is what's being removed. The old safe_reference is first on the block and item_location is the same. Half of its features are going in items themselves and the other half in references. Note that new item locations work similarly to the old ones, except they're a bit more granular wrt things like wielded/worn and of course they're kept up to date by methods like add_item_or_charges or rem_i etc. Other GOs will have a similar system of locations as well but this is less of a change for them.

Describe the solution

Most of the implementation is fairly simple if extensive. I said pointers were only used for turn to turn stuff but actually they're used internally for whatever's storing an object's location as well. This is a big change for items as there's an extra layer of indirection on most of their structures. Something like a vector of item is a compile error now with the private constructors, it needs to be item*. This means lots of syntax changes where . becomes -> etc, not to mention the many constructors becoming ::spawn and since I'm going through all of it anyway I'm going to take the time to look at the structures used for all this, move everything across to vectors for now and then come back and do some optimization in important places.

Safe references are not simple, mostly because I want something that could last in the json for compatibility down the line and that has good save scumming properties. Rather than go into detail on the implementation here check the comments in safe_reference.h but I still need to finish, test and debug it as with all the code. The idea is that save scumming can have some effects but they're limited. References to things that now never existed are perpetually unloaded. Otherwise it will usually only cause small amounts of memory to be leaked or safe references to destroyed things to tell you they're unloaded or vice versa and typically code won't care to draw a distinction between those two states anyway. In rare cases where objects that are merged together both already have an internal id it can cause one half to point to a perpetually unloaded object.

Internal ids used by safe references aren't exposed and are generated lazily. I'm gonna take a look at their format but for now they're using the fact that they're always generated as the result of a save and just storing the timestamp of that save as the start of the id. Really this will probably be enough, so long as you're not messing with your system clock collisions shouldn't be a problem. In the actual json GOs just have an optional id field, once they have an id they don't lose it, but it's stored externally to the GO itself in memory. Most GOs don't have one at all and then safe references in json are just a numeric id.

As you can see from the mess that is the current code there's still a lot to do. Next step is taking inventory of all the places an item can be in the game world, doing a location for each and restricting changes except through methods that update location. Then fix all the todos and errors and generally clean everything up, resolve conflicts, test the shit out of it and get this thing in a mergeable state. There'll probably be a lot of teething problems but at least things like activities should be able to track items more reliably. Then it's back for creatures and vehicles becoming GOs, then doing better actors, then character stats/abilities at which point we should have good interfaces that we're happy to expose to LUA and support going forward and I will finally have achieved the initial goal that led me down this path of not having to hardcode stuff against traits.

Then the real work can begin and we can start improving what's behind those interfaces. It's all a long way off but I want to make a real push towards getting a solid consistent engine that's easy to work with and debug. This first step with items is by far the hardest but consequently the most pressing. None of the code changes are at all final, they all need checking for correctness. They just show the general direction and extent of what's needed.

Describe alternatives you've considered

Myriad. More important than any specific plan is consistently applying that plan across the engine and there are still a million decisions to make. Next steps for me is I'm gonna create one of those fancy projects that lays out the wider plan. Then hack away at this whole thing until it's done. I'll be detouring for stuff like the renderer at some point and once the interfaces are nicely in place I do wanna come back to jumping and flight, but I'm gonna try to focus on these big architectural issues. I thought our biggest problems were g->m and npc ai. What a grand and intoxicating innocence.

Anyway, this is the first of probably many greats walls of text, not to mention this all needs good documentation. Let me know what you think. Any specifics are still up in the air.

[joveeater]

Right, more explaining the new location types. I've started to update the docs although some stuff is still missing. The big snarl to this is that it means all of the structures representing physical locations within the world need their types changing slightly. The new location_vector doesn't quite meet the interface for a vector, because it uses different types for adding/removing than it does for examining. I was hoping we could get away with a few simple types like vector but crafting vs real inventories means that needs duplicating too.

The real workhorse of this scheme is the detached_ptr type. detached_ptr is used to represent items that aren't currently in the world. You need to use it with std::move, which prevents any item duplication and if you don't move it the item will get destroyed when the detached_ptr does. It's very similar to a unique_ptr except that it's only doing our soft destroy, not a real delete which will happen at the end of the turn.

This means a bit of familiarity with std::move and rvalue references when writing code that moves items around. For the most part it just means you have to remember to use std::move when passing a detached_ptr somewhere and you get a compile error if not. When you're writing functions that take a detached_ptr (i.e. functions that might place it somewhere in the world, use a normal pointer/reference otherwise), you should use detached_ptr<item>&& in your signature. Then, the caller can check the detached_ptr as a boolean afterwards to see if the item was actually placed in the world and run a fallback and whatever they do it'll be destroyed correctly.

Note that a lot of item moving functions, for instance character::wield, still don't take a detached_ptr, because they deal with detaching the item themselves. You can easily get a detached_ptr from an item by calling detach(), but this removes it from the world and invalidates iterators etc. It also gives red text and fails if the item is already detached in order to maintain the only one detached_ptr at a time rule. All of the item removal functions now also return a detached_ptr.

There's a bit of fake item marshaling. Fake items are split into two categories. There are temporary items for things that're used and then immediately discarded and they're identical to spawning something then not moving the detached_ptr you get anywhere. There's also actual fake items that use a special fake item location, they have to live on actors and they won't let you detach them. Speaking of, the memory management around actors and activities is changing a little. They're now more consistently using unique_ptr and doing less copying of themselves which means we don't have to worry about copying their fake items.

What I'd like to do at some point is get rid of the null item as well or at least get rid of all its legitimate uses and rename it to the error item. I'm not gonna do that yet, but I am gonna use the null item as a cushion for memory errors. Attempting to dereference any of the new pointer types will give the null item and red text rather than an actual nullptr so we can mitigate crashes.

[joveeater]

Alright more stuff that'll probably make its way into the docs at some point. I want to describe more of the rationale behind it here though. The real aim here is safe references which require indirection. Detached pointers are just the guard rails to prevent the foot guns that opens us up to but the big problem with enforcing correctness is everything needs to be verifiably correct. For most stuff this is just a simple translation however there are two big things that kinda sucked anyway and are totally unsuitable now.

One is functions that take a cbc item and maybe take away some of its charges and then you have to store the old charges and compare them in order to see if it did anything, the other is functions that return true to mean delete me. The fundamental problem is the same, you have to use them right or they bite you. The solution to both is the same as well, these functions now take and return detached pointers (i.e. detached_ptr<item> && -> detached_ptr<item>), with the idea that they return anything they didn't handle.

This means in order to use these functions or any of the functions that add something to the world you need a detached pointer and they can't just be constructed normally. Instead there are a number of different ways to get one depending on the situation. Of course there's the simple ones. Spawning new things or calling a function like i_rem returns a detached pointer now.

The first is the item.detach() method. It just directly removes the item from the world and returns its detached_ptr. The downside is it happily invalidates any iterators and instantly forgets where the object came from. It also requires that the item exist somewhere on the map, there can't be two detached_ptrs at once. Note that any unhandled detached_ptrs will be cleaned up so if you just want to delete an item you can call detach and ignore the result.

After that we have attempt_detach(). This takes a lambda of detached_ptr && -> detached_ptr and solves the problem of forgetting where things came from. It will place whatever you return back where it came from. It doesn't support true transformation, you have to either return the original item or a null detached_ptr to delete, but this is something that could be improved in future. There is an exception in the form of cbc items, they only require you return something mergable with the original and they also support split/attempt_split methods with similar mechanics to detach except they take a quantity as well.

If you need iterator safety, methods like erase on location_vector take an extra detached_ptr out parameter. There's also remove_top_items_with/remove_all_items_with as part of visitable. If you want visit responses there's also remove_items_with which takes a detached_ptr && -> VisitResponse lambda. Technically returning the detached_ptrs isn't necessary. They're references anyway so they can be used as out parameters. It's just a lot cleaner to return.

And that's all you need to safely move items around without having to worry that they'll end up in two places at once or get forgotten about. Like I say at some point I'll write this all up into docs with examples and so on.

[scarf005]

[olanti-p]

Related to the bug causing #3552 ?