-
-
Notifications
You must be signed in to change notification settings - Fork 24
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
Econ 2.0 Buildings #572
base: master
Are you sure you want to change the base?
Econ 2.0 Buildings #572
Conversation
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.
Mostly semantics, looks very very good overall. I'm sure Idhrendur will find tons more though.
EU4ToVic3/Source/Mappers/BuildingMapper/ProductionMethodMapper/ProductionMethodMapper.cpp
Outdated
Show resolved
Hide resolved
EU4ToVic3/Source/Mappers/BuildingMapper/ProductionMethodMapper/ProductionMethodMapper.h
Outdated
Show resolved
Hide resolved
EU4ToVic3/Source/V3World/EconomyManager/Building/BuildingResources.cpp
Outdated
Show resolved
Hide resolved
EU4ToVic3/Source/V3World/EconomyManager/Building/BuildingResources.h
Outdated
Show resolved
Hide resolved
} | ||
for (const auto& type: investorWeights | std::views::keys) | ||
{ | ||
investorWeights[type] /= totalWeight; |
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.
Can we have a recognized country starting with no investors thus totalWeight = 0? Maybe some peasant republic going full communist or something?
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.
Those would be "national" or "local" investors. Also this function is turning the weights from configurbales/economy/ownership.txt into fractions on a country by country basis. I've refactored a couple things to hopefully paint a clearer picture there.
EU4ToVic3Tests/MapperTests/BuildingMapperTests/ProductionMethodMapperTests.cpp
Outdated
Show resolved
Hide resolved
EU4ToVic3Tests/MapperTests/BuildingMapperTests/ProductionMethodMapperTests.cpp
Show resolved
Hide resolved
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'm about 29% through, will continue in the coming days. For the future, when you've got bits fairly stable, it doesn't hurt to spin them off into smaller PRs so we can review and approve faster.
EU4ToVic3/Source/V3World/EconomyManager/Building/BuildingGroups.h
Outdated
Show resolved
Hide resolved
EU4ToVic3/Source/V3World/EconomyManager/Building/ProductionMethods/ProductionMethod.cpp
Show resolved
Hide resolved
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.
86% reviewed
EU4ToVic3Tests/MapperTests/BuildingMapperTests/ProductionMethodMapperTests.cpp
Show resolved
Hide resolved
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.
Sorry for the pause, 90% reviewed.
EU4ToVic3Tests/V3WorldTests/EconomyManagerTests/DemandTests/MarketJobsTests.cpp
Outdated
Show resolved
Hide resolved
Co-authored-by: Idhrendur <[email protected]>
auto sub0 = std::make_shared<V3::SubState>(); | ||
auto sub1 = std::make_shared<V3::SubState>(); | ||
|
||
V3::Pop pop0, pop1, pop2; |
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.
It would be more clear (and fewer lines) to use the constructor:
V3::Pop pop0, pop1, pop2; | |
V3:Pop pop0("english", "", "", 1000); | |
V3:Pop pop1("german", "", "", 500); | |
V3:Pop pop1("english", "", "", 500); |
@@ -333,4 +366,106 @@ TEST(V3World_CountryTests, YearCapFactorHitsCurve) | |||
EXPECT_NEAR(0.3, V3::Country::yearCapFactor(date("1650.1.1")), 0.015); | |||
EXPECT_NEAR(0.2, V3::Country::yearCapFactor(date("1490.1.1")), 0.01); | |||
EXPECT_NEAR(0.15, V3::Country::yearCapFactor(date("1350.1.1")), 0.01); | |||
} | |||
|
|||
TEST(V3World_CountryTests, HasAnyOfLawUnlockingPositiveCase) |
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.
TEST(V3World_CountryTests, HasAnyOfLawUnlockingPositiveCase) | |
TEST(V3World_CountryTests, HasAnyOfLawUnlockingIsTrueIfAnyOfLawsArePresent) |
EXPECT_TRUE(country.hasAnyOfLawUnlocking({"law_2", "law_3"})); | ||
} | ||
|
||
TEST(V3World_CountryTests, HasAnyOfLawUnlockingNegativeCase) |
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.
TEST(V3World_CountryTests, HasAnyOfLawUnlockingNegativeCase) | |
TEST(V3World_CountryTests, HasAnyOfLawUnlockingFalseIfNoneOfLawsPresent) |
EXPECT_FALSE(country.hasAnyOfLawUnlocking({"law_1", "law_5"})); | ||
} | ||
|
||
TEST(V3World_CountryTests, HasAnyOfLawBlockingPositiveCase) |
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.
TEST(V3World_CountryTests, HasAnyOfLawBlockingPositiveCase) | |
TEST(V3World_CountryTests, HasAnyOfLawBlockingTrueIfAnyOfLawsArePresent) |
EXPECT_TRUE(country.hasAnyOfLawBlocking({"law_2", "law_3"})); | ||
} | ||
|
||
TEST(V3World_CountryTests, HasAnyOfLawBlockingNegativeCase) |
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.
TEST(V3World_CountryTests, HasAnyOfLawBlockingNegativeCase) | |
TEST(V3World_CountryTests, HasAnyOfLawBlockingFalseIfNoneOfLawsPresent) |
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.
Remember to write tests here, as much as possible.
@@ -89,34 +94,44 @@ class EconomyManager | |||
[[nodiscard]] double calculateStateTraitMultiplier(const std::shared_ptr<SubState>& subState) const; | |||
[[nodiscard]] double getDensityFactor(double perCapitaDev) const; | |||
|
|||
void establishBureaucracy(const std::shared_ptr<Country>& country, const std::map<std::string, Law>& lawsMap, const Vic3DefinesLoader& defines) const; |
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.
void establishBureaucracy(const std::shared_ptr<Country>& country, const std::map<std::string, Law>& lawsMap, const Vic3DefinesLoader& defines) const; | |
void establishBureaucracy(const Country& country, const std::map<std::string, Law>& lawsMap, const Vic3DefinesLoader& defines) const; |
You're not taking (shared) ownership of the country, so just take a reference to it.
@@ -89,34 +94,44 @@ class EconomyManager | |||
[[nodiscard]] double calculateStateTraitMultiplier(const std::shared_ptr<SubState>& subState) const; | |||
[[nodiscard]] double getDensityFactor(double perCapitaDev) const; | |||
|
|||
void establishBureaucracy(const std::shared_ptr<Country>& country, const std::map<std::string, Law>& lawsMap, const Vic3DefinesLoader& defines) const; | |||
void hardcodePorts(const std::shared_ptr<Country>& country) const; |
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.
void hardcodePorts(const std::shared_ptr<Country>& country) const; | |
void hardcodePorts(const Country& country) const; |
Ditto.
{ | ||
for (const auto& subState: country->getSubStates()) | ||
{ | ||
for (const auto& building: subState->getBuildings()) |
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.
Are you getting a shared_ptr to a building? Maybe see if you can dereference it immediately in the loop construct.
@@ -89,34 +94,44 @@ class EconomyManager | |||
[[nodiscard]] double calculateStateTraitMultiplier(const std::shared_ptr<SubState>& subState) const; | |||
[[nodiscard]] double getDensityFactor(double perCapitaDev) const; | |||
|
|||
void establishBureaucracy(const std::shared_ptr<Country>& country, const std::map<std::string, Law>& lawsMap, const Vic3DefinesLoader& defines) const; | |||
void hardcodePorts(const std::shared_ptr<Country>& country) const; | |||
void integrateHardcodedBuildings(const std::shared_ptr<Country>& country, |
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.
void integrateHardcodedBuildings(const std::shared_ptr<Country>& country, | |
void integrateHardcodedBuildings(const Country& country, |
Ditto
Demand Building Component
Load employment from PMs(Already did, thanks past me.)Load input/output goods from PMsBalancing- Next PR- [ ] Have demand be primary factor, but allow the other weights to punch through at times.- [ ] Add function to generate extra arable land for extra pop.- [ ] Check additional bureaucracy cost incurred by nationalization, supplementary admin round if needed.Jobs
Buildings
Debug
Ownership