Configurable mine resource behavior by DevOpsOfChaos · Pull Request #1942 · Return-To-The-Roots/s25client

DevOpsOfChaos · 2026-05-21T21:20:37Z

Summary

This PR implements configurable mine resource behavior as a cleaned-up replacement path for the broader #1501 direction.

It adds per-mine resource behavior settings for coal, iron, gold, and granite mines, while reusing existing inexhaustible mine addon IDs where appropriate instead of stacking additional addons on top.

Details

Add configurable mine resource behavior per mine type:
- Default
- S4-like exhaustion
- Inexhaustible
- Work everywhere
Reuse existing INEXHAUSTIBLE_* addon IDs as per-mine behavior list settings where possible
Map old boolean ON values to the new Inexhaustible behavior
Migrate legacy global INEXHAUSTIBLE_MINES settings to per-mine inexhaustible behavior when loading old settings
Remove the separate granite work-everywhere addon path
Treat Work everywhere as ignoring ground-resource requirements instead of creating resources in the world
Add configurable no-output fallback behavior for failed S4-like production cycles
Teach AI resource rating and mine planning about configured mine behavior
Adjust displayed mine productivity for S4-like resource chance
Add integration coverage for production behavior, AI behavior, serialization/migration, and productivity display

S4-like behavior

For S4-like behavior:

each mine type can be configured independently
a mine attempts production based on remaining matching resources in its mining radius
20 remaining matching resources is the explicit full-productivity reference
below that reference, production chance is reduced proportionally
successful production depletes resources down to 1, not to 0
failed production runs a no-output cycle
the no-output cycle can either produce nothing or use the configured fallback production behavior

Work-everywhere behavior

Work everywhere means that the mine skips the ground-resource requirement.

It does not create temporary resources in the world and does not deplete such generated resources.

Validation

clang-format version 10.0.0
Built Test_integration
Built Test_UI
Passed Test_integration --run_test=Production
Passed Test_integration --run_test=AI
Passed Test_integration --run_test=Serialization
Passed ctest --test-dir .\build-vs-x64-debug-local -C Debug -R "^Test_integration$" --output-on-failure
Passed Test_UI.exe --report_level=short
git diff --check upstream/master...HEAD passed

Flamefire

Thanks for taking care of that.
Please remove duplications and check back with what was discussed before. This includes:

There is no need for multiple addons for a single mine type: GRANITEMINES_WORK_EVERYWHERE is superflous and all the existing "inexhaustible*" addons should be integrated into this ones and use the same IDs: The OFF setting should be "Default" and ON (the new) "inexhaustible"
FindPointWithResourceQuiet duplicates code already present -> Reuse existing one, can be done by simply adding a default bool parameter notify
INEXHAUSTIBLE_MINES addon should be removed: When detected during loading it should set the new addons accordingly, keep ID for that with a TODO for gamedata version increasing
Explain CalcAverageDisplayProductivity please, why the extra methods?
Isn't there already a method to convert AIResource to Resource so GetMineBuildingType doesn't need to be (fully) implemented twice?
CanCreateWorkEverywhereResource looks odd: "Work everywhere" should be just that: Ignore any resource in the ground, not add resources to the world, doesn't it? So just skip the search
IsMineResourceDepletable shouldn't use GetConfiguredMineResourceBehavior, in fact it seems only a single function is required, i.e. both Get... combined.

Maybe more.

Please give a quick summary of how "s4 like behavior" is supposed to work, so @Spikeone can verify it is what he intended in #1501 and we can verify it is implemented according to that spec.

DevOpsOfChaos · 2026-05-26T12:33:45Z

Thanks for taking care of that. Please remove duplications and check back with what was discussed before. This includes:

There is no need for multiple addons for a single mine type: GRANITEMINES_WORK_EVERYWHERE is superflous and all the existing "inexhaustible*" addons should be integrated into this ones and use the same IDs: The OFF setting should be "Default" and ON (the new) "inexhaustible"

FindPointWithResourceQuiet duplicates code already present -> Reuse existing one, can be done by simply adding a default bool parameter notify

INEXHAUSTIBLE_MINES addon should be removed: When detected during loading it should set the new addons accordingly, keep ID for that with a TODO for gamedata version increasing

Explain CalcAverageDisplayProductivity please, why the extra methods?

Isn't there already a method to convert AIResource to Resource so GetMineBuildingType doesn't need to be (fully) implemented twice?

CanCreateWorkEverywhereResource looks odd: "Work everywhere" should be just that: Ignore any resource in the ground, not add resources to the world, doesn't it? So just skip the search

IsMineResourceDepletable shouldn't use GetConfiguredMineResourceBehavior, in fact it seems only a single function is required, i.e. both Get... combined.

Maybe more.

Please give a quick summary of how "s4 like behavior" is supposed to work, so @Spikeone can verify it is what he intended in #1501 and we can verify it is implemented according to that spec.

Thanks, that direction makes sense.

My understanding of the requested cleanup is:

fold the existing INEXHAUSTIBLE_* addon IDs into the new per-mine behavior list settings
keep OFF == Default and map the old ON value to the new Inexhaustible behavior
remove the extra granite-specific work-everywhere addon path
remove the old global INEXHAUSTIBLE_MINES behavior as an active setting and migrate it during loading to the new per-mine settings, keeping its ID only for compatibility/TODO until the gamedata version can be increased
avoid helper duplication such as FindPointWithResourceQuiet
treat WorkEverywhere as “ignore ground resources”, not “create resources in the world”
simplify the effective behavior/depletion helper logic

My current S4-like behavior implementation is intended to work like this:

each mine type can be configured independently
for S4-like behavior, a mine can attempt production based on the remaining matching resources in its mining radius
the production chance is derived from remaining resource amount compared to the relevant possible resource capacity around the mine
resources are depleted only down to 1, not to 0
if the chance check fails, the mine runs a no-output cycle
the no-output cycle can either produce nothing or use the configured fallback production behavior

I’ll rework the implementation toward the existing-ID/list-setting approach and remove the duplicated/intermediate pieces instead of stacking this on top of the old addon set.

Flamefire · 2026-05-26T14:05:30Z

for S4-like behavior, a mine can attempt production based on the remaining matching resources in its mining radius

the production chance is derived from remaining resource amount compared to the relevant possible resource capacity around the mine

resources are depleted only down to 1, not to 0

if the chance check fails, the mine runs a no-output cycle

the no-output cycle can either produce nothing or use the configured fallback production behavior

@Spikeone Looks correct to me. Can you confirm?

"possible resource capacity" isn't fully clear to me. There are 7 points considered and currently each resource gives 5% chance. I.e. when all have at least 3 resources we will have 100%. IIRC the max amount of resources per node is 7 (we should have a constant for that if we don't already have one)
The code should be clear here, the 5% looks random. The chance could be e.g. directly calculated from the points and use the number of points and max amount. Or the constant derived from something like that.

DevOpsOfChaos · 2026-05-26T16:25:31Z

for S4-like behavior, a mine can attempt production based on the remaining matching resources in its mining radius

the production chance is derived from remaining resource amount compared to the relevant possible resource capacity around the mine

resources are depleted only down to 1, not to 0

if the chance check fails, the mine runs a no-output cycle

the no-output cycle can either produce nothing or use the configured fallback production behavior

@Spikeone Looks correct to me. Can you confirm?

"possible resource capacity" isn't fully clear to me. There are 7 points considered and currently each resource gives 5% chance. I.e. when all have at least 3 resources we will have 100%. IIRC the max amount of resources per node is 7 (we should have a constant for that if we don't already have one) The code should be clear here, the 5% looks random. The chance could be e.g. directly calculated from the points and use the number of points and max amount. Or the constant derived from something like that.

Good point. The current implementation intentionally models the existing S4-like “full productivity around ~20 remaining resources” behavior rather than deriving 100% from the absolute theoretical map capacity.

The current 5% per remaining resource effectively means:

20 remaining matching resources => 100% production chance
depletion continues reducing the chance below that point

I agree the current formulation makes the 5% look arbitrary in code and in the PR description.

I will rework this to express the behavior through named constants / derived calculation so the intended reference point becomes explicit instead of looking like a magic number.

Flamefire · 2026-05-27T08:46:25Z

Good point. The current implementation intentionally models the existing S4-like “full productivity around ~20 remaining resources” behavior rather than deriving 100% from the absolute theoretical map capacity.

Yes, its 100% at (exactly) 20 resources, but why 20 and not 10, 30 or 40 when 49 is the theoretical maximum? Or even as low as 7 which would be 1 node with full resources.
I'm not decided on what makes sense, but it needs to be explained in the code where the constant is defined with a short sentence.
And as the initial version was by @Spikeone I'd follow his preference if he has any.

DevOpsOfChaos · 2026-06-03T15:24:18Z

Updated the PR to address the requested architecture cleanup and the S4-like chance clarity point.

Main changes:

removed the separate GRANITEMINES_WORK_EVERYWHERE addon path
reused INEXHAUSTIBLE_GRANITEMINES as the granite mine behavior list ID
old boolean value 1 now maps to Inexhaustible
removed active registration/use of global INEXHAUSTIBLE_MINES
legacy enabled global INEXHAUSTIBLE_MINES settings are migrated to per-mine Inexhaustible selections when loading/deserializing settings
replaced the duplicated quiet resource lookup with FindPointWithResource(..., false)
simplified mine behavior helper logic
corrected Work everywhere semantics so it skips resource checks instead of creating/depleting world resources
kept S4-like behavior and no-output fallback behavior
made the S4-like chance reference explicit via a named 20-resource full-productivity reference instead of a buried “5% per resource” rule
updated Production, AI, Serialization, and UI/productivity tests

S4-like behavior after the rework:

each mine type is configured independently
production chance is based on remaining matching resources in the mining radius
20 remaining matching resources is the explicit full-productivity reference
successful production depletes resources down to 1, not 0
failed production runs the configured no-output/fallback cycle

Validation:

clang-format version 10.0.0
built Test_integration
built Test_UI
Test_integration --run_test=Production passed
Test_integration --run_test=AI passed
Test_integration --run_test=Serialization passed
ctest -R "^Test_integration$" passed
Test_UI.exe --report_level=short passed
git diff --check upstream/master...HEAD clean

Flamefire

Most things are now readability:

TODO(Replay) and/or TODO(Savegame) allows to quickly find all places
"display"productivity seems odd
less code is usually better: Reduce C&P and avoid enum constraints where not necessary or document why
The new notify param for FindPointWithResource now looks redundant so notification might get moved out which is better for SRP as a "find" should not "notify"

Flamefire · 2026-06-07T10:09:38Z

+ *  now selects the inexhaustible behavior.
 */
-class AddonInexhaustibleGraniteMines : public AddonBool
+class AddonInexhaustibleGraniteMines : public AddonMineResourceBehaviorBase


Please move this class to the other ones to avoid missing it. And you can rename the addon id to match the new ones if you keep the value.

Flamefire · 2026-06-07T10:12:11Z


 /**
- *  Addon for allowing to have unlimited resources.
+ *  Deprecated global mine setting.


Add TODO(Replay) TODO(Savegame) so it can be found when updating

Flamefire · 2026-06-07T10:12:34Z

 //
 // Add the #include for your AddonXXX.h in Addons.h!
 //
+// TODO: INEXHAUSTIBLE_MINES is kept only for loading old settings/savegames until the gamedata version can be raised.


Suggested change

// TODO: INEXHAUSTIBLE_MINES is kept only for loading old settings/savegames until the gamedata version can be raised.

// TODO(Replay) TODO(Savegame): INEXHAUSTIBLE_MINES is kept only for loading old settings/savegames until the gamedata version can be raised.

Flamefire · 2026-06-07T10:17:43Z

+
+MineResourceBehavior GetMineResourceBehavior(const GlobalGameSettings& settings, const BuildingType buildingType)
+{
+    switch(static_cast<MineResourceBehavior>(settings.getSelection(GetMineResourceBehaviorAddonId(buildingType))))


What is the reason for this switch? I should at least be explained by a short comment.
If it is only validation (why necessary?) then I'd rather have a auto result = ...; switch(result){case X:caseY:case Z:break /*or return*/; default: result=default}-like code to avoid having to read all this C&P

Flamefire · 2026-06-07T10:20:18Z

+    }
+}
+
+unsigned GetS4LikeMineFullProductivityResourceAmount()


Why have a function that returns a constant?

Flamefire · 2026-06-07T10:36:18Z

+
+MineNoOutputFallback GetConfiguredNoOutputFallback(const GlobalGameSettings& settings)
+{
+    switch(static_cast<MineNoOutputFallback>(settings.getSelection(AddonId::MINE_NO_OUTPUT_FALLBACK)))


Similar to the other method: At least avoid C&P by using a variable and directly returning or doing nothing in the "success" case

Flamefire · 2026-06-07T10:40:53Z

+
+std::vector<MapPoint> GetPointsWithResource(const GameWorld& world, const MapPoint pos, const ResourceType type)
+{
+    return world.GetMatchingPointsInRadius<1>(


This will only return a single point, which is not what you want, do you?

Flamefire · 2026-06-07T10:45:53Z

+    if(FindPointWithResource(GetRequiredResType(), false).isValid())
+        return true;
+
+    workplace->OnOutOfResources();
+    return false;


Why not

Suggested change

if(FindPointWithResource(GetRequiredResType(), false).isValid())

return true;

workplace->OnOutOfResources();

return false;

return FindPointWithResource(GetRequiredResType()).isValid();

Wasn't that the reason for the notify param? Or is that not required and we could always notify in the caller (which might be cleaner anyway)?

Flamefire · 2026-06-07T10:49:33Z

+
+#pragma once
+
+enum class MineNoOutputFallback : unsigned


do you need this to have an explicit type? Or is the default enough, i.e.:

Suggested change

enum class MineNoOutputFallback : unsigned

enum class MineNoOutputFallback

Only cases where I can imagine explicit types are serialized enums that need a fixed-size type or when you rely on overflow etc.

Flamefire · 2026-06-07T10:50:50Z

+
+enum class MineResourceBehavior : unsigned
+{
+    Default = 0,


Why the explicit type and values?

DevOpsOfChaos added 11 commits May 1, 2026 09:47

Fix inexhaustible granite mine production

ea2ee5f

Simplify granite mine resource handling

b887aaf

Tighten granite mine production tests

4e4b9d1

Clarify inexhaustible granite mines addon description

b636b5d

Avoid runtime addon toggle in granite mine test

2a4936e

Split granite mine work-everywhere addon

fb40fc9

Implement configurable mine resource behavior

44bbab0

Implement mine no-output fallback behavior

64a8b71

Teach AI mine planning configurable resource behavior

621d52e

Adjust mine productivity display for S4-like behavior

d1df485

Merge branch 'master' into sidequest/configurable-mine-resource-behavior

1733b37

Flamefire requested changes May 22, 2026

View reviewed changes

Merge branch 'master' into sidequest/configurable-mine-resource-behavior

b9be39f

Rework configurable mine resource behavior

bf24106

Clarify S4-like mine productivity reference

86587eb

Flamefire requested changes Jun 7, 2026

View reviewed changes

	// TODO: INEXHAUSTIBLE_MINES is kept only for loading old settings/savegames until the gamedata version can be raised.
	// TODO(Replay) TODO(Savegame): INEXHAUSTIBLE_MINES is kept only for loading old settings/savegames until the gamedata version can be raised.

	enum class MineNoOutputFallback : unsigned
	enum class MineNoOutputFallback

Conversation

DevOpsOfChaos commented May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Details

S4-like behavior

Work-everywhere behavior

Validation

Uh oh!

Flamefire left a comment

Choose a reason for hiding this comment

Uh oh!

DevOpsOfChaos commented May 26, 2026

Uh oh!

Flamefire commented May 26, 2026

Uh oh!

DevOpsOfChaos commented May 26, 2026

Uh oh!

Flamefire commented May 27, 2026

Uh oh!

DevOpsOfChaos commented Jun 3, 2026

Uh oh!

Flamefire left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

DevOpsOfChaos commented May 21, 2026 •

edited

Loading