Skip to content
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

Adds RequiredCountryBuy for Crafts and Soldiers. #118

Closed

Conversation

MaxMahem
Copy link

As the title says, it implements the feature requested here. It also adds the feature to craft. The filtering function is also reworked. The main improvement is that filtering for RequiredCountryBuy for items is processed when the items are added to the list, rather than whenever the list is updated. Also, the logic is centralized and standardized between Crafts, Soldiers, and Items.

@@ -54,6 +54,22 @@
namespace OpenXcom
{

/**
* @brief Combines any number of functions into a functions that returns true if all of them are true.
Copy link
Owner

Choose a reason for hiding this comment

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

"into a function" without the plural s

template <typename... Functions>
inline constexpr auto allOf(Functions... funcs)
{
return [=](const auto &x)
Copy link
Owner

Choose a reason for hiding this comment

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

Please left-align the * and &
In all new code.
I have also changed the clang format few days ago to help with this.

@@ -159,12 +175,39 @@ PurchaseState::PurchaseState(Base *base, CannotReequipState *parent) : _base(bas
_cats.push_back("STR_FILTER_MISSING");
}

RuleBaseFacilityFunctions providedBaseFunc = _base->getProvidedBaseFunc({});
auto providedBaseFunc = _base->getProvidedBaseFunc({});
Copy link
Owner

Choose a reason for hiding this comment

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

Please leave the explicit type name (RuleBaseFacilityFunctions).
I try to use auto only for pointers, references, lambdas, etc.
For simple ints, bools, etc. I try to avoid auto.
I also try to avoid auto when a copy is made; or where there is a chance of any confusion; or lack of specificity.
See 9a90fab

};
constexpr auto requiredCountryAllied = [](const auto *rule)
{
std::string_view requiredCountry = rule->getRequiresBuyCountry();
Copy link
Owner

Choose a reason for hiding this comment

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

This is OK, but we don't use std::string_view (yet).
Maybe replace with const std::string& just for the oldtimers :)
@Yankes any opinion on that?

return true;
};

auto standardFilter = allOf(costIsNotZero, requirementsAreResearched, necessaryBaseFunctionsPresent, requiredCountryAllied);
Copy link
Owner

Choose a reason for hiding this comment

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

probably missing a function for item's "buy requirements"?
see below
or maybe something else was intended and forgotten...

@@ -215,8 +257,7 @@ PurchaseState::PurchaseState(Base *base, CannotReequipState *parent) : _base(bas
for (auto& itemType : _game->getMod()->getItemsList())
{
RuleItem *rule = _game->getMod()->getItem(itemType);
RuleBaseFacilityFunctions purchaseBaseFunc = rule->getRequiresBuyBaseFunc();
if (rule->getBuyCost() != 0 && _game->getSavedGame()->isResearched(rule->getRequirements()) && _game->getSavedGame()->isResearched(rule->getBuyRequirements()) && (~providedBaseFunc & purchaseBaseFunc).none())
if (standardFilter(rule))
Copy link
Owner

Choose a reason for hiding this comment

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

BUG:
_game->getSavedGame()->isResearched(rule->getBuyRequirements())
was incorrectly removed

@@ -165,6 +165,7 @@ class RuleCraft
std::string _type;
std::vector<std::string> _requires;
RuleBaseFacilityFunctions _requiresBuyBaseFunc;
std::string _requiresBuyCountry;
Copy link
Owner

Choose a reason for hiding this comment

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

add also to StatsForNerdsState::initCraftList()

@MeridianOXC
Copy link
Owner

Fixed and merged manually: 80c02c6

@MeridianOXC MeridianOXC closed this Apr 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants