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

Feature: Add cargo filter support to vehicle list. #8308

Merged
merged 1 commit into from Nov 8, 2022

Conversation

stormcone
Copy link
Contributor

@stormcone stormcone force-pushed the cargo-filter-vehicle-list branch 2 times, most recently from d7ac3fa to 9847f67 Compare September 25, 2020 14:26
@TrueBrain TrueBrain added candidate: needs considering This Pull Request needs more opinions size: large This Pull Request is large in size; review will take a while labels Dec 14, 2020
@andythenorth
Copy link
Contributor

Hi Stormcone. Thanks for this! :)

Would it be possible to add a brief description covering the list below?

  • 'intent' for the PR
  • edge cases considered / handled / known issues
  • tips for testing (if relevant, or none)
  • screenshot(s)

This just really aids reviewing, and we would like to make reviewing faster and frequent. We have lots of nice contributions stuck and we would like to change that. :)

@stormcone
Copy link
Contributor Author

For example if you have trains that can transport multiple cargoes, and want to replace one kind of wagon to another one which can carry a specific cargo, then you can filter the trains which can carry that one cargo. A more detailed example, in the early game years you have only box cars to carry everything, but with time goes, a grain hopper getting available, so you want to use it to break the monotonicity of appearance of your trains, or the new wagon can carry more cargo, or can be hauled faster. So it makes easier to find the right trains to replace the wagons.

Or you can check on a station or on a waypoint whether a specific cargo goes through it or not.

openttd_cargo_filter

@andythenorth
Copy link
Contributor

andythenorth commented Dec 17, 2020

Thanks.

So currently I have to use 'Sort by' and 'Total capacity by cargo type' for the examples you give.

This new filter seems useful.

Does it have any weird interactions with 'refit at station' [to cargo] or [to any available]?

@stormcone
Copy link
Contributor Author

Sorry, I did not see your reply yesterday.

This patch collects the cargoes by the current state of what the vehicles can carry. So for example if a wagon currently can carry steel, then with filter set to steel the train will appear in the list. But if the train reaches a station where the wagon is refitted to carry goods, then it will disappear from the list. There is no difference whether the refit is explicit to a specific cargo or to any available.

So I would not say that is weird interaction, but indeed it is not ideal in the case of refit orders.

@frosch123 frosch123 added the preview This PR is receiving preview builds label Dec 22, 2020
@DorpsGek DorpsGek temporarily deployed to preview-pr-8308 December 22, 2020 21:50 Inactive
@2TallTyler
Copy link
Member

Needs a rebase. 🙂

I don't see an easy way to find vehicles which carry the selected cargo at some point in their journey, but are currently refitted to something else. It would be possible to loop through every order looking for a refit to the selected cargo, but that sounds expensive, and wouldn't work for vehicles with "refit to available cargo."

The fact that it wouldn't find each individual vehicle might not be a big a problem since I imagine most players doing refits also use shared orders, and most shared-order vehicle groups would probably have at least one vehicle currently on the leg of its journey where it carries the selected cargo. I'm okay with this corner case but would appreciate more opinions.

@DorpsGek DorpsGek temporarily deployed to preview-pr-8308 October 24, 2022 21:34 Inactive
@DorpsGek DorpsGek temporarily deployed to preview-pr-8308 October 24, 2022 21:44 Inactive
Copy link
Member

@2TallTyler 2TallTyler left a comment

Choose a reason for hiding this comment

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

Code looks good to me. Just a few nitpicks.

src/vehicle_gui.cpp Outdated Show resolved Hide resolved
src/vehicle_gui.cpp Outdated Show resolved Hide resolved
src/vehicle_gui_base.h Show resolved Hide resolved
src/widgets/group_widget.h Outdated Show resolved Hide resolved
@DorpsGek DorpsGek temporarily deployed to preview-pr-8308 October 24, 2022 23:59 Inactive
@2TallTyler 2TallTyler added this to the 13.0 milestone Oct 27, 2022
@michicc michicc merged commit 0d303d6 into OpenTTD:master Nov 8, 2022
@stormcone stormcone deleted the cargo-filter-vehicle-list branch November 8, 2022 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
candidate: needs considering This Pull Request needs more opinions preview This PR is receiving preview builds size: large This Pull Request is large in size; review will take a while
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants