Draft: dig-now plugin: remove dependencies on MapCache module.#5832
Draft
SilasD wants to merge 9 commits into
Draft
Draft: dig-now plugin: remove dependencies on MapCache module.#5832SilasD wants to merge 9 commits into
SilasD wants to merge 9 commits into
Conversation
this is intended to be a naive direct-replacement with minimal changes to algorithms. this commit is the easy stuff: remove MapExtras::MapCache &map parameter from functions change DFCoord to df::coord change MapCache::ensureBlockAt to Maps::ensureTileBlock change MapCache::BlockAtTile to Maps::getTileBlock change MapCache::tiletypeAt to *Maps::getTileType change MapCache::getDesignationAt to *Maps::getTileDesignation change MapCache::setDesignationAt to *Maps::getTileDesignation change MapCache::occupancyAt to *Maps::getTileOccupancy flag MapCache::layerMaterialAt for further work flag MapCache::baseMaterialAt for further work flag Block::veinTypeAt for further work flag Block::setStoneAt for further work flag Block::setSoilAt for further work flag DesignationJobs for further work add several now-necessary headers.
this is a naive-but-correct implementation with no caching.
cherry-pick from branch dig-now-test
ready to start code review.
turns out I'm not allowed to have unused functions.
so much for forward planning.
also, apparently 0 is signed! who would have known?
In file included from /home/runner/work/dfhack/dfhack/library/include/Error.h:31,
from /home/runner/work/dfhack/dfhack/library/include/BitArray.h:26,
from /home/runner/work/dfhack/dfhack/library/include/DataDefs.h:37,
from /home/runner/work/dfhack/dfhack/library/include/DataIdentity.h:38,
from /home/runner/work/dfhack/dfhack/library/include/DataFuncs.h:30,
from /home/runner/work/dfhack/dfhack/plugins/dig-now.cpp:5:
/home/runner/work/dfhack/dfhack/library/include/MiscUtils.h: In instantiation of ‘T clip_range(T, T1, T2) [with T = long unsigned int; T1 = int; T2 = long unsigned int]’:
/home/runner/work/dfhack/dfhack/plugins/dig-now.cpp:108:23: required from here
/home/runner/work/dfhack/dfhack/library/include/MiscUtils.h:553:11: error: comparison of integer expressions of different signedness: ‘long unsigned int’ and ‘int’ [-Werror=sign-compare]
553 | if (a < minv) return minv;
| ~~^~~~~~
ab9rf
reviewed
Jun 20, 2026
| return t_matpair(INORGANIC, geo_biome->layers[layer]->mat_index); | ||
| } | ||
|
|
||
| #if false // gcc warns about unused functions, and warnings are errors. |
Member
There was a problem hiding this comment.
use [[maybe_unused]] here to suppress the warning - the compiler will not warn about the function being unused but will still validate its syntax which doesn't happen with the #if
long term, if this is actually dead code it should simply be removed, there's no need to preserve it for reference or legacy purposes as it'll be forever in the repo change history
Contributor
Author
There was a problem hiding this comment.
Thank you for the compiler directive.
I'm uncertain if it is dead code or not; I originally wrote these functions wiuth the code combined into one function each, then decided to split out the map_block and local pos cases.
I will maintain it for the moment, but my long-term intent is to move all the code to block-centric algorithms, so this will likely be permanently dead code and I will remove it then.
ab9rf
reviewed
Jul 11, 2026
Comment on lines
+64
to
+389
| static const df::region_map_entry* biome_at(const df::map_block &block, const df::coord2d p) | ||
| { | ||
| auto blockptr = &static_cast<df::map_block &>(const_cast<df::map_block &>(block)); | ||
| auto biome = Maps::getRegionBiome(Maps::getBlockTileBiomeRgn(blockptr, p)); | ||
| return biome; | ||
| } | ||
|
|
||
| #if false // gcc warns about unused functions, and warnings are errors. | ||
| static const df::region_map_entry* biome_at(const df::coord pos) | ||
| { | ||
| auto block = Maps::getTileBlock(pos); | ||
| if (!block) | ||
| return nullptr; | ||
| auto blockref = static_cast<const df::map_block &>(*block); | ||
| return biome_at(blockref, df::coord2d(pos) & 15); | ||
| } | ||
| #endif | ||
|
|
||
| static const df::world_geo_biome* geo_biome_at(const df::map_block &block, const df::coord2d p) | ||
| { | ||
| auto biome = biome_at(block, p); | ||
| if (!biome) | ||
| return nullptr; | ||
| if (biome->geo_index < 0) | ||
| return nullptr; | ||
| if (static_cast<size_t>(biome->geo_index) >= df::world_geo_biome::get_vector().size()) | ||
| return nullptr; | ||
| auto geo_biome = df::world_geo_biome::get_vector()[biome->geo_index]; | ||
| return geo_biome; | ||
| } | ||
|
|
||
| #if false // gcc warns about unused functions, and warnings are errors. | ||
| static const df::world_geo_biome* geo_biome_at(const df::coord pos) | ||
| { | ||
| auto block = Maps::getTileBlock(pos); | ||
| if (!block) | ||
| return nullptr; | ||
| auto blockref = static_cast<const df::map_block &>(*block); | ||
| return geo_biome_at(blockref, df::coord2d(pos) & 15); | ||
| } | ||
| #endif | ||
|
|
||
| static const t_matpair layer_inorganic_n(const df::map_block &block, const df::coord2d p, size_t layer) | ||
| { | ||
| using namespace df::enums::builtin_mats; | ||
| auto geo_biome = geo_biome_at(block, p); | ||
| if (!geo_biome) | ||
| return t_matpair(INORGANIC, -1); // can't happen, generic "rock" | ||
| // gcc-11 complained that 0 was of different signed-ness than size_t! trying to cast it to unsigned. | ||
| layer = clip_range(layer, static_cast<size_t>(0), geo_biome->layers.size() - 1); | ||
| return t_matpair(INORGANIC, geo_biome->layers[layer]->mat_index); | ||
| } | ||
|
|
||
| #if false // gcc warns about unused functions, and warnings are errors. | ||
| static const t_matpair layer_inorganic_n(const df::coord pos, size_t layer) | ||
| { | ||
| using namespace df::enums::builtin_mats; | ||
| auto block = Maps::getTileBlock(pos); | ||
| if (!block) | ||
| return t_matpair(INORGANIC, -1); // can't happen, generic "rock" | ||
| auto blockref = static_cast<const df::map_block &>(*block); | ||
| return layer_inorganic_n(blockref, df::coord2d(pos) & 15, layer); | ||
| } | ||
| #endif | ||
|
|
||
| static const size_t geolayer_at(const df::map_block &block, df::coord2d p) | ||
| { | ||
| return index_tile(block.designation, p).bits.geolayer_index; | ||
| } | ||
|
|
||
| static const t_matpair layer_inorganic_at(const df::map_block &block, df::coord2d p) | ||
| { | ||
| return layer_inorganic_n(block, p, geolayer_at(block, p)); | ||
| } | ||
|
|
||
| #if false // gcc warns about unused functions, and warnings are errors. | ||
| static const t_matpair layer_inorganic_at(const df::coord pos) | ||
| { | ||
| using namespace df::enums::builtin_mats; | ||
| auto block = Maps::getTileBlock(pos); | ||
| if (!block) | ||
| return t_matpair(INORGANIC, -1); // generic "rock" | ||
| auto blockref = const_cast<const df::map_block &>(static_cast<df::map_block &>(*block)); | ||
| return layer_inorganic_at(blockref, df::coord2d(pos) & 15); | ||
| } | ||
| #endif | ||
|
|
||
| static const df::enums::tiletype_material::tiletype_material getGroundType(int32_t mat_index) | ||
| { | ||
| if (isSoilInorganic(mat_index)) | ||
| return df::enums::tiletype_material::SOIL; | ||
| if (isStoneInorganic(mat_index)) | ||
| return df::enums::tiletype_material::STONE; | ||
| return df::enums::tiletype_material::NONE; | ||
| } | ||
|
|
||
| static const df::enums::tiletype_material::tiletype_material getGroundType(const t_matpair matpair) | ||
| { | ||
| using namespace df::enums::builtin_mats; | ||
| if (matpair.mat_type != INORGANIC) | ||
| return df::enums::tiletype_material::NONE; | ||
| return getGroundType(matpair.mat_index); | ||
| } | ||
|
|
||
| // copied from Maps, used by getBiomeRgnPos() | ||
| static df::coord2d biome_offsets[9] = { | ||
| df::coord2d(-1,-1), df::coord2d(0,-1), df::coord2d(1,-1), | ||
| df::coord2d(-1,0), df::coord2d(0,0), df::coord2d(1,0), | ||
| df::coord2d(-1,1), df::coord2d(0,1), df::coord2d(1,1) | ||
| }; | ||
|
|
||
| // copied from Maps. arguably this should be exported from Maps; it's useful. | ||
| inline df::coord2d getBiomeRgnPos(const df::coord2d base, BiomeOffset idx) | ||
| { | ||
| auto r = base + biome_offsets[idx]; | ||
|
|
||
| int world_width = world->world_data->world_width; | ||
| int world_height = world->world_data->world_height; | ||
|
|
||
| return df::coord2d(clip_range(r.x, 0, world_width-1), | ||
| clip_range(r.y, 0, world_height-1)); | ||
| } | ||
|
|
||
| // copied and modified from MapCache getBaseMaterial(). | ||
| // returns the material that should be used for boulders/gems, | ||
| // or the invalid material t_matpair(-1). | ||
| static const t_matpair baseMaterialAt(const df::map_block &block, df::coord2d p) | ||
| { | ||
| using namespace df::enums::builtin_mats; | ||
| using namespace df::enums::tiletype_material; | ||
|
|
||
| p = p & 15; | ||
| t_matpair rv; | ||
| auto tt = index_tile(block.tiletype, p); | ||
| switch (tileMaterial(tt)) { | ||
| // not diggable, should not drop boulders. | ||
| case df::enums::tiletype_material::NONE: | ||
| case AIR: | ||
| case CONSTRUCTION: | ||
| case HFS: | ||
| case CAMPFIRE: | ||
| case FIRE: | ||
| case MAGMA: | ||
| case POOL: | ||
| case RIVER: | ||
| case TREE: | ||
| case ROOT: // TODO this is a bug; DF can dig roots. | ||
| case MUSHROOM: | ||
| case UNDERWORLD_GATE: | ||
| rv = t_matpair(-1); | ||
| break; | ||
|
|
||
| // diggable but should not drop boulders, so return the first layer, presumably soil. | ||
| // (these won't drop anyway because they are not WALL or FORTIFICATION shaped, but safety first.) | ||
| case DRIFTWOOD: | ||
| case BROOK: | ||
| case ASHES: | ||
| case PLANT: | ||
| case GRASS_LIGHT: | ||
| case GRASS_DARK: | ||
| case GRASS_DRY: | ||
| case GRASS_DEAD: | ||
| rv = layer_inorganic_n(block, p, 0); | ||
| break; | ||
|
|
||
| case STONE: | ||
| { | ||
| rv = t_matpair(INORGANIC, -1); // fallback: generic "rock" | ||
| auto geo_biome = geo_biome_at(block, p); | ||
| if (!geo_biome) | ||
| break; | ||
|
|
||
| // IS THIS CODE CORRECT? | ||
| // intent here is to index 0..15 into a vector with 13 or 16 elements | ||
| // and get the element at min(index, vector length-1) . | ||
| // ranges::advance seems like the right tool. | ||
| auto geo_layer_it = begin(geo_biome->layers); | ||
| std::ranges::advance(geo_layer_it, geolayer_at(block, p), end(geo_biome->layers) - 1); | ||
| rv = t_matpair(INORGANIC, (*geo_layer_it)->mat_index); | ||
| if (getGroundType(rv) == STONE) | ||
| break; | ||
|
|
||
| // fall back to the first stone layer, or the very last layer. | ||
|
|
||
| // IS TNIS CODE CORRECT? | ||
| // intent here is to search a vector with 13 or 16 elements | ||
| // for the first element matching a predicate, and get that element | ||
| // OR the last element in the vector, whether or not it matches. | ||
| // all this is a lot of faffing around for something that's more | ||
| // concise in C style C++; see the SOIL code below. | ||
| auto is_stone = [&](df::world_geo_layer *geo_layer) | ||
| { return getGroundType(geo_layer->mat_index) == STONE; }; | ||
| // this vector typically has 16 entries, but I have seen 13 in oceans. | ||
| auto &geo_layers = geo_biome->layers; | ||
| geo_layer_it = std::find_if(begin(geo_layers), end(geo_layers) - 1, is_stone); | ||
| rv = t_matpair(INORGANIC, (*geo_layer_it)->mat_index); | ||
| break; | ||
| } | ||
|
|
||
| case SOIL: | ||
| rv = layer_inorganic_at(block, p); | ||
| if (getGroundType(rv) != SOIL) { | ||
| // fall back to the last soil layer, or the very first layer. | ||
| for (auto i = 15; i >= 0; i--) { | ||
| rv = layer_inorganic_n(block, p, i); // this call clips to vector size. | ||
| if (getGroundType(rv) == SOIL) | ||
| break; | ||
| } | ||
| } | ||
| break; | ||
|
|
||
| case FEATURE: | ||
| { | ||
| // fallback: no feature flag, or no event? no drops. | ||
| rv = t_matpair(-1); | ||
| auto des = index_tile(block.designation, p); | ||
| t_feature feature; | ||
| feature.type = df::enums::feature_type::feature_type::NONE; | ||
| if (des.bits.feature_local) | ||
| Maps::ReadFeatures( | ||
| &(static_cast<df::map_block &>(const_cast<df::map_block &>(block))), | ||
| &feature, nullptr); | ||
| else if (des.bits.feature_global) | ||
| Maps::ReadFeatures( | ||
| &(static_cast<df::map_block &>(const_cast<df::map_block &>(block))), | ||
| nullptr, &feature); | ||
| if (feature.type != df::enums::feature_type::feature_type::NONE) | ||
| rv = t_matpair(feature.main_material, feature.sub_material); | ||
| break; | ||
| } | ||
|
|
||
| case LAVA_STONE: | ||
| { | ||
| auto region_details = world->world_data->midmap_data.region_details; | ||
| auto des = index_tile(block.designation, p); | ||
| auto block_region_offset_idx = static_cast<BiomeOffset>(des.bits.biome); | ||
| if (block_region_offset_idx >= eBiomeCount) | ||
| // "can't happen", fall back to the local region tile's biome. | ||
| block_region_offset_idx = eHere; | ||
| auto blockptr = &static_cast<df::map_block &>(const_cast<df::map_block &>(block)); | ||
| auto region_details_coord2d = Maps::getBlockTileBiomeRgn(blockptr, p); | ||
| auto region_details_idx = linear_index(region_details, | ||
| &df::world_region_details::pos, region_details_coord2d); | ||
| if (region_details_idx == -1) | ||
| { | ||
| // "can't happen"; fall back to the first region in the region_details vector. | ||
| DEBUG(general).print("BaseMaterialAt(block {}, tile {}, case " | ||
| "LAVA_STONE, didn't find region_details for region coord {}\n", | ||
| block.map_pos, p, region_details_coord2d); | ||
| region_details_idx = 0; | ||
| } | ||
| rv = t_matpair(INORGANIC, region_details[region_details_idx]->lava_stone); | ||
| break; | ||
| } | ||
|
|
||
| case MINERAL: | ||
| // fall back to the layer material, whether SOIL or STONE. | ||
| rv = layer_inorganic_at(block, p); | ||
| for (auto event : block.block_events) { | ||
| if (event->getType() == df::block_square_event_type::mineral) { | ||
| auto vein = (df::block_square_event_mineralst *)event; | ||
| if (vein->getassignment(p)) { | ||
| rv = t_matpair(INORGANIC, vein->inorganic_mat); | ||
| // do not early-out. later mineral events can override earlier ones. | ||
| } | ||
| } | ||
| } | ||
| break; | ||
|
|
||
| case FROZEN_LIQUID: | ||
| rv = t_matpair(WATER, 0); | ||
| break; | ||
|
|
||
| // no default so we get a compiler warning. | ||
| } | ||
|
|
||
| if (rv == t_matpair(-1)) | ||
| return t_matpair(-1); | ||
| MaterialInfo mi; | ||
| mi.decode(rv); | ||
| if (mi.isValid() && mi.material->flags.is_set(df::material_flags::UNDIGGABLE)) | ||
| return t_matpair(-1); | ||
| return rv; | ||
| } | ||
|
|
||
| static const t_matpair baseMaterialAt(df::coord pos) | ||
| { | ||
| auto block = Maps::getTileBlock(pos); | ||
| if (!block) | ||
| return t_matpair(-1); | ||
| auto blockref = const_cast<const df::map_block &>(static_cast<df::map_block &>(*block)); | ||
| return baseMaterialAt(blockref, df::coord2d(pos) & 15); | ||
| } | ||
|
|
||
| static const df::inclusion_type veinTypeAt(df::map_block &block, df::coord2d p) | ||
| { | ||
| using namespace df::enums::inclusion_type; | ||
| auto veintype = TOTAL; | ||
| for (auto event : block.block_events) { | ||
| if (event->getType() == df::block_square_event_type::mineral) { | ||
| auto vein = (df::block_square_event_mineralst *)event; | ||
| if (vein->getassignment(p)) { | ||
| if (vein->flags.bits.cluster_one) | ||
| veintype = CLUSTER_ONE; | ||
| else if (vein->flags.bits.cluster_small) | ||
| veintype = CLUSTER_SMALL; | ||
| else if (vein->flags.bits.vein) | ||
| veintype = VEIN; | ||
| else if (vein->flags.bits.cluster) | ||
| veintype = CLUSTER; | ||
| // do not early-out. later mineral events can override earlier ones. | ||
| } | ||
| } | ||
| } | ||
| return veintype; | ||
| } | ||
|
|
||
| static const df::inclusion_type veinTypeAt(df::coord pos) | ||
| { | ||
| auto block = Maps::getTileBlock(pos); | ||
| if (!block) | ||
| return df::enums::inclusion_type::TOTAL; | ||
| auto blockref = const_cast<const df::map_block &>(static_cast<df::map_block &>(*block)); | ||
| return veinTypeAt(blockref, df::coord2d(pos) & 15); | ||
| } | ||
|
|
Member
There was a problem hiding this comment.
Should we be adding some or all of these as custom methods to the relevant codegen types? This is something we can do.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I request code review on this draft PR, with a focus on
dig-now.cpplines 60 to 380 or so, which contain new code, and thedig_type()function in lines 624-682, which is a complete reimplementation.I would like this code to be up to modern C++ standards, and I know that my C++ knowledge and skills are not the best.
The remainder of the file is old code updated with different types, e.g.
df::coordinstead ofDFCoord, and removal of all references to theMapExtras::MapCache &mapobject.There are also a couple of minor bugfixes, in particular in the
dig_tile()functionChannelcase.I don't think that part of the plugin needs full review yet, but I do welcome comments.
LuaLS annotations were added to
dig-now.lua. No code changes.General notes:
This PR is intended to be a naive-but-correct reimplementation of dig-now using the Maps module instead of the MapCache module. Note that it does not have the caching that MapCache provided. This results in a lot of unnecessary info lookups.
This PR is known to have bugs. These bugs already existed in the old version. They will be fixed in the near future.
This code has been verified to be almost bug-for-bug compatible with the old version. It results in the same map state with these exceptions:
map_block.flags.designated. This new version replicates the buggy behavior, but it properly clears that flag. I can't be bothered to track down the exact line and state to replicate that tiny bug, when I expect to fix the larger bug soon.clean_ramp()function. This has been seen on tower-cap cap ramps, but has not been verified for all undiggable ramps.Intended future directions, roughly in this order:
Maps::cuboid::forBlock(). Currently it is a naivelayer-by-layer, column-by-column, row-by-row scan.