-
Notifications
You must be signed in to change notification settings - Fork 165
Introduce advanced tax & wage contribution logic #1163
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
base: master
Are you sure you want to change the base?
Changes from 20 commits
d751924
e8eb3f8
1bc7bef
51fde7b
04c5d91
618838e
9a7f864
dc46c8b
91135dc
57a99e0
4979e7e
f154f23
2e34ea6
990456a
ebdaa02
a45a2f8
58eebdf
59c1c30
c834aad
17db04d
1950d23
f9e1f11
db4c714
cc909fe
8993c5c
d7bbfb7
5876bde
4e8dff6
a5e5c5d
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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -172,6 +172,21 @@ static void remove_adjacent_types(building *b) | |||||
| b->next_of_type = 0; | ||||||
| } | ||||||
|
|
||||||
| void initialize_sentiment_cooldown(building *b) { | ||||||
| if (b->house_adv_sentiment.cooldown_initialized) { | ||||||
| return; | ||||||
| } | ||||||
|
|
||||||
| b->house_adv_sentiment.cooldown_initialized = 1; | ||||||
|
|
||||||
| if (b->type <= BUILDING_HOUSE_GRAND_INSULA) { | ||||||
| b->house_adv_sentiment.cooldown = ADVANCED_SENTIMENT_COOLDOWN_MAX_TICKS; | ||||||
| } else { | ||||||
| // Do not apply cooldown to villas | ||||||
| b->house_adv_sentiment.cooldown = 0; | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| building *building_create(building_type type, int x, int y) | ||||||
| { | ||||||
| building *b; | ||||||
|
|
@@ -205,6 +220,7 @@ building *building_create(building_type type, int x, int y) | |||||
|
|
||||||
| // subtype | ||||||
| if (building_is_house(type)) { | ||||||
| initialize_sentiment_cooldown(b); | ||||||
|
Collaborator
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. Not sure you need the function. The cooldown ticks should only be set when the building is actually created (right here in this function), then they'll slowly go down when there are people in the house. For removing cooldowns from villas, just set the ticks to |
||||||
| b->subtype.house_level = type - BUILDING_HOUSE_VACANT_LOT; | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -268,7 +284,7 @@ static void building_delete(building *b) | |||||
|
|
||||||
| void building_clear_related_data(building *b) | ||||||
| { | ||||||
| if (b->storage_id) { | ||||||
| if (building_is_storage_kind(b->type) && b->storage_id) { | ||||||
| building_storage_delete(b->storage_id); | ||||||
| b->storage_id = 0; | ||||||
| } | ||||||
|
|
@@ -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) | ||||||
|
Collaborator
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.
Suggested change
Author
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. Not sure if the suggested naming is somehow better than proposed in the PR. |
||||||
| { | ||||||
| return type == BUILDING_WAREHOUSE || type == BUILDING_GRANARY; | ||||||
| } | ||||||
|
|
||||||
| // For Venus GT base bonus | ||||||
| int building_is_statue_garden_temple(building_type type) | ||||||
| { | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -7,13 +7,27 @@ | |||||||||||||
| #include "game/resource.h" | ||||||||||||||
| #include "translation/translation.h" | ||||||||||||||
|
|
||||||||||||||
| // Ticks of cooldown before new advanced sentiment logic will be applied | ||||||||||||||
| // for the house building. Each tick is equal to 3 months. That value | ||||||||||||||
| // shouldn't ever exceed 7, since it takes 3 bits of data. | ||||||||||||||
| // To extend or reduce the cooldown duration edit the | ||||||||||||||
| // ADVANCED_SENTIMENT_COOLDOWN_TICK_MONTHS preprocessor definition. | ||||||||||||||
| #define ADVANCED_SENTIMENT_COOLDOWN_MAX_TICKS 7 | ||||||||||||||
| #define ADVANCED_SENTIMENT_COOLDOWN_TICK_MONTHS 3 | ||||||||||||||
|
Collaborator
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. Wouldn't it just be simpler to use one tick per month and increase max ticks?
Author
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. 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 |
||||||||||||||
|
|
||||||||||||||
| typedef enum order_condition_type { | ||||||||||||||
| ORDER_CONDITION_NEVER = 0, | ||||||||||||||
| ORDER_CONDITION_ALWAYS, | ||||||||||||||
| ORDER_CONDITION_SOURCE_HAS_MORE_THAN, | ||||||||||||||
| ORDER_CONDITION_DESTINATION_HAS_LESS_THAN | ||||||||||||||
| } order_condition_type; | ||||||||||||||
|
|
||||||||||||||
| typedef struct advanced_sentiment { | ||||||||||||||
| uint8_t cooldown_initialized: 1; | ||||||||||||||
| uint8_t cooldown: 3; | ||||||||||||||
| uint8_t reserved: 4; | ||||||||||||||
| } advanced_sentiment; | ||||||||||||||
|
Collaborator
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. Please, no bitfields. Just use plain
Author
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. 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
Collaborator
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. 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. |
||||||||||||||
|
|
||||||||||||||
| typedef struct order { | ||||||||||||||
| resource_type resource_type; | ||||||||||||||
| int src_storage_id; | ||||||||||||||
|
|
@@ -180,6 +194,11 @@ typedef struct building { | |||||||||||||
| signed char house_happiness; | ||||||||||||||
| signed char native_anger; | ||||||||||||||
| } sentiment; | ||||||||||||||
| // 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; | ||||||||||||||
|
Collaborator
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.
Suggested change
No need to call it Also, the explanation of the mechanics can stay on the |
||||||||||||||
| unsigned char show_on_problem_overlay; | ||||||||||||||
| unsigned char house_tavern_wine_access; | ||||||||||||||
| unsigned char house_tavern_food_access; | ||||||||||||||
|
|
@@ -220,6 +239,8 @@ building *building_main(building *b); | |||||||||||||
|
|
||||||||||||||
| building *building_next(building *b); | ||||||||||||||
|
|
||||||||||||||
| void initialize_sentiment_cooldown(building *b); | ||||||||||||||
|
|
||||||||||||||
|
Collaborator
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. As mentioned above, I don't think this function is needed. But if it was needed, since it is a public function, it should be declared as |
||||||||||||||
| building *building_create(building_type type, int x, int y); | ||||||||||||||
|
|
||||||||||||||
| void building_clear_related_data(building *b); | ||||||||||||||
|
|
@@ -232,6 +253,11 @@ void building_update_state(void); | |||||||||||||
|
|
||||||||||||||
| void building_update_desirability(void); | ||||||||||||||
|
|
||||||||||||||
| /** | ||||||||||||||
| * Checks if building can store goods | ||||||||||||||
| */ | ||||||||||||||
| int building_is_storage_kind(building_type type); | ||||||||||||||
|
|
||||||||||||||
| int building_is_house(building_type type); | ||||||||||||||
|
|
||||||||||||||
| int building_is_ceres_temple(building_type type); | ||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,7 @@ | ||
| #include "state.h" | ||
|
|
||
| #include <assert.h> | ||
|
|
||
|
Comment on lines
+3
to
+4
Collaborator
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. Remove include. See below. |
||
| #include "building/industry.h" | ||
| #include "building/monument.h" | ||
| #include "building/roadblock.h" | ||
|
|
@@ -164,6 +166,14 @@ void building_state_save_to_buffer(buffer *buf, const building *b) | |
| buffer_write_u8(buf, b->is_adjacent_to_water); | ||
| buffer_write_u8(buf, b->storage_id); | ||
| buffer_write_i8(buf, b->sentiment.house_happiness); // which union field we use does not matter | ||
|
|
||
| assert (sizeof(b->house_adv_sentiment) == sizeof(uint8_t)); | ||
|
Collaborator
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. No need for the |
||
| if (building_is_house(b->type)) { | ||
| buffer_write_raw(buf, &b->house_adv_sentiment, sizeof(b->house_adv_sentiment)); | ||
| } else { | ||
| buffer_skip(buf, sizeof(b->house_adv_sentiment)); | ||
| } | ||
|
Comment on lines
+171
to
+174
Collaborator
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 understand your change here, however, I try to avoid bumping the savegame version as much as I can. You have a line above ( buffer_write_i16(buf, 0);This means you have two bytes of unused space there, which you can use to store the cooldown ticks. So I'd replace that line with: buffer_write_i16(buf, sentiment_cooldown_ticks);Yes the variable is being converted from a 32-bit int to a 16-bit int, but I hardly think we'll need more than |
||
|
|
||
| buffer_write_u8(buf, b->show_on_problem_overlay); | ||
|
|
||
| // expanded building data | ||
|
|
@@ -486,8 +496,20 @@ void building_state_load_from_buffer(buffer *buf, building *b, int building_buf_ | |
| b->desirability = buffer_read_i8(buf); | ||
| b->is_deleted = buffer_read_u8(buf); | ||
| b->is_adjacent_to_water = buffer_read_u8(buf); | ||
| b->storage_id = buffer_read_u8(buf); | ||
| if (building_is_storage_kind(b->type)) { | ||
| b->storage_id = buffer_read_u8(buf); | ||
| } else { | ||
| b->storage_id = 0; | ||
| buffer_skip(buf, 1); // do not load storage_id for non-storage buildings | ||
|
Collaborator
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. No need for the comment, the code is self explanatory. As a matter of fact, comments should be avoided unless you can't really understand what's going on by just looking at the code. That forces you to improve code readability, which in turn is better for other coders to understand what's going on. See below for an example where your comments are a good idea. Other than that, I agree with this particular change. This will avoid weird corruptions in the future. |
||
| } | ||
| b->sentiment.house_happiness = buffer_read_i8(buf); // which union field we use does not matter | ||
| if (save_version >= SAVE_GAME_LAST_ADVANCED_SENTIMENT) { | ||
| if (building_is_house(b->type)) { | ||
| buffer_read_raw(buf, &b->house_adv_sentiment, sizeof(b->house_adv_sentiment)); | ||
| } else { | ||
| buffer_skip(buf, sizeof(b->house_adv_sentiment)); | ||
| } | ||
| } | ||
|
Comment on lines
+506
to
+511
Collaborator
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. As mentioned above, you can use the two free bytes. You may notice that on line However that variable is only set to a value other than So you can set line int loads_stored = 0;
if (save_version <= SAVE_GAME_LAST_NO_NEW_MONUMENT_RESOURCES) {
loads_stored = buffer_read_i16(buf);
} else {
b->sentiment_cooldown = buffer_read_i16(buf);
}That way you can use that value and avoid bumping the savegame version. |
||
| b->show_on_problem_overlay = buffer_read_u8(buf); | ||
|
|
||
| // Wharves produce fish and don't need any progress | ||
|
|
@@ -687,4 +709,8 @@ void building_state_load_from_buffer(buffer *buf, building *b, int building_buf_ | |
| if (building_buf_size > BUILDING_STATE_CURRENT_BUFFER_SIZE) { | ||
| buffer_skip(buf, building_buf_size - BUILDING_STATE_CURRENT_BUFFER_SIZE); | ||
| } | ||
|
|
||
| if (building_is_house(b->type)) { | ||
| initialize_sentiment_cooldown(b); | ||
| } | ||
|
Collaborator
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 really don't think this is needed. This will reinitialize the sentiment cooldown to ongoing cities that are loaded, which I don't think is needed or even recommended. Worse, it will allow exploits by saving/reloading the cities. |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
Owner
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. 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. |
||
| #define BUILDING_STATE_SICKNESS (BUILDING_STATE_ADV_SENTIMENT + 5) // 143 | ||
| #define BUILDING_STATE_WITHOUT_RESOURCES (BUILDING_STATE_SICKNESS - RESOURCE_MAX_LEGACY) // 127 (plus variable resource size) | ||
|
Collaborator
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. By not adding bytes to the savefile, you don't need to bump the version and you also don't need these defines. |
||
| #define BUILDING_STATE_DYNAMIC_RESOURCES (BUILDING_STATE_WITHOUT_RESOURCES + BUILDING_STATE_NONSTATIC_RESOURCE_SIZE) | ||
| #define BUILDING_STATE_CURRENT_BUFFER_SIZE (BUILDING_STATE_DYNAMIC_RESOURCES + 8) | ||
|
|
||
|
|
||
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 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?
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 ainitialize_sentiment_cooldown, then probably function could be removed, since you didn't like theinitializedflag, probably it could be removed in favor of checking save file version instead.