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

Fix #10469, 5e14a20: [Script] League Table rating element is a int64 everywhere else #10471

Merged

Conversation

SamuXarick
Copy link
Contributor

@SamuXarick SamuXarick commented Feb 12, 2023

Motivation / Problem

Closes #10469
Rating is a int64 in the Cmd functions, in the script API functions, but not on the league table element struct. Sorting through negative values would cause 0 to be below them.

Description

Turn rating in struct into int64, and add savegame bump and conversion.

Limitations

Probably will cause issues if the values saved on an old savegame were already above INT64_MAX to begin with, they would turn negative after load, most likely.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR touches english.txt or translations? Check the guidelines
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@SamuXarick SamuXarick force-pushed the league-table-element-int64-rating branch from 45a9d65 to 5bed5e6 Compare February 12, 2023 14:49
ldpl
ldpl previously approved these changes Feb 12, 2023
Copy link
Contributor

@ldpl ldpl left a comment

Choose a reason for hiding this comment

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

I don't remember why it ended up with different types. Probably I changed it at some point and missed one place. At the end of the day it's just an arbitrary field only used for sorting so it doesn't matter much what type it is. But as GS types are signed having it stored as signed as well makes more sense. Probably GS methods can even accept SQInteger or whatever is GS type called. But as SQInteger isn't used in other functions yet it's probably fine to leave it as int64 is for this PR.

Also, savegame conversion stuff and version bump is logically redundant as it is just a int64 stored in that uint64 anyway. But I don't know of a better way to express that in the new save format so I guess it's fine as it is.

@glx22
Copy link
Contributor

glx22 commented Feb 13, 2023

I don't think any changes in the savegame stuff is needed, using SLE_UINT64 for an int64 should work fine.

@ldpl
Copy link
Contributor

ldpl commented Feb 13, 2023

I don't think any changes in the savegame stuff is needed, using SLE_UINT64 for an int64 should work fine.

It feels kinda dumb to have to use a wrong type to bypass a type check.

@SamuXarick SamuXarick force-pushed the league-table-element-int64-rating branch 3 times, most recently from 79a8318 to 29cec53 Compare February 18, 2023 11:49
@2TallTyler 2TallTyler added the backport requested This PR should be backport to current release (RC / stable) label Feb 20, 2023
@rubidium42
Copy link
Contributor

I don't think any changes in the savegame stuff is needed, using SLE_UINT64 for an int64 should work fine.

It feels kinda dumb to have to use a wrong type to bypass a type check.

There is a middle ground solution SLE_VAR(LeagueTableElement, rating, SLE_FILE_U64 | SLE_VAR_I64),, which could be accompanied by a comment that the type in the file is technically wrong, but that a savegame bump is an excessive change. But that, when a savegame bump is required for the LeageTableElement, this should be corrected by a proper change as well.

@rubidium42 rubidium42 force-pushed the league-table-element-int64-rating branch from 29cec53 to 46a5a55 Compare February 26, 2023 20:56
@rubidium42 rubidium42 enabled auto-merge (rebase) February 26, 2023 21:02
@glx22
Copy link
Contributor

glx22 commented Feb 26, 2023

Backport can only be partial due to savegame bump, but uint64 -> int64 itself can go to release branch I think.

@rubidium42
Copy link
Contributor

Backport can only be partial due to savegame bump, but uint64 -> int64 itself can go to release branch I think.

Hmm, true. How to annotate that for the poor soul that's going to try to backport?

@rubidium42 rubidium42 merged commit 6eabbaa into OpenTTD:master Feb 26, 2023
@SamuXarick SamuXarick deleted the league-table-element-int64-rating branch February 26, 2023 21:27
@rubidium42 rubidium42 added backported This PR is backported to a current release (RC / stable) and removed backport requested This PR should be backport to current release (RC / stable) labels Apr 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported This PR is backported to a current release (RC / stable)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: League Table: zero is lower than any negative number
5 participants