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 #8153: Report autoreplace failure when new vehicle cannot carry the cargo #8169

Merged
merged 1 commit into from Jan 8, 2021

Conversation

Chr12t0pher
Copy link
Contributor

@Chr12t0pher Chr12t0pher commented May 25, 2020

Fixes #8153

For more information we'll have to modify the variables that are passed to the string template, I can see this is done here:

OpenTTD/src/vehicle.cpp

Lines 1058 to 1067 in c972a63

StringID message;
if (error_message == STR_ERROR_TRAIN_TOO_LONG_AFTER_REPLACEMENT) {
message = error_message;
} else {
message = STR_NEWS_VEHICLE_AUTORENEW_FAILED;
}
SetDParam(0, v->index);
SetDParam(1, error_message);
AddVehicleAdviceNewsItem(message, v->index);

Should I be adding some extra logic here to set a third parameter (cargo type) if the error message is my new one? Seems like there's a couple of conditionals for building the message depending on the error here already.

Made a barebones save for easier testing too - bugboats.zip

@Chr12t0pher Chr12t0pher marked this pull request as draft May 25, 2020 21:08
@nielsmh
Copy link
Contributor

nielsmh commented May 25, 2020

You should definitely try to include as much useful information to the player as possible:

  • Tell the player which specific cargo type was not supported
  • If the failure is due to orders requiring an impossible refit, tell the player which number order is the offender

@Chr12t0pher Chr12t0pher force-pushed the incompatible-cargo-renew branch 3 times, most recently from 81d6329 to 26e34e2 Compare May 26, 2020 11:25
@TrueBrain TrueBrain added candidate: yes This Pull Request is a candidate for being merged size: small This Pull Request is small, and should be relative easy to process labels Dec 14, 2020
@TrueBrain
Copy link
Member

Hello @Chr12t0pher ; it has been a while since you opened this draft Pull Request. Is it ready for review? Or were you still working on this? :)

If it was ready, please do rebase it and hit the Ready for review button. If you are no longer interested in this, also no problem, but please let us know so we can clean our Pull Request list a bit :)

Thank you!

@TrueBrain TrueBrain added the work: minor details This Pull Request has some minor details left to do label Dec 21, 2020
@TrueBrain TrueBrain added this to the 1.11.0 milestone Jan 5, 2021
@TrueBrain
Copy link
Member

I rebased your work and made some minor coding-style fixes, nothing crazy :)

@TrueBrain TrueBrain removed the work: minor details This Pull Request has some minor details left to do label Jan 8, 2021
@TrueBrain TrueBrain marked this pull request as ready for review January 8, 2021 11:19
Copy link
Member

@LordAro LordAro left a comment

Choose a reason for hiding this comment

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

LGTM

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 size: small This Pull Request is small, and should be relative easy to process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Autoreplace does not report failures due to refittability
4 participants