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

Possible fix for #7430: when train visits station, only reset time_since_pickup if has room to load #7595

Merged
merged 1 commit into from Dec 23, 2019

Conversation

@MingweiSamuel
Copy link
Contributor

MingweiSamuel commented May 16, 2019

@MingweiSamuel MingweiSamuel force-pushed the MingweiSamuel:patch-1 branch 2 times, most recently from 27dc663 to f3e4a7e May 18, 2019
@LordAro LordAro requested a review from PeterN Jun 6, 2019
@LordAro
Copy link
Member

LordAro commented Oct 25, 2019

For me, it makes sense to only reset time_since_pickup when cargo is actually loaded, not before. If a train with spare capacity for some cargo comes into a station, but for some reason doesn't pick any up, the cargo is still "old" and the pickup time shouldn't be reset. It's time since cargo pick up, not time since last (nonfull) train arrival

@nielsmh
Copy link
Contributor

nielsmh commented Oct 25, 2019

The time since load needs to be reset even when there is no cargo available to load on the station, since that value is used for station rating calculation. If a player that doesn't receive any cargo on their station due to low rating can't raise their rating by having vehicles attempt to load, they will never be able to compete.

I haven't looked at the code in detail, but the idea presented sounds reasonable.

@MingweiSamuel MingweiSamuel force-pushed the MingweiSamuel:patch-1 branch from fd92707 to 55c2a5c Oct 29, 2019
@MingweiSamuel
Copy link
Contributor Author

MingweiSamuel commented Oct 29, 2019

I've split the PR into two commits:

  • The first one contains the functional changes; making if (cap_left > 0) { its own branch and only setting ge->time_since_pickup = 0; in there. Note the wrong indentation.
  • And the second one just fixes the indentation (which produces a big diff)

Should be a little easier to review (edit: though it will fail the commit checker)

Copy link
Member

LordAro left a comment

Seems fine to me (does need commits merging though)

@LordAro LordAro added this to the 1.10.0 milestone Nov 23, 2019
… has room to load
@MingweiSamuel MingweiSamuel force-pushed the MingweiSamuel:patch-1 branch from 55c2a5c to a85673f Nov 30, 2019
@MingweiSamuel
Copy link
Contributor Author

MingweiSamuel commented Nov 30, 2019

Alright, squashed, looks like github automatically dismissed the review on force push unfortunately. I'm not too familiar with gh's system.

Copy of old commit without indentation: MingweiSamuel@d0c1353

@nielsmh nielsmh merged commit 26ce4eb into OpenTTD:master Dec 23, 2019
8 checks passed
8 checks passed
OpenTTD CI #20191130.1 succeeded
Details
OpenTTD CI (Linux commit-checker) Linux commit-checker succeeded
Details
OpenTTD CI (Linux linux-amd64-clang-3.9) Linux linux-amd64-clang-3.9 succeeded
Details
OpenTTD CI (Linux linux-amd64-gcc-6) Linux linux-amd64-gcc-6 succeeded
Details
OpenTTD CI (Linux linux-i386-gcc-6) Linux linux-i386-gcc-6 succeeded
Details
OpenTTD CI (MacOS) MacOS succeeded
Details
OpenTTD CI (Windows Win32) Windows Win32 succeeded
Details
OpenTTD CI (Windows Win64) Windows Win64 succeeded
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

3 participants
You can’t perform that action at this time.