Standardizing the way dynamically loaded stuff is handled

[BevapDin]

Handling the dynamically loaded stuff is very _in_consistently done:

• Storing the loaded data: in a global variable (e.g. termap) vs inside a singleton, (e.g. item_action_generator, which is not a real singleton because it has a public constructor)
• Access:
  • no restriction at all, e.g. termap, bionics, Skills::skills
  • no restriction at all, but only visible in the contained cpp file, e.g. the list of construction in construction.cpp
  • access through functions, e.g. profession::_all_profs, item_factory
• Container:
  • global variables, not contained at all, e.g. termap
  • as static variable in a struct/class, usually private, but sometimes public, e.g. Skills::skills
  • inside a class instance as non-static member, e.g. item_factory
• Function names differ everywhere, for accessing single values, for checking, for loading, ...

Having this consistent would make it easier to understand (one has to understand it for one object type and can transfer that knowledge to all the other object types). It might also allow to include a better (and generic) blacklist/whitelist. Hey, this just came up in another issue.

---

I thought about the following changes:

To address the access restriction:

struct ter_t {
private:
    ...
    static std::vector<ter_t> terlist;
    static std::map<std::string, ter_t> termap;
public:
    static const ter_t &get( ter_id id );
    static const std::map<std::string, ter_t> &get_all();
    static void load( JsonObject &jo );
};

Move all the maps/lists etc. into the class they contain, as static private variables. Add public load functions that can write to the private members (which allows to make those members private in the first place) and add / remove from the map.
Add get/find functions that can access the map, return a dummy value if no matching entry is found (along with a debug message), return a const reference to the entry to forbid writing to them.

The maps/lists do not need to be inside the struct, they can be hidden completely inside the cpp file as introduced in #11791, through the wrapper function should probably be part of the struct to prevent accessing it in a non-const way when a const reference is just as good.

---

At this point a standardization of those access function should be discussed, for example:

struct ter_t {
    ...
    // find exists only for things with a string and an int id (e.g. terrain types).
    // It converts the string id to the int id. Inspired by the existing findter function.
    static int find( const std::string & id);
    // Get the object itself via id (id can be a string or an int,
    // add overloads for both if needed).
    static const ter_t &get( int id );
    static const ter_t &get( const std::string &id );
    // Returns whether that id refers to an existing/defined
    // object, this is usually used for consistency checking.
    // Overloads for int/string ids if needed.
    static bool has( id );
    // Returns the internal map with all the objects, but as a const reference,
    // therefor the caller can not change it, but can easily iterator over it.
    static const map & get_all();
};

Instead of the whole map (get_all), one could expose begin and end iterators as it is already done for example in the profession class, but I think this is not very useful. It has no advantage at all and exposing the map (as const reference) allows much more (like find and range-based loops) and still respects the const.

Having overloads for the get and has functions makes it easier to use, one can write get(X) without thinking about whether X is the int id, or the string id.

The names are short, even with the struct prefix:

x = ter_t::find( X ); // instead of:
x = terfind( X );

For loading/unloading, the container struct only needs to contain the following 7 lines (or less if the check_consistency/finalize function are not needed):

    /*@{*/
    /** Initialization, see init.h for how this works */
    static void load( JsonObject &jo );
    static void finalize();
    static void check_consistency();
    static void unload();
    /*@}*/

This packs the functions neatly together, has a reference to the place that explains the dynamic load system and has no redundant documentation because the function behave the same for all types, load loads the given entry (and adds it to the internal map), check_consistency checks for internal consistency/invalid references, unload unloads the data (inverse of load), finalize does some stuff after everything else has been loaded to finalize the data.

The /*@{*/ marks the functions as belonging together (used in the automatically generated documentation).

---

This leaves the places that use a wrapper instance, like item_factory, item_action, MonsterGenerator, probably more.

Those are inconsistent in the way they expose the single instance: item_factory exists as a global static pointer variable, MonsterGenerator has the static generator function that returns the single instance, snippet_library exists as a global static variable (not a pointer).

Their functions should probably follow the same naming scheme as the static functions.

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

[bkentel]

You've echoed some of the thoughts I've had since beginning to participate in the project. Even better than just standardizing how to handle singleton-like data, would be to eliminate it altogether where feasible. What you are suggesting, though, would be a fantastic first step seeing as elimination is much harder and would require significant re-organization to accommodate.

[kevingranade]

I think this would be a great way to go, perhaps with a abstract class acting as an interface specification for it?
@bkentel I'm curious, what's the alternative to the singleton-like data you'd propose?

[kevingranade]

To answer my own question, there should be a top-level object that "owns" all of the various flyweight entity definitions, so instead of having code to initialize them statically and then reset them on demand i.e. when exiting a game session and loading a new save (that has a different set of mods and therefore different entity definitions), you just make one of these top level objects when you load a game session, and as part of initialization it creates all the containers for entity definitions, and populates them from json.

There's one tricky thing, which is how to map calls like:
item foo = item("taco");
to the 'current' item definition store. Right now there is "the" item definition store that is statically created, so all of the functions just point to that, but if it isn't a statically-allocated singleton, how so various functions find it?
One option is to have the game session object be singleton-like, and IT is what everything uses to look up what the current state of the game session is.

I'm pretty anti-singleton in general, but IMO one singleton is better than many singletons :)