Skip to content

Spike/addons/settlers iv mining #1501

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 15 commits into
base: master
Choose a base branch
from
Draft

Spike/addons/settlers iv mining #1501

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
a778a07
AI: do no longer treat inexhaustible granite mines like it was intend…
Spikeone Feb 19, 2022
088db97
Addons: Add options for each mine type for changing the behavior
Spikeone Feb 19, 2022
174c99b
use one base class for mining overhaul addons
Spikeone Feb 21, 2022
33bd9ab
use enum
Spikeone Feb 21, 2022
3c20586
fixup local build failing due to includes, remove workplace changes
Spikeone Feb 21, 2022
40a9160
rework coding for settlers iv like mining, remove some magic numbers
Spikeone Feb 21, 2022
4e5a182
add new line in mapbase.cpp
Spikeone Feb 28, 2022
65f7e39
rename numPointsRadius to GetNumPointsInRadius to match other naming,…
Spikeone Feb 28, 2022
a25a950
rename new consts
Spikeone Feb 28, 2022
833f17a
remove unused include
Spikeone Feb 28, 2022
bf302f2
move include
Spikeone Feb 28, 2022
df850e3
improve main implementation
Spikeone Feb 28, 2022
8d58bd5
improve addon description
Spikeone Feb 28, 2022
b44986b
add saving and loading of isAlteredWorkcycle
Spikeone Feb 28, 2022
ae9e40f
ctrl + k, ctrl + d for all files that failed formatting tests
Spikeone Feb 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions libs/s25main/addons/AddonMiningOverhaul.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,14 @@
#include "AddonList.h"
#include "mygettext/mygettext.h"

enum class MiningBehavior
{
Normal,
S4Like,
Inexhaustible,
AlwaysAvailable
};

class AddonMiningOverhaulBase : public AddonList
Copy link
Member

@Flamefire Flamefire Feb 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@Spikeone Spikeone Feb 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Member

@Flamefire Flamefire Feb 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Contributor

@falbrechtskirchinger falbrechtskirchinger Jul 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
protected:
Expand Down
39 changes: 17 additions & 22 deletions libs/s25main/figures/nofMiner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "ogl/glArchivItem_Bitmap_Player.h"
#include "world/GameWorld.h"
#include <random/Random.h>
#include <GlobalGameSettings.cpp>

nofMiner::nofMiner(const MapPoint pos, const unsigned char player, nobUsual* workplace)
: nofWorkman(Job::Miner, pos, player, workplace)
Expand Down Expand Up @@ -72,41 +73,38 @@ helpers::OptionalEnum<GoodType> nofMiner::ProduceWare()

bool nofMiner::AreWaresAvailable() const
{
// FindPointWithResource triggeres outofresource message
if(GetAddonSetting() == 3)
if(!nofWorkman::AreWaresAvailable())
return false;

if(GetMiningBehavior() == MiningBehavior::AlwaysAvailable)
return true;

return nofWorkman::AreWaresAvailable() && FindPointWithResource(GetRequiredResType()).isValid();
return FindPointWithResource(GetRequiredResType()).isValid();
}

unsigned int nofMiner::GetAddonSetting() const
MiningBehavior nofMiner::GetMiningBehavior() const
{
const GlobalGameSettings& settings = world->GetGGS();

switch(workplace->GetBuildingType())
{
case BuildingType::GoldMine: return settings.getSelection(AddonId::MINING_OVERHAUL_GOLD);
case BuildingType::IronMine: return settings.getSelection(AddonId::MINING_OVERHAUL_IRON);
case BuildingType::CoalMine: return settings.getSelection(AddonId::MINING_OVERHAUL_COAL);
case BuildingType::GraniteMine: return settings.getSelection(AddonId::MINING_OVERHAUL_GRANITE);
default: return 0;
case BuildingType::GoldMine: return (MiningBehavior)settings.getSelection(AddonId::MINING_OVERHAUL_GOLD);
case BuildingType::IronMine: return (MiningBehavior)settings.getSelection(AddonId::MINING_OVERHAUL_IRON);
case BuildingType::CoalMine: return (MiningBehavior)settings.getSelection(AddonId::MINING_OVERHAUL_COAL);
case BuildingType::GraniteMine: return (MiningBehavior)settings.getSelection(AddonId::MINING_OVERHAUL_GRANITE);
default: return MiningBehavior::Normal;
}
}

bool nofMiner::StartWorking()
{
workplace->is_emptyCycle = false;
Copy link
Member

@Flamefire Flamefire Feb 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this a property of the workplace? Shouldn't this be part of the miner? You can use this in ProduceWare to return boost::none aka "nothing"


// needs to have at least one resource spot
// 0 = is exhaustible
// 1 = settlers IV like
// 2 = inexhaustible
// 3 = everywhere
unsigned int addonSettings = GetAddonSetting();
MiningBehavior addonSettings = GetMiningBehavior();

switch (addonSettings)
{
case 1: // settlers IV style
case MiningBehavior::S4Like:
{
int sumResAmount = 0;
MapPoint useResPt;
Expand Down Expand Up @@ -144,18 +142,15 @@ bool nofMiner::StartWorking()
}
}
break;
case 2: // inexhaustible
case MiningBehavior::Inexhaustible:
{
MapPoint resPt = FindPointWithResource(GetRequiredResType());
if(!resPt.isValid())
return false;
}
break;
case 3: // inexhaustible, everywhere
{
}
break;
case 0: // original behavior
case MiningBehavior::AlwaysAvailable: break;
case MiningBehavior::Normal:
default:
{
MapPoint resPt = FindPointWithResource(GetRequiredResType());
Copy link
Member

@Flamefire Flamefire Feb 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Expand Down
2 changes: 1 addition & 1 deletion libs/s25main/figures/nofMiner.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ class nofMiner : public nofWorkman
bool AreWaresAvailable() const override;
bool StartWorking() override;
ResourceType GetRequiredResType() const;
unsigned int GetAddonSetting() const;
MiningBehavior GetMiningBehavior() const;

public:
nofMiner(MapPoint pos, unsigned char player, nobUsual* workplace);
Expand Down