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

Autoreplace does not report failures due to refittability #8153

Closed
giordy opened this issue May 17, 2020 · 6 comments · Fixed by #8169
Closed

Autoreplace does not report failures due to refittability #8153

giordy opened this issue May 17, 2020 · 6 comments · Fixed by #8169
Labels
component: interface This is an interface issue good first issue Good for newcomers
Milestone

Comments

@giordy
Copy link

giordy commented May 17, 2020

Version of OpenTTD

1.10.1

Expected result

When I'm setting some vehicles to be replaced with some policy (either immediately or when they're old) I expect them to be replaced

Actual result

Sometimes it happens that some vehicles get "stuck" indefinitely and never get replaced.

Steps to reproduce

  1. Load the save game attached: bugboats.sav.zip
  2. The first company has some "Clipper" ships that are 38 years old. They are set to be replaced immediately but they never get replaced.
@giordy
Copy link
Author

giordy commented May 17, 2020

Some of them go to depot to refit and other ones pass close to the depots so they should have chances to enter for maintenance.

I also tried to send them manually but they never get replaced. Dozens of other ships (even on the same route I think) got replaced correctly but these ones are stuck somehow.

It happened as well in another game to a friend of mine, this time with road vehicles.

@nielsmh
Copy link
Contributor

nielsmh commented May 17, 2020

I traced through the code, it turns out the autoreplace fails (silently) because the replacement vehicle can not be refitted to the correct cargo types required for the vehicle's current cargo or any refit order the vehicle has.
For example, company 1 ship 3 has an order to refit to passengers (order 7), but the replacement vehicle (Little Cumbrae Freighter) can not carry passengers.

The bug here isn't that the replacement fails, but that you don't get any feedback why it fails.

@nielsmh nielsmh added component: interface This is an interface issue good first issue Good for newcomers labels May 17, 2020
@nielsmh nielsmh changed the title Sometimes vehicles don't get replaced Autoreplace does not report failures due to refittability May 17, 2020
@James103
Copy link
Contributor

So the message would be something like "Autorenew failed on Ship 3 ... Could not refit vehicle" plus an optional "to all cargoes" in the case of refit orders?

@nielsmh
Copy link
Contributor

nielsmh commented May 17, 2020

There's two cases to report really. Ideas for error messages:

  • New vehicle can not carry {cargo} (currently carried cargo)
  • New vehicle can not carry {cargo} as required by order {index} (refit check for orders)

A bonus feature would be to check all orders for refit-compatibility instead of bailing on first failure.

This is where the check happens:

/* Does it need to be refitted */
CargoID refit_cargo = GetNewCargoTypeForReplace(old_veh, e, part_of_chain);
if (refit_cargo == CT_INVALID) return CommandCost(); // incompatible cargoes

@Chr12t0pher
Copy link
Contributor

Happy to try and tackle this one! I've added a string and put it into CommandCost so it displays a failure message:

image

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

@glx22
Copy link
Contributor

glx22 commented May 25, 2020

Feel free to open a PR @Chr12t0pher, will be easier to discuss with the code

Chr12t0pher added a commit to Chr12t0pher/OpenTTD that referenced this issue May 25, 2020
Chr12t0pher added a commit to Chr12t0pher/OpenTTD that referenced this issue May 26, 2020
Chr12t0pher added a commit to Chr12t0pher/OpenTTD that referenced this issue May 26, 2020
Chr12t0pher added a commit to Chr12t0pher/OpenTTD that referenced this issue May 26, 2020
Chr12t0pher added a commit to Chr12t0pher/OpenTTD that referenced this issue May 26, 2020
@TrueBrain TrueBrain added this to the 1.11.0 milestone Jan 8, 2021
TrueBrain pushed a commit to Chr12t0pher/OpenTTD that referenced this issue Jan 8, 2021
TrueBrain pushed a commit to Chr12t0pher/OpenTTD that referenced this issue Jan 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: interface This is an interface issue good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants