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

[Bug]: Wallclock mode minutes per year setting does not work correctly for values other than 0 and 12 #12154

Closed
JGRennison opened this issue Feb 22, 2024 · 1 comment · Fixed by #12158
Assignees
Labels
bug Something isn't working
Milestone

Comments

@JGRennison
Copy link
Contributor

Version of OpenTTD

master

Expected result

Wallclock mode minutes per year setting does what it says on the tin

Actual result

At minutes per year = 12, a calendar day is 74 ticks
At minutes per year = 13, a calendar day is 153 ticks
At minutes per year = 24, a calendar day is 221 ticks

TimerGameCalendar::sub_date_fract counts from 0 to ((74 * min_per_year) / 12) - 1
then date_fract counts from 0 to 74

Steps to reproduce

Use the minutes per year setting

@2TallTyler 2TallTyler added the bug Something isn't working label Feb 22, 2024
@2TallTyler 2TallTyler added this to the 14.0 milestone Feb 22, 2024
@2TallTyler 2TallTyler self-assigned this Feb 22, 2024
@2TallTyler
Copy link
Member

Yikes! I changed this algorithm immediately before merge after using a different one for months, and apparently failed to properly test it.

For anyone looking into this, here's the relevant code: https://github.com/OpenTTD/OpenTTD/blob/master/src/timer/timer_game_calendar.cpp#L106-L119

The original algorithm scaled the number of date_fract ticks in a day. @TrueBrain rejected that because we expose date_fract to GRFs and shouldn't break their assumptions that it only goes to 74 before resetting. Our discussion resulted in sub_date_fract. (I am absolutely not throwing TB under the bus here, my bugs are my fault! Just trying to give some background.)

This whole algorithm probably needs to be rewritten from scratch, because it's broken in several ways.

I will do some thinking about this, but if anyone else has any ideas, feel free to chime in! 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants