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

Persist the last cycle tick in emigration for a more consistent gameplay experience #1330

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

master-spike
Copy link
Contributor

@master-spike master-spike commented Oct 27, 2024

@myk002
Copy link
Member

myk002 commented Nov 12, 2024

I fixed the formatting of the "Fixes" line in the description so GitHub can understand it and link the issue properly

changelog.txt Outdated
@@ -41,6 +41,7 @@ Template for new versions:

## Misc Improvements
- `control-panel`: Add realistic-melting tweak to control-panel registry
- `emigration`: last cycle tick is persisted so that save-and-reload no longer resets the emigration timeout, making gameplay more consistent
Copy link
Member

Choose a reason for hiding this comment

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

I'd categorize this as a "Fix" since it prevents the undesired behavior of artificially increased rate of emigration

emigration.lua Outdated
@@ -191,12 +192,20 @@ function checkmigrationnow()
else
for _, civ_id in pairs(merchant_civ_ids) do checkForDeserters('merchant', civ_id) end
end

last_cycle_tick = dfhack.world.ReadCurrentTick() + 403200 * dfhack.world.ReadCurrentYear()
Copy link
Member

Choose a reason for hiding this comment

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

please add named constants instead of magic numbers. example:

local TICKS_PER_DAY = 1200
local TICKS_PER_MONTH = 28 * TICKS_PER_DAY
local TICKS_PER_SEASON = 3 * TICKS_PER_MONTH

note to self: we really really need to fix up and merge DFHack/dfhack#3907

emigration.lua Outdated
@@ -210,8 +219,9 @@ dfhack.onStateChange[GLOBAL_KEY] = function(sc)
return
end

local persisted_data = dfhack.persistent.getSiteData(GLOBAL_KEY, {enabled=false})
local persisted_data = dfhack.persistent.getSiteData(GLOBAL_KEY, {enabled=false,last_cycle_tick=0})
Copy link
Member

Choose a reason for hiding this comment

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

this will leave last_cycle_tick as nil in current games with saved state. It would be better to normalize the value here so you can remove the check for nil in event_loop.

I suggest a pattern like this:

    state = get_default_state()
    utils.assign(state, dfhack.persistent.getSiteData(GLOBAL_KEY, state))

emigration.lua Outdated
Comment on lines 6 to 7
enabled = enabled or false
last_cycle_tick = last_cycle_tick or 0
Copy link
Member

Choose a reason for hiding this comment

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

now that we have more than just enabled here, I suggest making this clearer by putting the state in a table.

local function get_default_state()
    return {enabled=false, last_cycle_tick=0}
end

state = state or get_default_state()

@master-spike master-spike force-pushed the emigration_timeout_persist branch 3 times, most recently from 2dd967a to db847ce Compare November 13, 2024 13:28
…consistent with respect to save-and-reloads
@master-spike
Copy link
Contributor Author

Tested and works

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Todo
2 participants