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: GetCurveSpeedLimit needs railtype from current tile #8466

Merged

Conversation

@andythenorth
Copy link
Contributor

@andythenorth andythenorth commented Dec 30, 2020

Motivation / Problem

This is a fix to make railtype prop 11 work as described in the newgrf spec https://newgrf-specs.tt-wiki.net/wiki/Action0/Railtypes#Curve_Speed_advantage_multiplier_.2811.29

"This property sets the multiplier to the curve speed advantage which all trains running on this track type get."

GetCurveSpeedLimit was using the railtype from train prop 05, not the current railtype on the tile. Seems plausible that it was just missed when NewGRF railtypes were added.

I noticed this (some time ago) when I created a railtype grf setting a higher value for prop 11 and could see no difference in game.

Additionally for Jan/Feb 2021 there is a tt-forums contest running for 'make a High Speed grf' which motivated me to look into this again, as contestants may encounter this bug.

Description

GetCurveSpeedLimit now checks current railtype on the tile.

Test grf and savegame attached. Behaves as expected in the test case.

8466-railtype-prop-11-test-case.zip

Limitations

I tried a test case where the railtype changes mid-curve. It's not in the savegame, but it does appear to work.

GetAccelerationType in train.h (railtype prop 15) may have the same issue, but the NewGRF spec is less definitive there and is likely to trigger a debate, so I would like to stick to just fixing prop 11 as a single issue.

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 affects the save game format? (label 'savegame upgrade')
    • not as I understand it, although it will affect behaviour of any games with grfs that implement prop 11 (they will start working correctly which might mess with network performance in complex networks)
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • not as I understand it
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')
    • yes, but as it's a bug fix to match NewGRF spec this doesn't need any changes. NMLC and GRFCodec already support railtype prop 11 correctly
@Eddi-z
Copy link
Contributor

@Eddi-z Eddi-z commented Dec 30, 2020

i haven't spent a great deal of time on looking at the implementation, but i support this change conceptually, as the spec pretty clearly state that this is what it meant.

GetAccelerationType in train.h (railtype prop 15) may have the same issue, but the NewGRF spec is less definitive there and is likely to trigger a debate, so I would like to stick to just fixing prop 11 as a single issue.

i'm not taking any sides here, but if we are looking at possibly supporting stuff like "rack rail", or "cable car", this would need both the engine and the tracks to be providing the new acceleration model. although you could just tell people that non-rack rail engines should be defined as not powered on rack rails, then it would fall back to just the rails defining acceleration model.

@michicc
Copy link
Member

@michicc michicc commented Dec 30, 2020

Prop 15 is indeed a bit unclear. First of all, there is literally only one singular use of that in OTTD. If that prop is set to 2, "maglev" is assumed and the acceleration calculation will ignore rolling resistance.

Right now it functions more like an engine property. I don't know if it would be better to make it a true railtype property. I also wonder whether there is any railtype GRF at all that uses prop 15 purposefully and not just by accident.

@michicc
michicc approved these changes Jan 3, 2021
Copy link
Member

@michicc michicc left a comment

Other props can be handled in a separate PR if needed.

@michicc michicc merged commit 0f91cb0 into OpenTTD:master Jan 3, 2021
11 checks passed
11 checks passed
Check for preview label
Details
Check for preview label
Details
Check for preview label
Details
Check for preview label
Details
Emscripten
Details
Commit checker
Details
Linux (clang, clang++)
Details
Linux (gcc, g++)
Details
Mac OS (x64, x86_64) Mac OS (x64, x86_64)
Details
Windows (x86) Windows (x86)
Details
Windows (x64) Windows (x64)
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants