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

Allow Scripts to convert returned uint32 values to int64 #10373

Closed
wants to merge 16 commits into from

Conversation

SamuXarick
Copy link
Contributor

@SamuXarick SamuXarick commented Jan 18, 2023

Motivation / Problem

ScriptBase Rand functions returns values that are negative at times, while the documentation about them mentions that they should be in a range starting from 0 to their max.

As a quick fix to that problem, I allowed Scripts to convert returned any uint32 values to int64. But such fix was traversal to the entire API, I quickly realized some values could now be returning different results. One such case that was detected by regression test was the return of an invalid BridgeID. BridgeID itself is a uint. Multiple conversions were occurring before the final result was delivered.

return (BridgeID)-1 was actually doing 2 conversions. -1 to uint = 4 294 967 295, then from uint to int32 = 4 294 967 295 to -1 and that was the final result it was being delivered. But now that the final conversion is to int64, the new result is 4 294 967 295 again, and that's what regression caught on.

I knew then that there could be more functions which could now be returning wrong values, so I went with a search or every API function returning uint32's and made changes accordingly.

I also took the liberty to fix some related bugs along the way, mainly if they didn't match their documentation, such as when they should return -1 for invalid parameters. I also created compatibility functions so that older scripts, would still behave as they did previously to the changes.

Description

  • ScriptBase::Rand now correctly returns a value between 0 and MAX(uint32)

  • Script::RandItem now correctly returns a value between 0 and MAX(uint32)

  • ScriptBase::RandRange now correctly returns a value between 0 and 'max - 1'

  • ScriptBase::RandRangeItem now correctly returns a value between 0 and 'max - 1'

  • ScriptBase::Chance randomized 'max' value is now in the correct range when compared with 'out'

  • ScriptBase::ChanceItem randomized 'max' value is now in the correct range when compared with 'out'
    All 6 above were fixed by converting returned uint32 values as int64 instead of int32.
    Compatibility functions were added to the older APIs to maintain old behaviour.

  • ScriptBridge::GetBridgeID now returns BRIDGE_INVALID for invalid bridge ids, which is still -1.
    Instead of returning 4 294 967 295 after the conversion fix, I created a BridgeID enum containing BRIDGE_INVALID = -1. Since now there's ::BridgeID and ScriptBridge::BridgeID, parts of the code were modified accordingly to use the correct type around.
    No actual compatibility functions are necessary.

  • ScriptGroup::GetCurrentUsage could not return -1
    It shared the same fate as the previous one. Since it was uint, after the conversion fix, returning -1 would mean 4 294 967 295, so i turned the function as a int to solve the issue.
    No actual compatibility functions are necessary.

  • ScriptAirport::GetMaintenanceCostFactor returns -1 for unavailable airport information instead of 65535
    When airport information was unavailable, it was erroneously returning INVALID_TOWN (65535). I decided -1 was an appropriate value to return, because I wanted to diferentiate it from the only positive values that cost factors could return.
    Compatibility functions were added to the older APIs to maintain old behaviour.

  • ScriptRail::GetMaintenanceCostFactor returns -1 for unavailable rail types instead of 0
    I decided -1 was an appropriate value to return, because I wanted to diferentiate it from the only positive values that cost factors could return.
    Compatibility functions were added to the older APIs to maintain old behaviour.

  • ScriptRoad::GetMaintenanceCostFactor returns -1 for unavailable road types instead of 0
    I decided -1 was an appropriate value to return, because I wanted to diferentiate it from the only positive values that cost factors could return.
    Compatibility functions were added to the older APIs to maintain old behaviour.

  • ScriptRoad::GetMaxSpeed now correctly returns -1 for invalid road types instead of 0
    I decided -1 was an appropriate value to return, because 0 is used for unlimited speed.
    Compatibility functions were added to the older APIs to maintain old behaviour.

  • ScriptEngine::GetMaximumOrderDistance returns -1 for invalid engines instead of 0
    I decided -1 was an appropriate value to return, because 0 is used for unlimited distance.
    Compatibility functions were added to the older APIs to maintain old behaviour.

  • ScriptVehicle::GetMaximumOrderDistance returns -1 for invalid vehicles instead of 0
    I decided -1 was an appropriate value to return, because 0 is used for unlimited distance.
    Compatibility functions were added to the older APIs to maintain old behaviour.

  • ScriptTown::GetCargoGoal returns -1 for invalid towns or invalid town effects instead of UINT32_MAX
    I decided -1 was an appropriate value to return, because UINT32_MAX could be misinterpreted as an actual goal requirement.
    Compatibility functions were added to the older APIs to maintain old behaviour.

  • ScriptOrder::GetOrderDistance now tests the validity of the vehicle type, returning -1 for invalid vehicle types
    There was no check in place to test the vehicle type. Testing for invalid tiles was attempting to return -1 (which would become 4 294 967 295 as this function was returning uint)
    Compatibility functions were added to the older APIs to maintain old behaviour.

Limitations

I could have missed some API functions that disguisely return uint32's. If you can spot some, please alert me.

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')

@2TallTyler 2TallTyler added the needs review: Script API Review requested from GS/AI expert label Jan 22, 2023
@SamuXarick SamuXarick force-pushed the script-random-values branch 2 times, most recently from c7d044c to 6dc9814 Compare February 2, 2023 14:17
@SamuXarick
Copy link
Contributor Author

SamuXarick commented Feb 10, 2023

This PR lost its purpose now, with #10443 in master. It was tackling the problem in another direction.
There are still some salvageable parts, the bug fixes in some other functions. Not sure what to do about them now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs review: Script API Review requested from GS/AI expert
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants