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 #147: rounding issues #150

Merged
merged 2 commits into from Aug 25, 2020
Merged

Fix #147: rounding issues #150

merged 2 commits into from Aug 25, 2020

Conversation

FLHerne
Copy link
Contributor

@FLHerne FLHerne commented May 23, 2020

No description provided.

@FLHerne
Copy link
Contributor Author

FLHerne commented May 23, 2020

@frosch123 -- you made the opposite change to the first commit before merging #75 , so I should probably ask why you think it's wrong. ;-)

@LordAro LordAro requested a review from frosch123 May 23, 2020 12:02
@FLHerne
Copy link
Contributor Author

FLHerne commented May 23, 2020

Hm, now the round() is redundant. I think it's that bit that's wrong.

@FLHerne
Copy link
Contributor Author

FLHerne commented May 23, 2020

Updated -- I still don't really understand what the OpenTTD code does, but now I'm confident the Python version does the same thing...

This is an integer division in OpenTTD, and the floating-point one
 rounds to the wrong result for some inputs.
return (round(value.value / divisor * 10 * unit.ottd_mul) >> unit.ottd_shift) // 16
def ottd_display_speed(value, mul, div, unit):
# Convert value to km/h-ish.
kmh_ish = value.value * mul // div
Copy link
Member

Choose a reason for hiding this comment

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

While this now matches OTTD's GetDisplaySpeed, it throws away the fractional part of the speed.
You can define a road vehicles that has a max speed of 7 units (=3.5 km-ish/h), and the movement code will use that speed. OTTD will round it for display to 3 km-ish/h though.

So, ideally it should be possible to define vehicles in NML with a speeds of 3, 3.5 and 4 km/h, which have different speed in-game, and the 3 and 4 is also shown in the vehicle lists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is only used to clamp the internal value calculated elsewhere, by incrementing/decrementing it until the display speed matches the input. Fractional values that display as the correct integer should be unaffected.

@FLHerne FLHerne merged commit 467f81f into OpenTTD:master Aug 25, 2020
@FLHerne FLHerne deleted the flh-fix-speeds branch April 2, 2021 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants