-
Notifications
You must be signed in to change notification settings - Fork 121
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
Introduce advanced tax & wage contribution logic #1163
base: master
Are you sure you want to change the base?
Introduce advanced tax & wage contribution logic #1163
Conversation
…res storage_id/sentiment_cooldown
…anced-tax-wage-sentiment-contribution
…ile version to 0x9f
…anced-tax-wage-sentiment-contribution
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.
Ok, I've finally looked at this.
While I think the core of the changes is going in a good direction, there are some points I don't quite agree.
Anyway, if you have any questions please let me know!
@@ -205,6 +220,7 @@ building *building_create(building_type type, int x, int y) | |||
|
|||
// subtype | |||
if (building_is_house(type)) { |
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 (building_is_house(type)) { | |
if (building_is_house(type) && b->subtype.house_level < HOUSE_SMALL_VILLA) { |
I think this would be more readable and not require the check in the function.
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.
Which check are you referring to?
int building_is_house(building_type type)
{
return type >= BUILDING_HOUSE_VACANT_LOT && type <= BUILDING_HOUSE_LUXURY_PALACE;
}
It has nothing to the check of villa house, just a generic housing. Also in that if branch there is also an assignment b->subtype.house_level = type - BUILDING_HOUSE_VACANT_LOT;
which will stop working for villas with your suggestion. If you're referring to a initialize_sentiment_cooldown
, then probably function could be removed, since you didn't like the initialized
flag, probably it could be removed in favor of checking save file version instead.
src/building/building.c
Outdated
@@ -424,6 +440,11 @@ int building_is_house(building_type type) | |||
return type >= BUILDING_HOUSE_VACANT_LOT && type <= BUILDING_HOUSE_LUXURY_PALACE; | |||
} | |||
|
|||
int building_is_storage_kind(building_type type) |
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.
int building_is_storage_kind(building_type type) | |
int building_uses_storage(building_type type) |
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.
Not sure if the suggested naming is somehow better than proposed in the PR.
src/building/building.h
Outdated
typedef struct advanced_sentiment { | ||
uint8_t cooldown_initialized: 1; | ||
uint8_t cooldown: 3; | ||
uint8_t reserved: 4; | ||
} advanced_sentiment; |
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.
Please, no bitfields. Just use plain int
s in the house
structure. We're not starved for memory.
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.
That were done under the purpose of potential improvements to keep compatibility with versions without making a breaking change. By using int, no more future improvements which could require some bit flags possible without incrementing the save version
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.
Frankly I don't think it's worth it. We're saving just 4 bits and then we'll need a version bump anyway. And I'd really prefer to use a whole int, even if wasteful.
I mean if you want to save some savegame bytes for later you might as well reserve them now, if we do bump the savegame version.
src/building/building.h
Outdated
#define ADVANCED_SENTIMENT_COOLDOWN_MAX_TICKS 7 | ||
#define ADVANCED_SENTIMENT_COOLDOWN_TICK_MONTHS 3 |
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.
Wouldn't it just be simpler to use one tick per month and increase max ticks?
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 use an int, then yes, but as I replied above, there will be no room for improvements without breaking changes to save files. The initial idea was to re-use existing byte of data previously not used by houses, however it then were moved to it's own memory
src/building/building.h
Outdated
// New advanced sentiment contribution logic requires cooldown for newly built houses. | ||
// The new happiness gain/drop logic will not be applied until cooldown expires. | ||
// Cooldown ticks get decreased every Jan/Apr/Jul/Oct, which gives 18-20 months in total. | ||
// That should be enough to build new housing block and evolve it. | ||
struct advanced_sentiment house_adv_sentiment; |
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.
// New advanced sentiment contribution logic requires cooldown for newly built houses. | |
// The new happiness gain/drop logic will not be applied until cooldown expires. | |
// Cooldown ticks get decreased every Jan/Apr/Jul/Oct, which gives 18-20 months in total. | |
// That should be enough to build new housing block and evolve it. | |
struct advanced_sentiment house_adv_sentiment; | |
int sentiment_cooldown_ticks; |
No need to call it advanced
or anything 😉 Also, I think only the actual ticks
are needed. See above.
Also, the explanation of the mechanics can stay on the sentiment.c
side of things.
src/city/sentiment.h
Outdated
@@ -25,6 +25,6 @@ void city_sentiment_reduce_crime_cooldown(void); | |||
int city_sentiment_get_blessing_festival_boost(void); | |||
void city_sentiment_decrement_blessing_boost(void); | |||
|
|||
void city_sentiment_update(void); | |||
void city_sentiment_update(int update_sentiment_cooldown); |
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.
We can either do this (call the function with update_sentiment_cooldown
enabled or disabled), or we can double the number of ticks as well as reduce the sentiment jump effect by half, therefore making the whole thing more granular.
This is also only possible because I got rid of tick updates only every X months.
@@ -72,6 +72,7 @@ static const char *ini_keys[] = { | |||
"gameplay_change_yearly_autosave", | |||
"gameplay_change_auto_kill_animals", | |||
"gameplay_change_nonmilitary_gates_allow_walkers", | |||
"gameplay_change_advanced_tax_wage_sentiment_contribution", |
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.
As mentioned above, I don't think we need a config for this.
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 think we do need, it's still could require some improvements, but should be distributed with players to test. In the end of course it could become non-optional, but for now it's suggested to provide an option
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 think it's fine to implement this as an option now for testing, but endstate being integrating this change as a base. We may do it with the larger roundup of option integration before the next release.
src/game/save_version.h
Outdated
@@ -2,7 +2,7 @@ | |||
#define GAME_SAVE_VERSION_H | |||
|
|||
typedef enum { | |||
SAVE_GAME_CURRENT_VERSION = 0x9e, | |||
SAVE_GAME_CURRENT_VERSION = 0x9f, |
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.
As mentioned above, no need for save version bump.
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 don't think I'm following this suggestion
src/game/tick.c
Outdated
int update_sentiment_cooldown = game_time_day() == 0; | ||
if (update_sentiment_cooldown || game_time_day() == 8) { | ||
city_sentiment_update(update_sentiment_cooldown); |
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.
int update_sentiment_cooldown = game_time_day() == 0; | |
if (update_sentiment_cooldown || game_time_day() == 8) { | |
city_sentiment_update(update_sentiment_cooldown); | |
if (game_time_day() == 0 || game_time_day() == 8) { | |
city_sentiment_update(); |
By having more granular value changes and more cooldown ticks you can still update sentiment twice a month.
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 using int probably yes
…anced-tax-wage-sentiment-contribution
src/building/state.h
Outdated
@@ -11,8 +11,9 @@ | |||
#define BUILDING_STATE_TOURISM_BUFFER_SIZE (BUILDING_STATE_ORIGINAL_BUFFER_SIZE + 6) // 134 | |||
#define BUILDING_STATE_VARIANTS_AND_UPGRADES (BUILDING_STATE_TOURISM_BUFFER_SIZE + 2) // 136 | |||
#define BUILDING_STATE_STRIKES (BUILDING_STATE_VARIANTS_AND_UPGRADES + 1) // 137 | |||
#define BUILDING_STATE_SICKNESS (BUILDING_STATE_STRIKES + 5) // 142 | |||
#define BUILDING_STATE_WITHOUT_RESOURCES (BUILDING_STATE_SICKNESS - RESOURCE_MAX_LEGACY) // 126 (plus variable resource size) | |||
#define BUILDING_STATE_ADV_SENTIMENT (BUILDING_STATE_STRIKES + sizeof(advanced_sentiment)) // 138 |
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.
These defines here are used for the purpose of backward compatibility, the state where we actually have the newly added values never happened, so any additions to the size should be at the end of the segment.
…anced-tax-wage-sentiment-contribution
@@ -35,6 +37,22 @@ | |||
#define IMPERIAL_GAMES_SENTIMENT_BONUS 15 | |||
#define POP_STEP_FOR_BASE_AVERAGE_HOUSE_LEVEL 625 | |||
|
|||
#define min(a,b) \ |
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.
Looks like MSVC doesn't support these kind of macros, so MSVC builds fail. Could you perhaps add it as a function instead? Or maybe even just inline it. It looks like it's only being used once.
Description
This PR adds configurable option for advanced Tax & Wage sentiment contribution logic. This is recommended to prevent exploiting in-game default tax & wages logic, when player sets very high taxes and wages to make profit. With new advanced logic proposal tax & wages are divided by intervals and every next interval gives twice less sentiment modifier than previous. Additionally the new sentiment/happiness drop has been applied. Originally we had a maximum drop rate of -2 sentiment per tick. With new settings it has increased. To prevent suffering at early game or when building a new block the cooldowns has been applied for first 30 months of gameplay and for 18-20 months for each house individually (starts once house get first citizens). Cooldown resets once house becomes villa.
Example:
Some house happiness level is 82, new sentiment/happiness target is 10, the raw change is -72. Difference percent is 72/82*100% = 87%. In VeryHard mode 50% of it is applied to a new delta change, which is 43% of -72 => -30. So on next tick the house will loose 30 points of happiness instead of 2.