Cataclysm-dda: map: refactor map class to get rid of optional z-levels

[mlangsdorf]

Summary

the map class and associated classes needs to be refactored. Here's my proposal:

level_cache becomes a class and gets setter and getter functions for all members which are now private. There may be a child class called fake_level_cache used by the remote_map class (see below) which ignores or simplifies some caching operations.

map and tinymap change quite a bit:

• abstract_map is the parent map class. It gets all the terrain/furniture/trap setting and inspection functions such as furn() and ter_set(), but none of the processing functions like move_vehicle(). It should be possible to run mapgen and update_mapgen on an abstract_map. There is some level of caching in abstract_map but the exact amount will need to be determined and should not include the visibility cache, for instance.
• reality_bubble (or maybe local_map or viewport_map) is a child of abstract_map. It has all the functions and caching of the current map class.
• remote_map is a replacement for tinymap and a child of abstract_map. It provides a way to do map inspection and editing of map tiles that are not in the reality bubble. It doesn't have any processing functions.

There may need to be a 4th class, map_slice that gets returned instead of a remote_map when the requested overmap tile is already in the reality bubble. Or perhaps the remote_map loader will detect that condition and use different caching functions. There needs to be a way to edit overmap tiles within the reality bubble (using map local co-ordinates) but have the reality bubble's caches get updated.

This refactoring is going to conflict a lot with #41693 and I don't intend to work on coding it until 41693 is mostly merged.

In the meantime, I invite critique of this plan.

Details

tinymap and remote_map

tinymap is currently a child class of map. It is used exclusively to load a single overmap tile's worth of submaps (on a single z-level), perform some inspection or alteration of those submaps, and possibly save the results.

tinymap benefits:

• allows for the inspection and alteration of submaps that are outside the reality bubble - this is required for for mapgen_update and also simplifies the translocation code in Magiclysm.
• map editing is done in mapgen co-ordinates instead of global co-ordinates or co-ordinates local to avatar.

tinymap has some problems currently:

• because only a single overmap tile is typically loaded, effects that alter the map above or map below cause problems. vehicles can't be placed on the edges of a tinymap because the adjacent submaps are not loaded.
• tinymaps take the full overhead of caching, even though the cache is discarded as soon as the function calling the tinymap is complete.
• a tinymap can load and alter an overmap tile that is within the reality bubble. Changes to the tinymap's level_cache are not reflected in the reality bubble's level_cache, which results in visual glitches until some event refreshes the reality bubble's caches.

remote_map is intended to fix these problems while keeping the benefits of tinymap.

[jbytheway]

I agree with most of this, but I'm not convinced there's much benefit to making level_cache access go via getters and setters. I think this is coming at it from the wrong angle. Caches work well when they are an implementation detail of some class which presents a higher-level interface. Methods on that class should update the cache appropriately, and clients of that class shouldn't need to care that the cache exists. That's not the same as protecting access to the cache data itself.

To put it another way, if the proposed getters and setters are doing nothing more than getting or setting values in level_cache then they serve no purpose; it's just a more roundabout way to achieve the same end. If the getters and setters are needed to maintain class invariants, then they are useful.

[Zireael07]

+100 to having an explicitly named reality_bubble class.

[mlangsdorf]

I haven't worked through the code yet, but I think that remote_map is going to use base_map calls that interact with the cache but that won't need to update the cache. So yes, this is about maintaining class invariance.

I also think the cache is better served by being a black box to the map classes, but I concede that's a more abstract point.

[jbytheway]

I think I was imagining that the base_map calls would be virtual and reality_bubble would override those functions to add the cache manipulations needed. In other words, putting the polymorphism at the map interface rather than the cache interface, as you seem to be imagining.

Of course, I haven't worked through any code either, so I don't know which approach would be cleaner.