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

@LordAro LordAro added candidate: yes This Pull Request is a candidate for being merged component: NewGRF This issue is related to NewGRFs needs review: NewGRF Review requested from a NewGRF expert size: trivial This Pull Request is trivial labels Dec 30, 2020
@Eddi-z
Copy link
Contributor

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 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.

Copy link
Member

@michicc michicc left a comment

Choose a reason for hiding this comment

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
candidate: yes This Pull Request is a candidate for being merged component: NewGRF This issue is related to NewGRFs needs review: NewGRF Review requested from a NewGRF expert size: trivial This Pull Request is trivial
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants