Spike/addons/settlers iv mining #1501

Spikeone · 2022-02-19T15:50:33Z

@Flamefire @Flow86 - als draft bis die Punkte die euch stören behoben sind, danach entferne ich dann das inexhaustibleGraniteMines und InexhaustibleMines addon da beide nicht mehr notwendig sind.

…ed (inexhaustible = everywhere)

Flamefire

General: Good idea I'd say. Although I'm not sure it is worth (i.e. will it be used?) having 4 addons instead of 1-2 (all mines or granite and others). Also I'm not really a fan of the "Everywhere" setting. This makes geologists worthless and IMO changes the game to much... Do people really want this? Personal opinion though and it is an addon so things like this are possible.

As for the implementation I added some comments. Some (very) specific some more general, so don't panic ;-)

General advise (kind of a TLDR):

If you C&P something think again about alternatives it often becomes an issue later on
If you need comments think again. Here: Mostly the "magic numbers" which can be replaced by enums, named constants, functions...
const on anything which doesn't need to be changed
No need for a variable if you simply return it, return it directly.

These are rules of thumb, so conditions apply ;-)

libs/s25main/figures/nofMiner.h

libs/s25main/figures/nofMiner.cpp

Flamefire · 2022-02-19T17:21:50Z

libs/s25main/figures/nofMiner.cpp

+        case 0: // original behavior
+        default:
+        {
+            MapPoint resPt = FindPointWithResource(GetRequiredResType());


Suggested change

MapPoint resPt = FindPointWithResource(GetRequiredResType());

const MapPoint resPt = FindPointWithResource(GetRequiredResType());

libs/s25main/figures/nofWorkman.cpp

libs/s25main/GlobalGameSettings.cpp

Spikeone · 2022-02-19T20:15:16Z

Well, while I was thinking about the "InexhaustibleGraniteMine" addon which had a different idea initially than it currently implements, I felt like people might use a mix of the settings, or at least they made sense for me:

Granit Mines => Everywhere => Enough building material for longer games (maps often do not provide enough with low/very low goods)
Coal/Iron => Settlers IV like => There is always some production left and the game never ends in a lock
Gold => Original => Still reason to go for the mountains containing gold

At least thats the setting I'm currently thinking about as default for myself. On the other hand I agree, that having 4 addons (which I need to find a good name for, so they are next to each other) isn't that good either. But as I really think the "Granite Mines Everywhere" Addon is a good idea, I'd go with 2 anyway - and then 4 isn't that much more. Mhh well

Flamefire · 2022-02-20T11:53:26Z

At least thats the setting I'm currently thinking about as default for myself. On the other hand I agree, that having 4 addons (which I need to find a good name for, so they are next to each other) isn't that good either. But as I really think the "Granite Mines Everywhere" Addon is a good idea, I'd go with 2 anyway - and then 4 isn't that much more.

Agreed.
As for the sorting: Sorting is done by the name. So instead of Change coal mine behavior I'd suggest: "Mine behavior: Coal, i.e. especially get rid of the word "Change" because "Change coal mine behavior: Default" doesn't make sense ;-) So the shorter version even reads better.
Instead of "Mine" one could use "Mining", not sure what is better. E.g. "Mining behavior (Coal)" is also possible.

Spikeone · 2022-02-21T10:29:33Z

@Flamefire hope I implemented your suggestions correctly. I have 3 ToDos left, 2 of them are simply for skipping checks which are possibly redundant (in some cases). But saving the current workcycle state is required, so the savegame version increases - and I have to find out on how to correctly push and pop the workcycle

Flamefire

I have 3 ToDos left, 2 of them are simply for skipping checks which are possibly redundant (in some cases). But saving the current workcycle state is required, so the savegame version increases

I don't think it is really possible to skip the checks, or worth it... IMO ok like it is.
As for saving/loading: You have a FIFO buffer here, i.e. what goes in first, comes out first.

As the base class needs to be constructed first, it means it needs to be saved first. I.e. your miner-save function starts with calling the workman save function. Then you push your own states: sgd.pushBool(produceNothing) (IIRC)

The loading is then the other way round: First the base class, done by : nofWorkman(sgd, obj_id) in the constructor call. Then you pop your own states: , produceNothing(sgd.popBool()) (instead of the , isAlteredWorkcycle(false) in the other constructor)

However as we add stuff here we could keep compatibility with the old version: If the sgd version is smaller than the new version, init the field with false (we didn't have empty cycles before), else pop the value. This way you can load old and new save games :) Search for "sgd.GetGameDataVersion()" to see some examples.

Flamefire · 2022-02-21T15:36:48Z

libs/s25main/addons/AddonMiningOverhaul.h

+#include "AddonList.h"
+#include "mygettext/mygettext.h"
+
+class AddonMiningOverhaulBase : public AddonList


Why this lengthy name (here and in the name)? How would you translate it (to German)?

I mean:

Name: "Mining overhaul: Coal"

Description: "This addon lets you control mining behavior"

This is why I suggested "Mining behavior" thinking about how it will look in the (already long) list of addons with their currently active setting in the window: "Mining behavior (coal): No change" vs "Mining overhaul: Coal: No Change"

PS: Funny how the roles are reversed for once: You thinking like a developer, me like a user :D

Why this lengthy name (here and in the name)? How would you translate it (to German)?

AddonZumAnpassenDerBergwerklogik :D

I'm not so happy with the behavior anymore. This somewhat more feels like something visible, possibly a worker. Like how does a soldier behave when his home building is lost, does he return, does he wander around. Overhaul implied some change of logic, which is often invisible and just like "physics", you feel the result but can't grab the reason itself - it's just the way it is. Can change the name if you don't agree to that though :)

"Mining behavior (coal): No change" vs "Mining overhaul: Coal: No Change"

I don't see where this Representation comes from, as there is no textual representation that way as values are highlighted by the right column. So I rather see Mining behavior (coal) vs Mining overhaul: coal where I liked the second version more, besides the discussion if it should be overhaul or behavior. Anyway, I don't care that much for the name in the window (except lenghty names like "Number of scouts required fore exploration expedition"). So same here, can change the name if you'd like me to :)

I don't see where this Representation comes from, as there is no textual representation that way

I mean the Addon settings window. Currently it really reads (to me) like Addon title: Current setting. E.g. "Economy mode game length: 120min" and I kinda like that. I.e. a list that reads like plain English for every row so you understand it easily even when not reading the hint.

AddonZumAnpassenDerBergwerklogik :D

Ouch :D Seriously, I have that settings window in mind and try to imagine how it would look there.

So I rather see Mining behavior (coal) vs Mining overhaul: coal where I liked the second version more, besides the discussion if it should be overhaul or behavior.

I'd really like "behavior" better for a setting. Yes you are right with the description of "overhaul" but (at least to me) "overhaul" is the process of changing the behavior. I.e. reimplementing a part of the game to be "better" (performance, crash-resistance, ...). E.g. we need an overhaul of the save/load system as the current one sucks and crashes on big maps ;-)

As for the setting: This is something you change when selecting an option. And that option doesn't "overhaul" something but changes its "behavior".

Maybe we haven't found a good name yet. I mean what it changes is the behavior of the mines regarding resources. So maybe "Resource usage by mines: Coal". But that still doesn't fully hit it. It misses that the miner may produce nothing for a cycle. That's why I was asking for the German name. Just another way to think about it to come up with a nice flashy name for this :)

So I'd say come up with an English and German name here, maybe ask in Discord if anyone else has ideas, and check how it looks like in the Addon window before we finalize the name. So it is at least as good as possible in English and German ;-)

My current favourite is still "behavior", similar to what we have for the toolmaker: "Behavior [...]: ProduceNothing | ProduceRandom"

If I may add my 2 cents:

First, on "mine" vs "mining":

"Mining overhaul", as the act of mining is being overhauled.

"Mine behavior", as behavior applies to the exhaustion of the mine.

Personally, I'd use "Mine exhaustion (Coal)"/"Minenerschöpfung (Kohle)". Also, note the parentheses. Otherwise, you'd end up with an awkward double-colon when writing out settings: "Mine exhaustion: Coal: Default"

An alternative might be "Mine productivity"/"Minenergiebigkeit". I don't like it but maybe it sparks someone else's idea.

libs/s25main/figures/nofMiner.cpp

libs/s25main/figures/nofWorkman.cpp

libs/s25main/figures/nofWorkman.h

libs/s25main/gameData/GameConsts.h

libs/s25main/world/MapBase.cpp

… make it constexpr

Flamefire

Some more comments.

BTW: Do you use Visual Studio?
Download this static-build of clangformat Just put this anywhere in your PATH, or into a new folder which you add to your PATH environment variable. Then rerun CMake which will find this (or add it manually under ClangFormat_BINARY)
This will add another target which you can rightclick->build/erzeugen and that will reformat all changed(!) files (on first time, all files as it doesn't know which were changed)

Flamefire · 2022-02-28T10:21:21Z

libs/s25main/figures/nofMiner.cpp

@@ -138,7 +138,7 @@ bool nofMiner::StartWorking()
            }

            // depending on remaining resources, roll if this workcycle needs to be altered or not
-            if(RANDOM_RAND(world->numPointsRadius(MINER_RADIUS_SETTLERSIV, true) * MINER_MAX_QUANTITY)
+            if(RANDOM_RAND(world->GetNumPointsInRadius(MINER_RADIUS_SETTLERSIV, true) * MINER_MAX_QUANTITY)


TBH: I actually want to push it to the other direction: Types (classes, structs, ...) start with Uppercase, the rest with lowercase. But well, for consistency right now it makes sense

Flamefire · 2022-02-28T10:42:57Z

libs/s25main/addons/AddonMiningOverhaul.h

+#include "AddonList.h"
+#include "mygettext/mygettext.h"
+
+class AddonMiningOverhaulBase : public AddonList


I don't see where this Representation comes from, as there is no textual representation that way

I mean the Addon settings window. Currently it really reads (to me) like Addon title: Current setting. E.g. "Economy mode game length: 120min" and I kinda like that. I.e. a list that reads like plain English for every row so you understand it easily even when not reading the hint.

AddonZumAnpassenDerBergwerklogik :D

Ouch :D Seriously, I have that settings window in mind and try to imagine how it would look there.

So I rather see Mining behavior (coal) vs Mining overhaul: coal where I liked the second version more, besides the discussion if it should be overhaul or behavior.

I'd really like "behavior" better for a setting. Yes you are right with the description of "overhaul" but (at least to me) "overhaul" is the process of changing the behavior. I.e. reimplementing a part of the game to be "better" (performance, crash-resistance, ...). E.g. we need an overhaul of the save/load system as the current one sucks and crashes on big maps ;-)

As for the setting: This is something you change when selecting an option. And that option doesn't "overhaul" something but changes its "behavior".

Maybe we haven't found a good name yet. I mean what it changes is the behavior of the mines regarding resources. So maybe "Resource usage by mines: Coal". But that still doesn't fully hit it. It misses that the miner may produce nothing for a cycle. That's why I was asking for the German name. Just another way to think about it to come up with a nice flashy name for this :)

So I'd say come up with an English and German name here, maybe ask in Discord if anyone else has ideas, and check how it looks like in the Addon window before we finalize the name. So it is at least as good as possible in English and German ;-)

My current favourite is still "behavior", similar to what we have for the toolmaker: "Behavior [...]: ProduceNothing | ProduceRandom"

Flamefire · 2022-02-28T10:43:47Z

libs/s25main/figures/nofMiner.cpp

-    if(!inexhaustibleRes)
-        world->ReduceResource(resPt);
+
+    auto miningBehavior = MiningBehavior::Normal;


Either use the default switch or a default value. Using both doesn't make sense... I'd say the default case doesn't make sense anway: It should be unreachable...
BTW: With the current implementation of this function you are better off with return static_cast<...>(...) instead of all this C&P miningBehavior = static_cast...; break because the return makes the break superflous so it can be removed.

I guess this comes from a variation of my suggestion: That was to use the switch only to "translate" the building type to the correct AddonId, which I still think is a good idea, as it makes the code shorter which is usually good ;)

Had to add the default statement as I otherwise get warned that for any other building types no value is returned and that warning is treated as error. But when using return values thats needed anways.

Flamefire · 2022-02-28T13:26:18Z

libs/s25main/figures/nofMiner.cpp

+#include <random/Random.h>
+#include <gameData/GameConsts.h>


Suggested change

#include <random/Random.h>

#include <gameData/GameConsts.h>

#include "random/Random.h"

#include "gameData/GameConsts.h"

I'm not sure we already have a convention yet. But basically: Everything from us in quotes, external (e.g. Boost) and stdlib stuff in angle brackets is what we use the most.

Flamefire · 2022-02-28T13:39:36Z

libs/s25main/figures/nofMiner.h

+    /// alter workcycle (addon)
+    bool isAlteredWorkcycle;


While it is very good to write code with future extensions in mind, I don't think it applies here. If we never implement that new addon (cool idea, though!) everyone will always be confused by this. And: We might want an enum here later, like: produceNormal, produceNothing, produceSpecial. And it is a private class variable, so with very limited scope where renaming later on will not hurt.

So here, in this case we likely gain more by less speculation and giving it a clear name for the current use, and adapt when and if required.

Flamefire · 2022-02-28T13:42:14Z

libs/s25main/gameData/GameConsts.h

@@ -20,6 +20,10 @@ constexpr GameSpeed referenceSpeed = GameSpeed::Normal;

 /// Reichweite der Bergarbeiter
 constexpr unsigned MINER_RADIUS = 2;
+constexpr unsigned MINER_ORE_RADIUS_SETTLERSIV = 1u;


Nit: I Always read this as "settler siv" and am confused :D Maybe add another underscore or just use "4"? Similar to "MiningBehavior::S4Like"

From the other comment:

I've never seen a well as a mine - which makes the name MINER_RADIUS_SETTLERSIV also wrong.

This is actually a (code quality) bug, that nofWorkman::FindPointWithResource is used by the wellguy and miners and has MINER_RADIUS hard-coded. So your original name here was correct and you can remove the _ORE infix.
If you want, you can fix this bug by removing nofWorkman::FindPointWithResource(ResourceType type) (i.e. the one without the radius as the parameter) and keep only the one with the radius which you added. Then either change the call sites to always pass the correct radius constant (add a new WELL_RADIUS or so here) Optionally you can move the old FindPointWithResource (without radius) into the nofMiner and nofWellGuy classes to avoid repeating the radius constant in multiple places of the class and even remove the ResourceType from the signature, as e.g. for the wellguy it is always water, and for the miner you can use GetRequiredResType. This makes a nice default function: MapPoint FindPointWithResource() const while for special cases like your addon there is still the function taking 2 arguments in the base class. Finally: For the wellguy you can even name that new, parameterless function FindPointWithWater or similar to reflect its implementation

Flamefire · 2022-02-28T13:53:03Z

libs/s25main/figures/nofMiner.cpp

+        case MiningBehavior::S4Like:
+        {
+            int sumResAmount = 0;
+            MapPoint nonMinimumResPt;


I think this variable is worth a comment, despite its name. Something like: Resources never deplete, only get reduced to 1. Hence find any point with at least 2 of the required resource and reduce that.

Flamefire · 2022-02-28T14:07:54Z

libs/s25main/figures/nofMiner.cpp

+        {
+            if(!FindPointWithResource(GetRequiredResType()).isValid())
+                return false;
+        }


I think this can never be false, as this is checked in the has wares function.

Suggested change

{

if(!FindPointWithResource(GetRequiredResType()).isValid())

return false;

}

// Already checked in AreWaresAvailable

RTTR_Assert(FindPointWithResource(GetRequiredResType()).isValid())

Notes:

Braces in switch-case only required when introducing a variable, otherwise you can omit them to save space ;-)

RTTR_Assert "asserts" (i.e. assumes) something which should be true at this point. In release builds this check is removed(!), in debug builds (or in non-default release builds, there is a CMake-option) the condition is checked and the game aborts if it doesn't hold true. So only use for something which really must be true at this point and if not obvious add a comment why.

Spikeone · 2023-07-17T06:24:12Z

Currently (as far as I remember) the following is missing:
AI:

should honor possible settings (i.e. if it's allowed to mine gold everywhere, the AI should do that instead of searching)
should honor combinations (i.e. granite mines always but all other mines just as they are)
should somewhat honor the current accumulated productivity of mines (i.e. 10 coal mines at 10% are just as good as 1 coal mine at 100%)

Alternative production cycle product:
This one alters whats produced when an alternate production cycle runs. The addon should have the following settings:

produce nothing (default)
produce granite (25%)
produce granite (50%)
produce granite (100%)
produce lower grade (e.g. gold mine produces iron, iron produce coal, coal produce granite, and granite just nothing)
Although the last setting doesn't make sense logical, it does make sense for the gameplay.
This addon is compatible to other addon settings, since it is only taken into account when setting the alternate cycle resource

Show productivity:
This one shows a productivity rate for mines. If a mine type is inexhaustible, 100% is shown anyway. Maybe some color could indicate if this mine is already at it's lowest rate or not (e.g. >50% = "good green", >min% = "okay yellow", == min% = "bad red")
settings:

disable (default)
enable
This addon is compatible to other settings, since it should show 100% if original mining behavior is enabled or inexhaustible.

FYI @falbrechtskirchinger

Spikeone added 2 commits February 19, 2022 12:54

AI: do no longer treat inexhaustible granite mines like it was intend…

a778a07

…ed (inexhaustible = everywhere)

Addons: Add options for each mine type for changing the behavior

088db97

Spikeone requested review from Flamefire and Flow86 February 19, 2022 15:50

Flamefire requested changes Feb 19, 2022

View reviewed changes

Spikeone added 4 commits February 21, 2022 09:46

use one base class for mining overhaul addons

174c99b

use enum

33bd9ab

fixup local build failing due to includes, remove workplace changes

3c20586

rework coding for settlers iv like mining, remove some magic numbers

40a9160

Flamefire requested changes Feb 21, 2022

View reviewed changes

Spikeone added 9 commits February 28, 2022 09:45

add new line in mapbase.cpp

4e5a182

rename numPointsRadius to GetNumPointsInRadius to match other naming,…

65f7e39

… make it constexpr

rename new consts

a25a950

remove unused include

833f17a

move include

bf302f2

improve main implementation

df850e3

improve addon description

8d58bd5

add saving and loading of isAlteredWorkcycle

b44986b

ctrl + k, ctrl + d for all files that failed formatting tests

ae9e40f

Flamefire requested changes Feb 28, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spike/addons/settlers iv mining #1501

Spike/addons/settlers iv mining #1501

Spikeone commented Feb 19, 2022

Flamefire left a comment

Flamefire Feb 19, 2022

Spikeone commented Feb 19, 2022

Flamefire commented Feb 20, 2022

Spikeone commented Feb 21, 2022

Flamefire left a comment

Flamefire Feb 21, 2022

Spikeone Feb 28, 2022

Flamefire Feb 28, 2022

falbrechtskirchinger Jul 7, 2023

Flamefire left a comment

Flamefire Feb 28, 2022

Flamefire Feb 28, 2022

Flamefire Feb 28, 2022

Spikeone Feb 28, 2022

Flamefire Feb 28, 2022

Flamefire Feb 28, 2022

Flamefire Feb 28, 2022

Flamefire Feb 28, 2022

Flamefire Feb 28, 2022

Spikeone commented Jul 17, 2023 •

edited

Loading

	MapPoint resPt = FindPointWithResource(GetRequiredResType());
	const MapPoint resPt = FindPointWithResource(GetRequiredResType());

Spike/addons/settlers iv mining #1501

Are you sure you want to change the base?

Spike/addons/settlers iv mining #1501

Conversation

Spikeone commented Feb 19, 2022

Flamefire left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Spikeone commented Feb 19, 2022

Flamefire commented Feb 20, 2022

Spikeone commented Feb 21, 2022

Flamefire left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Flamefire left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Spikeone commented Jul 17, 2023 • edited Loading

Spikeone commented Jul 17, 2023 •

edited

Loading