-
Notifications
You must be signed in to change notification settings - Fork 79
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.
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
Spike/addons/settlers iv mining #1501
base: master
Are you sure you want to change the base?
Changes from all commits
a778a07
088db97
174c99b
33bd9ab
3c20586
40a9160
4e5a182
65f7e39
a25a950
833f17a
bf302f2
df850e3
8d58bd5
b44986b
ae9e40f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,62 @@ | ||
// Copyright (C) 2005 - 2021 Settlers Freaks (sf-team at siedler25.org) | ||
// | ||
// SPDX-License-Identifier: GPL-2.0-or-later | ||
|
||
#pragma once | ||
|
||
#include "AddonList.h" | ||
#include "mygettext/mygettext.h" | ||
|
||
enum class MiningBehavior | ||
{ | ||
Normal, | ||
S4Like, | ||
Inexhaustible, | ||
AlwaysAvailable | ||
}; | ||
|
||
class AddonMiningOverhaulBase : public AddonList | ||
{ | ||
protected: | ||
AddonMiningOverhaulBase(AddonId addonId, const std::string& addonName) | ||
: AddonList(addonId, AddonGroup::Economy, addonName, | ||
_("This addon allows you to change the ore mining behavior.\n\n" | ||
"No change: Original behavior\n" | ||
"Settlers IV: Mines never fully deplete. Range is decreased to 1. Chance for production" | ||
" depends on remaining resource amount in range.\n" | ||
"Inexhaustible: Mines never deplete\n" | ||
"Everywhere: Mines never deplete, can mine everywhere"), | ||
{ | ||
_("No change"), | ||
_("Settlers IV"), | ||
_("Inexhaustible"), | ||
_("Everywhere"), | ||
}) | ||
{} | ||
}; | ||
|
||
class AddonMiningOverhaulCoal : public AddonMiningOverhaulBase | ||
{ | ||
public: | ||
AddonMiningOverhaulCoal() : AddonMiningOverhaulBase(AddonId::MINING_OVERHAUL_COAL, _("Mining overhaul: Coal")) {} | ||
}; | ||
|
||
class AddonMiningOverhaulGold : public AddonMiningOverhaulBase | ||
{ | ||
public: | ||
AddonMiningOverhaulGold() : AddonMiningOverhaulBase(AddonId::MINING_OVERHAUL_GOLD, _("Mining overhaul: Gold")) {} | ||
}; | ||
|
||
class AddonMiningOverhaulGranite : public AddonMiningOverhaulBase | ||
{ | ||
public: | ||
AddonMiningOverhaulGranite() | ||
: AddonMiningOverhaulBase(AddonId::MINING_OVERHAUL_GRANITE, _("Mining overhaul: Granite")) | ||
{} | ||
}; | ||
|
||
class AddonMiningOverhaulIron : public AddonMiningOverhaulBase | ||
{ | ||
public: | ||
AddonMiningOverhaulIron() : AddonMiningOverhaulBase(AddonId::MINING_OVERHAUL_IRON, _("Mining overhaul: Iron")) {} | ||
}; |
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -5,18 +5,27 @@ | |||||||||||||
#include "nofMiner.h" | ||||||||||||||
#include "GlobalGameSettings.h" | ||||||||||||||
#include "Loader.h" | ||||||||||||||
#include "SerializedGameData.h" | ||||||||||||||
#include "SoundManager.h" | ||||||||||||||
#include "addons/const_addons.h" | ||||||||||||||
#include "buildings/nobUsual.h" | ||||||||||||||
#include "network/GameClient.h" | ||||||||||||||
#include "ogl/glArchivItem_Bitmap_Player.h" | ||||||||||||||
#include "world/GameWorld.h" | ||||||||||||||
#include <gameData/GameConsts.h> | ||||||||||||||
#include <random/Random.h> | ||||||||||||||
|
||||||||||||||
nofMiner::nofMiner(const MapPoint pos, const unsigned char player, nobUsual* workplace) | ||||||||||||||
: nofWorkman(Job::Miner, pos, player, workplace) | ||||||||||||||
: nofWorkman(Job::Miner, pos, player, workplace), isAlteredWorkcycle(false) | ||||||||||||||
{} | ||||||||||||||
|
||||||||||||||
nofMiner::nofMiner(SerializedGameData& sgd, const unsigned obj_id) : nofWorkman(sgd, obj_id) {} | ||||||||||||||
nofMiner::nofMiner(SerializedGameData& sgd, const unsigned obj_id) : nofWorkman(sgd, obj_id) | ||||||||||||||
{ | ||||||||||||||
if(sgd.GetGameDataVersion() >= 10) | ||||||||||||||
isAlteredWorkcycle = sgd.PopBool(); | ||||||||||||||
else | ||||||||||||||
isAlteredWorkcycle = false; | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
void nofMiner::DrawWorking(DrawPoint drawPt) | ||||||||||||||
{ | ||||||||||||||
|
@@ -60,6 +69,9 @@ unsigned short nofMiner::GetCarryID() const | |||||||||||||
|
||||||||||||||
helpers::OptionalEnum<GoodType> nofMiner::ProduceWare() | ||||||||||||||
{ | ||||||||||||||
if(isAlteredWorkcycle) | ||||||||||||||
return boost::none; | ||||||||||||||
|
||||||||||||||
switch(workplace->GetBuildingType()) | ||||||||||||||
{ | ||||||||||||||
case BuildingType::GoldMine: return GoodType::Gold; | ||||||||||||||
|
@@ -71,20 +83,103 @@ helpers::OptionalEnum<GoodType> nofMiner::ProduceWare() | |||||||||||||
|
||||||||||||||
bool nofMiner::AreWaresAvailable() const | ||||||||||||||
{ | ||||||||||||||
return nofWorkman::AreWaresAvailable() && FindPointWithResource(GetRequiredResType()).isValid(); | ||||||||||||||
if(!nofWorkman::AreWaresAvailable()) | ||||||||||||||
return false; | ||||||||||||||
|
||||||||||||||
const MiningBehavior addonSetting = GetMiningBehavior(); | ||||||||||||||
|
||||||||||||||
if(addonSetting == MiningBehavior::AlwaysAvailable) | ||||||||||||||
return true; | ||||||||||||||
|
||||||||||||||
if(addonSetting == MiningBehavior::S4Like) | ||||||||||||||
return FindPointWithResource(GetRequiredResType(), MINER_ORE_RADIUS_SETTLERSIV).isValid(); | ||||||||||||||
else | ||||||||||||||
return FindPointWithResource(GetRequiredResType()).isValid(); | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
bool nofMiner::StartWorking() | ||||||||||||||
MiningBehavior nofMiner::GetMiningBehavior() const | ||||||||||||||
{ | ||||||||||||||
MapPoint resPt = FindPointWithResource(GetRequiredResType()); | ||||||||||||||
if(!resPt.isValid()) | ||||||||||||||
return false; | ||||||||||||||
const GlobalGameSettings& settings = world->GetGGS(); | ||||||||||||||
bool inexhaustibleRes = settings.isEnabled(AddonId::INEXHAUSTIBLE_MINES) | ||||||||||||||
|| (workplace->GetBuildingType() == BuildingType::GraniteMine | ||||||||||||||
&& settings.isEnabled(AddonId::INEXHAUSTIBLE_GRANITEMINES)); | ||||||||||||||
if(!inexhaustibleRes) | ||||||||||||||
world->ReduceResource(resPt); | ||||||||||||||
|
||||||||||||||
auto miningBehavior = MiningBehavior::Normal; | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Either use the 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 ;) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||||||||||
|
||||||||||||||
switch(workplace->GetBuildingType()) | ||||||||||||||
{ | ||||||||||||||
case BuildingType::GoldMine: | ||||||||||||||
miningBehavior = static_cast<MiningBehavior>(settings.getSelection(AddonId::MINING_OVERHAUL_GOLD)); | ||||||||||||||
break; | ||||||||||||||
case BuildingType::IronMine: | ||||||||||||||
miningBehavior = static_cast<MiningBehavior>(settings.getSelection(AddonId::MINING_OVERHAUL_IRON)); | ||||||||||||||
break; | ||||||||||||||
case BuildingType::CoalMine: | ||||||||||||||
miningBehavior = static_cast<MiningBehavior>(settings.getSelection(AddonId::MINING_OVERHAUL_COAL)); | ||||||||||||||
break; | ||||||||||||||
case BuildingType::GraniteMine: | ||||||||||||||
miningBehavior = static_cast<MiningBehavior>(settings.getSelection(AddonId::MINING_OVERHAUL_GRANITE)); | ||||||||||||||
break; | ||||||||||||||
default: miningBehavior = MiningBehavior::Normal; | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
return miningBehavior; | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
bool nofMiner::StartWorking() | ||||||||||||||
{ | ||||||||||||||
isAlteredWorkcycle = false; | ||||||||||||||
|
||||||||||||||
switch(GetMiningBehavior()) | ||||||||||||||
{ | ||||||||||||||
case MiningBehavior::S4Like: | ||||||||||||||
{ | ||||||||||||||
int sumResAmount = 0; | ||||||||||||||
MapPoint nonMinimumResPt; | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||||||||||
|
||||||||||||||
const std::vector<MapPoint> reachablePts = | ||||||||||||||
world->GetPointsInRadiusWithCenter(workplace->GetPos(), MINER_ORE_RADIUS_SETTLERSIV); | ||||||||||||||
|
||||||||||||||
for(const MapPoint curPt : reachablePts) | ||||||||||||||
{ | ||||||||||||||
const Resource resource = world->GetNode(curPt).resources; | ||||||||||||||
|
||||||||||||||
if(resource.getType() != GetRequiredResType()) | ||||||||||||||
continue; | ||||||||||||||
|
||||||||||||||
const auto resAmount = resource.getAmount(); | ||||||||||||||
sumResAmount += resAmount; | ||||||||||||||
|
||||||||||||||
if(resAmount > 1u && !nonMinimumResPt.isValid()) | ||||||||||||||
nonMinimumResPt = curPt; | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
// depending on remaining resources, roll if this workcycle needs to be altered or not | ||||||||||||||
if(RANDOM_RAND(reachablePts.size() * MAX_ORE_QUANTITY) > sumResAmount) | ||||||||||||||
{ | ||||||||||||||
isAlteredWorkcycle = true; | ||||||||||||||
} else | ||||||||||||||
{ | ||||||||||||||
if(nonMinimumResPt.isValid()) | ||||||||||||||
world->ReduceResource(nonMinimumResPt); | ||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
break; | ||||||||||||||
case MiningBehavior::Inexhaustible: | ||||||||||||||
{ | ||||||||||||||
if(!FindPointWithResource(GetRequiredResType()).isValid()) | ||||||||||||||
return false; | ||||||||||||||
} | ||||||||||||||
Comment on lines
+166
to
+169
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this can never be false, as this is checked in the has wares function.
Suggested change
Notes:
|
||||||||||||||
break; | ||||||||||||||
case MiningBehavior::AlwaysAvailable: break; | ||||||||||||||
case MiningBehavior::Normal: | ||||||||||||||
{ | ||||||||||||||
const MapPoint resPt = FindPointWithResource(GetRequiredResType()); | ||||||||||||||
if(!resPt.isValid()) | ||||||||||||||
return false; | ||||||||||||||
|
||||||||||||||
world->ReduceResource(resPt); | ||||||||||||||
} | ||||||||||||||
break; | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
return nofWorkman::StartWorking(); | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
|
@@ -98,3 +193,12 @@ ResourceType nofMiner::GetRequiredResType() const | |||||||||||||
default: return ResourceType::Granite; | ||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
void nofMiner::Serialize(SerializedGameData& sgd) const | ||||||||||||||
{ | ||||||||||||||
nofWorkman::Serialize(sgd); | ||||||||||||||
|
||||||||||||||
sgd.PushBool(isAlteredWorkcycle); | ||||||||||||||
|
||||||||||||||
sgd.GetGameDataVersion(); | ||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,11 +5,11 @@ | |
#pragma once | ||
|
||
#include "nofWorkman.h" | ||
#include "addons/AddonMiningOverhaul.h" | ||
|
||
class SerializedGameData; | ||
class nobUsual; | ||
|
||
/// Klasse für den Schreiner | ||
class nofMiner : public nofWorkman | ||
{ | ||
protected: | ||
|
@@ -19,14 +19,19 @@ class nofMiner : public nofWorkman | |
unsigned short GetCarryID() const override; | ||
/// Der Arbeiter erzeugt eine Ware | ||
helpers::OptionalEnum<GoodType> ProduceWare() override; | ||
/// alter workcycle (addon) | ||
bool isAlteredWorkcycle; | ||
Comment on lines
+22
to
+23
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How is the work cycle "altered" (i.e. "changed", I guess)? I mean: It is the same, it simply produces nothing. See also the usage in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I knew you'd stumble across this :) I called it altered for two reasons, I may also add an addon which lets you switch between "Produce nothing" and "Produce granite" when using settlers IV mining. This also allows other addons (if one likes) like gold mines have a chance to produce a gem. So in my special case, I could name the variable like you suggested, but as this can be a more general variable I named it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
bool AreWaresAvailable() const override; | ||
bool StartWorking() override; | ||
ResourceType GetRequiredResType() const; | ||
MiningBehavior GetMiningBehavior() const; | ||
|
||
public: | ||
nofMiner(MapPoint pos, unsigned char player, nobUsual* workplace); | ||
nofMiner(SerializedGameData& sgd, unsigned obj_id); | ||
|
||
GO_Type GetGOT() const final { return GO_Type::NofMiner; } | ||
|
||
void Serialize(SerializedGameData& sgd) const override; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,10 @@ constexpr GameSpeed referenceSpeed = GameSpeed::Normal; | |
|
||
/// Reichweite der Bergarbeiter | ||
constexpr unsigned MINER_RADIUS = 2; | ||
constexpr unsigned MINER_ORE_RADIUS_SETTLERSIV = 1u; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
This is actually a (code quality) bug, that |
||
|
||
/// maximum quantity for ores | ||
constexpr unsigned MAX_ORE_QUANTITY = 7u; | ||
|
||
/// Konstante für die Pfadrichtung bei einer Schiffsverbindung | ||
constexpr unsigned char SHIP_DIR = 100; | ||
|
There was a problem hiding this comment.
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:
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
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)
vsMining 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 :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.Ouch :D Seriously, I have that settings window in mind and try to imagine how it would look there.
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"
There was a problem hiding this comment.
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":
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.