[RDY] Ad hoc string identifiers for terrain + generic id caching (refactoring)

[codemime]

Rationale:

• Provide convenient access to terrain objects using the existing tool (string_id<T>)

• Make usage of the generic_factory class (or its derivative) possible for handling terrain. The current terrain code looks pretty old (probably it's mostly a bequest from the original game).

• Get templated loading of ids from JSON files

• Get rid of the hard-coded terrain id tables (at least try to)

• And the last, but not least. While working on JSONize gates #15945 I realised how convenient it would be to cache int ids inside string ids (see the d22c736 - there's an ugly attempt, but it reveals the intent. dc1dbba088c1f756a62edfe420cda9640a527ec4 - the same code after). Since we don't remove existing prototype objects separately, we're in position to kill two birds with one stone:

  1. Reduce hashmap lookup
  2. Make the code nicer without any harm to performance
  

  Edit: even if we do delete them, there's still a good way to cache ids.

Done:

• IDs are in place and (as far as I can tell) work fine, but still require some testing
• I added filename and linenumber to the displayed debugmsg() for testing purposes (will revert the commit later if it's not desirable) Reverted and made a separate extended PR
• Integer ids are retreived from string ids using the existing map lookup for the time being cached value
• ter_t and furn_t now have different identifier types (same plain strings for furniture --> another PR later)
• string_ids can be compared with standart strings (no need to construct an object for that)
• Identifiers are passed by reference where former strings were passed by value
• Things are partly encapsulated~~, but there's still some work to do~~.

TODO:

• Get rid of the remaining terfind()'s
• Hide the rest of the termap inside mapdata.cpp and don't export it
• Initialize optional string ids inside constructor init lists
• Implement int_id<T> caching
• Get rid of the obsolete loadid
• Other code improvements.

[BevapDin]

Please make those references const.

[codemime]

I've done it already (as a part of 'Other code improvements'), but didn't push the unfinished commit yet.

[BevapDin]

The whole caching of int id looks like premature optimization to me. I doubt it's worth the effort. Have you measured that this is really a problem?

Besides that, it looks good.

[codemime]

I have and I'm convinced, that it's worth the spent effort. I'll describe my point more in detail in a summary a bit later. Thank you for the review.

[BevapDin]

Regarding id caching: another issue would be invalidating those cached ids when loading a new game. Some string ids are statically allocated, this would mean their cached ids are allocated statically as well. They are not reset when a new world is loaded, but loading new world data may change the mapping of string to int ids.

The int ids are sequentially (based on the load order), imagine a world 1 with mod A and mod B (loading in that order) and a world 2 with mod B and mod A (in that order).

In world 1, the terrain from mod A gets id 100,101, ... and from mod B would get 200,201, ... (example numbers). In world 2, the mod B is loaded first and its terrain would now get ids 100,101, ...

[codemime]

Summary (apart from the stated above):

Correct me if I'm wrong, but currently we have two equal copies of each loaded ter_t object (one in termap and another one in terlist). Same for the furniture. This PR eliminates duplication for the terrain.

Using cached int is approx 50-70 times faster, than hashmap lookup (~25 times faster if it needs to recheck the index, but it shall only occur when objects are deleted/reloaded). We don't need to worry about performance loss anymore (previous loadid was addressing this problem), just pass a string_id variable which can be converted to int/obj pretty fast after the first usage. The cached int has no overhead and is protected against invalidation:

// the first condition checks int range, the second checks matching
if( tid.is_valid() && terlist[tid].id == *this ) { ...

Also, I don't see any reason why we should distinguish empty-ids ("") and null-ids ("t_null" etc). It's just a source of complexity and confusion:

if( !ter.open.empty() && ter.open != "t_null" ) { ...

Any optional id can be initially set to NULL_ID, which means emptiness. Then any id which is not NULL_ID is a valid non-empty identifier. The whole condition can be rewritten as:

if( ter.open.is_null() ) { ...
// or even
if( ter.open ) { ... // omit is_null() as BevapDin proposed

My next goal is making the generic_factoryclass responsible for the routine functions and cache-management (concrete identifiers may or may not use it). If they use it, they'd just let the generic_factory do the job:

template<>
bool string_id<ter_t>::is_valid() const
{
    return terrain_factoy.is_valid( *this );
}
template<>
int_id<ter_t> string_id<ter_t>::id() const
{
    return terrain_factoy.get_int_id( *this );
}

Also, forthcoming PRs will decouple terrain and furniture further.

[codemime]

Anything else to do before it's mergeable? A couple of my next PRs depend on it.

[Coolthulhu]

Has a merge conflict, I assume due to the astyling

[Coolthulhu]

I'll mark it as a priority to make it stand out despite being "buried" under other PRs.

If you have something important depending on it, you can branch it out of this one and note in the PR that you did that.

[BevapDin]

Why are these includes needed here? I can't see anything in this files that uses their content.

[codemime]

Well spotted. Thanks. I had added them for debugging purposes, but then forgot to delete.

[BevapDin]

It looks fined, besides the strange includes. Are they really needed, it compiles and runs fine without them, for me.

[codemime]

Rebased against master and represents changes after the latest @BevapDin's review.
@Coolthulhu Thanks for the label.