Copy wagon configuration to all trains in shared orders/group. #10476

serg-bloim · 2023-02-13T18:36:19Z

This is a draft. No code-review required yet. I just wanted to initiate a discussion first.

Motivation / Problem

We have a great feature called auto-replacement and it works really great for engines and saves time. But there is a similar process that players have to do manually. It is to update their trains with more/new/other train cars(wagons). It is no fun cause it's a tedious process , it takes time to watch all the trains in a group and plus it messes up with the timetables if played without PAUSE.

Description

I know there is a patch Template Based Train Replacement. But I play vanila and don't know the status of that patch, although the idea looks really cool.

How I see my solution? I want it to be simple and interfere with the existing code/logic as little as possible.
I see it as a button somewhere at the vehicle group window, so player can to click the button and then select some train as a target(similar to clone vehicle). When selected, all vehicles in the group are marked as to be refitted(not really, but it's the closest existing term).

Later on, when a marked vehicle enters a depot, it gets refitted. Which means, it will compare its wagons to the ones of the target and fix the difference. Once done, the vehicle clears its status and doesn't need to do the procedure until again explicitly marked by player.

Limitations

This functionality is only applicable to trains as I don't know of any other transports with non-engine units.

As I'm very new to OpenTTD, it would be great if I can get help with the following questions. Just keep in mind, I didn't even play any non-default NewGRFs.

Is it only trains can have attachable parts like wagons?
Is it possible for a train wagon to be refit-able?
What is the relation between cargo_type and cargo_subtype? Should I consider different subtypes as different cargos and replace the wagon if it is a different subtype?
Please suggest a good naming for the feature. The problem is that there are already 2 related but different processes: autoreplace and refit. My best candidate is Copy (wagons|train cars) from. But I'm open to proposals.
Are there any game mechanics (maybe enabled in NewGFRs) that may interfere with my feature?

Thanks

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 touches english.txt or translations? Check the guidelines
This PR affects the save game format? (label 'savegame upgrade')
This PR affects the GS/AI API? (label 'needs review: Script API')
- ai_changelog.hpp, gs_changelog.hpp need updating.
- The compatibility wrappers (compat_*.nut) need updating.
This PR affects the NewGRF API? (label 'needs review: NewGRF')
- newgrf_debug_data.h may need updating.
- PR must be added to API tracker

krysclarke · 2023-02-14T02:45:38Z

How would this be different to the functionality that already exists in the "Replace vehicles"?
In the top-centre of the "Replace Train" window, there's a drop down that lets you view either "Engines" or "Wagons".

src/vehicle.cpp

src/vehicle_gui.cpp

serg-bloim · 2023-02-14T05:38:29Z

How would this be different to the functionality that already exists in the "Replace vehicles"? In the top-centre of the "Replace Train" window, there's a drop down that lets you view either "Engines" or "Wagons".

So the scenario for my feature is like that:
You have a route between 2 towns with N trains on it. Trains carry 5 passenger cars and 1 mail car each. Over time you realize you want to remove 1 passenger car and add 3 mail cars. With replace wagons you cannot do that, you cannot even replace all passenger cars with mail.

What I propose, you pick 1 train, get it to depot and do whatever needs to be done to its wagons: add, remove, reorder. Then open some train group, either shared orders->all trains sharing orders or a vehicle group containing all trains you want to update. Select action Manage list->Copy cars from train, select the train(example) with the desired config and then all the vehicles from this group change accordingly to the example when visiting a depot next time.

src/vehicle.cpp

src/vehicle_cmd.cpp

andythenorth · 2023-02-20T07:50:58Z

This is an interesting alternative to defining train templates (which there is an example patch for in JGRPP). Templates are quite complex and require complicated new UI.

This looks more lightweight.

A suitable name might be 'match', but I'm not sure.

Possibilities from the thesaurus: 'replicate', 'facsimile', 'correspond', 'harmonise', 'conform'.

src/vehicle.cpp

src/vehicle_cmd.cpp

serg-bloim · 2023-02-24T02:23:27Z

CMakeLists.txt

@@ -340,6 +340,8 @@ if(EMSCRIPTEN)
    # Build the .html (which builds the .js, .wasm, and .data too).
    set_target_properties(openttd PROPERTIES SUFFIX ".html")
    target_link_libraries(openttd WASM::WASM)
+    file(COPY ${CMAKE_SOURCE_DIR}/os/emscripten/demo.sav


Don't mind this line. I wanted to include a demo save file into the browser version of the game. So ppl can try the functionality if any question.
Before merging I will revert this change.

It applies to files CMakeLists.txt and os/emscripten/demo.sav

serg-bloim · 2023-02-24T02:39:34Z

This is an interesting alternative to defining train templates (which there is an example patch for in JGRPP). Templates are quite complex and require complicated new UI.

This looks more lightweight.

A suitable name might be 'match', but I'm not sure.

Possibilities from the thesaurus: 'replicate', 'facsimile', 'correspond', 'harmonise', 'conform'.

Thanks for the feedback. In my latest version I use these for button label: Copy wagons from | Stop copying wagons.
From your variants I probably like replicate more cause others are somewhat niche words and match sounds to me more like a visual comparison, analysis, rather than some physical work to do. The main idea for the label is to be concise and the shorter the better.
So currently the 3 top variants for me are:

1. Copy wagons from | Stop copying wagons
2. Replicate wagons | Stop replication
3. Match wagons     | Sop matching wagons

I'm still hesitant, so feel free to point which one you like more or propose if you have another variant.

merni-ns · 2023-02-24T11:03:00Z

Is it only trains can have attachable parts like wagons?

Yes. Road vehicles, trams, ships and aircrafts cannot have attachable wagons (though the first two can be articulated units).

Is it possible for a train wagon to be refit-able?

Yes.

What is the relation between cargo_type and cargo_subtype?

Cargo types are like passenger, mail, goods, coal, etc. Subtypes determine different graphics (and maybe capacity and other properties) to be used for what the game regards as one cargo type. There are two main applications for them in NewGRFs (which I'm aware of):

They are used to distinguish different kinds of real-life cargo, without them being implemented as different cargo types. For example, a "Goods" wagon might be shown as carrying cars, or shipping containers, or anything else
They are used to implement liveries. For example, a passenger carriage might have a red livery, green livery, blue livery etc. Or in case of NewGRFs implementing real-life trains, the liveries might be different liveries used historically, or ones used by various railway operators, or for various kinds of services, etc. Different cargo subtypes may have different capacities and loading speeds for a vehicle, for example a passenger coach may be a second class (with more seats), first class (with less seats), or a sleeping car (with very few seats and slow loading), which may be implemented with subtypes. Even though the new "variants" feature in 13.0 is a better way to implement liveries, there are -- and will continue to be -- still plenty of NewGRFs which use subtypes for the purpose.

Are there any game mechanics (maybe enabled in NewGFRs) that may interfere with my feature?

You should try playing with various vehicle NewGRFs to check how your feature works with them.

andythenorth · 2023-02-25T08:36:48Z

Are there any game mechanics (maybe enabled in NewGFRs) that may interfere with my feature?

Wagon attach callback https://newgrf-specs.tt-wiki.net/wiki/Callbacks#Can_wagon_be_attached_.281D.29
Checks like 'train cannot start unless it has a caboose'.

Sorry, I can't give a list of grfs which use these features.

My opinion is that they're not features grfs should be using, but fortunately grf authors have the freedom to ignore me :)

serg-bloim · 2023-02-25T17:08:07Z

Are there any game mechanics (maybe enabled in NewGFRs) that may interfere with my feature?

Wagon attach callback https://newgrf-specs.tt-wiki.net/wiki/Callbacks#Can_wagon_be_attached_.281D.29

Checks like 'train cannot start unless it has a caboose'.

Sorry, I can't give a list of grfs which use these features.

My opinion is that they're not features grfs should be using, but fortunately grf authors have the freedom to ignore me :)

Thanks! I just read a bunch of wikis about the callbacks trying to understand how they work and I feel like a grandma with a smartphone =) Just curious, is it possible to put a whatever logic like compiled or scripted code into GRFs or it's more like some predefined actions that you may configure?

But anyway my feature is using just some standard commands like buy, sell, refit and move vehicles, so all the callbacks should be covered and if anything, it should handle everything gracefully, maximum popping an error. Plus the feature requires a functioning target that user creates by hand, so it should be at least structurally correct. The only situation I can image is copying a vehicle after its end-of-life. But I'm not sure if end-of-life applies to wagons at all, I only saw it for engines. And again if any encountered, it must be just another normal game error.

serg-bloim · 2023-02-25T17:09:54Z

I think the PR is ready for review.

src/vehicle_cmd.cpp

serg-bloim · 2023-02-26T16:03:36Z

Hi @2TallTyler How do I proceed with the PR to get reviewed and merged?

Initially it was a draft. But couple days ago as I finished coding I turned it into a normal PR to indicate it is ready for review. I'm not sure what is the process, do I need to somehow signal about it, so devs take a look?
Thanks.

LordAro

a load of nitpicky codestyle things to start with :)

LordAro · 2023-02-26T19:32:03Z

src/vehicle.cpp

+ * Sets a target for this vehicle to copy wagons from
+ * @param src - source vehicle to copy wagons from
+ */
+void Vehicle::CopyWagonsFrom(VehicleID src) {


opening brace goes on its own line

LordAro · 2023-02-26T19:32:22Z

src/vehicle.cpp

-		StringID error_message = res.GetErrorMessage();
-		if (error_message == STR_ERROR_AUTOREPLACE_NOTHING_TO_DO || error_message == INVALID_STRING_ID) continue;
+		// Copy wagons from another train
+		if (v->type == VehicleType::VEH_TRAIN && v->copy_wagons_from != v->index){


missing space (various other places too)

Suggested change

if (v->type == VehicleType::VEH_TRAIN && v->copy_wagons_from != v->index){

if (v->type == VehicleType::VEH_TRAIN && v->copy_wagons_from != v->index) {

LordAro · 2023-02-26T19:32:40Z

src/vehicle_cmd.cpp

+
+	if (example->vehstatus & VS_CRASHED) return_cmd_error(STR_ERROR_VEHICLE_IS_DESTROYED);
+
+	for (const Vehicle *v: vl) {


Suggested change

for (const Vehicle *v: vl) {

for (const Vehicle *v : vl) {

LordAro · 2023-02-26T19:33:16Z

src/vehicle_cmd.cpp

+					}
+				}
+				if (replacement == nullptr) {
+					// we didn't find enough matching wagons, we have to buy a new one.


multi line comments, or comments on their own line should use /* */

LordAro · 2023-02-26T19:33:31Z

src/vehicle_cmd.cpp

+				assert(d_prev != nullptr);
+			}
+		}
+		//When we finish copying all the source wagon units, it needs to sell the leftovers if any.


missing space

LordAro · 2023-02-26T19:33:58Z

src/vehicle_gui_base.h

@@ -154,6 +159,14 @@ struct BaseVehicleListWindow : public Window {
 				NOT_REACHED();
 		}
 	}
+
+	bool OnVehicleSelect(const struct Vehicle *v) override {


unneeded struct

LordAro · 2023-02-26T19:35:29Z

src/vehicle_cmd.cpp

+ * Performs copy wagons operation for a train.
+ * @param flags type of operation
+ * @param train train to copy wagons to from its target (train->copy_wagons_from)
+ * @return


missing doc :)

serg-bloim · 2023-02-26T20:47:17Z

a load of nitpicky codestyle things to start with :)

@LordAro
I'm sorry you have to go through so many silly code style things in my code. C++ is not really my occupation, so I'm not used to its code style.

Is there a linter or extension fo clion/visual studio code/anything else that can help me avoid all these code-style things that I miss? Smth that I can run on mac.

LordAro · 2023-02-26T20:49:32Z

Practically speaking, no, I'm afraid not - we've experimented with various things in the past but none of them really work well with OTTD's style (particularly when it purposefully deviates from it)

serg-bloim · 2023-02-26T21:01:33Z

Ok, then I'll manually go through your comments and similar places in the change.
That's a huge motivation to make changes small =)

serg-bloim · 2023-03-04T04:06:16Z

@LordAro I decided to add some code changes into the PR and thus have a question regarding command flags and DC_EXEC.
I can see that each command runs in two modes: DC_NONE and then DC_EXEC. I assume first it runs in test mode and checks there's no errors, enough money and if all good, then runs with DC_EXEC and applies the change.

In my algorithm I perform these kinds of operations with vehicles(wagons): build, refit, move, sell.
There are two possible approaches how to implement a test run of the algo (flags == DC_NONE):

In the very beginning of the algorithm I know how many buy, refit and sell operations I will have to do. So I can simply run them with DC_NONE and return the combined command cost. Pros: very easy implementation. Cons: I do not run vehicle move operations. Thus wagon movements are not tested. But since we replicate an existing train, I don't think it can go wrong.
Regardless of flags, I run the algorithm the same way, buying, selling and refitting the wagons as I need. But if DC_EXEC is not set, then in the end I have to revert everything back to how it was before, i.e sell, buy and refit back. Also I'll have to keep track of every op. Pros: it will follow all the operations as they are in the main run. Cons: makes the implementation more complex, issues many commands and creates many objects that will be shortly discarded.

If I would decide by myself, I'd go with approach#1 as I think it will be a simpler and cleaner code. But can you suggest is there a recommended way of implementing command's behavior when flags == DC_NONE?

2TallTyler added the preview This PR is receiving preview builds label Feb 13, 2023

DorpsGek temporarily deployed to preview-pr-10476 February 13, 2023 19:13 Inactive

DorpsGek temporarily deployed to preview-pr-10476 February 14, 2023 02:56 Inactive

github-advanced-security bot found potential problems Feb 14, 2023

View reviewed changes

src/vehicle.cpp Fixed Show fixed Hide fixed

src/vehicle_gui.cpp Fixed Show fixed Hide fixed

serg-bloim force-pushed the feature/copy-wagon-configuration branch from aac7b82 to b3a765c Compare February 14, 2023 20:03

DorpsGek temporarily deployed to preview-pr-10476 February 14, 2023 20:03 Inactive

serg-bloim force-pushed the feature/copy-wagon-configuration branch from b3a765c to 3616668 Compare February 20, 2023 07:04

DorpsGek temporarily deployed to preview-pr-10476 February 20, 2023 07:04 Inactive

github-advanced-security bot found potential problems Feb 20, 2023

View reviewed changes

serg-bloim force-pushed the feature/copy-wagon-configuration branch from 3616668 to ae3b46d Compare February 20, 2023 20:36

DorpsGek temporarily deployed to preview-pr-10476 February 20, 2023 20:36 Inactive

serg-bloim force-pushed the feature/copy-wagon-configuration branch from ae3b46d to 87cc454 Compare February 20, 2023 20:55

DorpsGek temporarily deployed to preview-pr-10476 February 20, 2023 20:55 Inactive

github-advanced-security bot found potential problems Feb 20, 2023

View reviewed changes

serg-bloim force-pushed the feature/copy-wagon-configuration branch from 87cc454 to a1e606f Compare February 24, 2023 01:42

DorpsGek temporarily deployed to preview-pr-10476 February 24, 2023 01:42 Inactive

github-advanced-security bot found potential problems Feb 24, 2023

View reviewed changes

DorpsGek temporarily deployed to preview-pr-10476 February 24, 2023 02:16 Inactive

serg-bloim commented Feb 24, 2023

View reviewed changes

DorpsGek temporarily deployed to preview-pr-10476 February 24, 2023 03:01 Inactive

DorpsGek temporarily deployed to preview-pr-10476 February 24, 2023 03:47 Inactive

DorpsGek temporarily deployed to preview-pr-10476 February 24, 2023 04:23 Inactive

serg-bloim force-pushed the feature/copy-wagon-configuration branch from 6927283 to a1e606f Compare February 24, 2023 06:37

DorpsGek temporarily deployed to preview-pr-10476 February 24, 2023 06:37 Inactive

serg-bloim force-pushed the feature/copy-wagon-configuration branch from a1e606f to c0c424c Compare February 24, 2023 14:35

DorpsGek temporarily deployed to preview-pr-10476 February 24, 2023 14:35 Inactive

serg-bloim force-pushed the feature/copy-wagon-configuration branch from c0c424c to 2b26885 Compare February 25, 2023 06:38

DorpsGek temporarily deployed to preview-pr-10476 February 25, 2023 06:39 Inactive

serg-bloim force-pushed the feature/copy-wagon-configuration branch from 2b26885 to d186ae4 Compare February 25, 2023 08:03

DorpsGek temporarily deployed to preview-pr-10476 February 25, 2023 08:03 Inactive

serg-bloim force-pushed the feature/copy-wagon-configuration branch from d186ae4 to 1d29278 Compare February 25, 2023 16:42

DorpsGek temporarily deployed to preview-pr-10476 February 25, 2023 16:42 Inactive

serg-bloim marked this pull request as ready for review February 25, 2023 17:09

github-advanced-security bot found potential problems Feb 25, 2023

View reviewed changes

src/vehicle_cmd.cpp Fixed Show resolved Hide resolved

src/vehicle_cmd.cpp Fixed Show fixed Hide fixed

src/vehicle_cmd.cpp Fixed Show fixed Hide fixed

src/vehicle_cmd.cpp Fixed Show fixed Hide fixed

src/vehicle_cmd.cpp Fixed Show fixed Hide fixed

src/vehicle_cmd.cpp Fixed Show fixed Hide fixed

LordAro reviewed Feb 26, 2023

View reviewed changes

serg-bloim force-pushed the feature/copy-wagon-configuration branch from 1d29278 to 8c1ade5 Compare March 7, 2023 21:29

DorpsGek temporarily deployed to preview-pr-10476 March 7, 2023 21:29 Inactive

Bilongozhko, Serhii (Contractor) added 2 commits March 7, 2023 18:26

Feature: Allows to copy wagons from another train

de7098a

remove dbg info

5a7c9c6

serg-bloim force-pushed the feature/copy-wagon-configuration branch from 8c1ade5 to 5a7c9c6 Compare March 7, 2023 23:26

DorpsGek temporarily deployed to preview-pr-10476 March 7, 2023 23:26 Inactive

TrueBrain removed the preview This PR is receiving preview builds label Jul 8, 2023

2TallTyler added work: needs more work This Pull Request needs more work before it can be accepted work: needs rebase This Pull Request needs a rebase labels Sep 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Copy wagon configuration to all trains in shared orders/group. #10476

Copy wagon configuration to all trains in shared orders/group. #10476

serg-bloim commented Feb 13, 2023

krysclarke commented Feb 14, 2023

serg-bloim commented Feb 14, 2023

andythenorth commented Feb 20, 2023 •

edited

Loading

serg-bloim Feb 24, 2023

serg-bloim Feb 24, 2023

serg-bloim commented Feb 24, 2023

merni-ns commented Feb 24, 2023 •

edited

Loading

andythenorth commented Feb 25, 2023

serg-bloim commented Feb 25, 2023

serg-bloim commented Feb 25, 2023

serg-bloim commented Feb 26, 2023

LordAro left a comment

LordAro Feb 26, 2023

LordAro Feb 26, 2023

LordAro Feb 26, 2023

LordAro Feb 26, 2023

LordAro Feb 26, 2023

LordAro Feb 26, 2023

LordAro Feb 26, 2023

serg-bloim commented Feb 26, 2023

LordAro commented Feb 26, 2023

serg-bloim commented Feb 26, 2023

serg-bloim commented Mar 4, 2023

	if (v->type == VehicleType::VEH_TRAIN && v->copy_wagons_from != v->index){
	if (v->type == VehicleType::VEH_TRAIN && v->copy_wagons_from != v->index) {


		if (example->vehstatus & VS_CRASHED) return_cmd_error(STR_ERROR_VEHICLE_IS_DESTROYED);

		for (const Vehicle *v: vl) {

Copy wagon configuration to all trains in shared orders/group. #10476

Are you sure you want to change the base?

Copy wagon configuration to all trains in shared orders/group. #10476

Conversation

serg-bloim commented Feb 13, 2023

Motivation / Problem

Description

Limitations

Checklist for review

krysclarke commented Feb 14, 2023

serg-bloim commented Feb 14, 2023

andythenorth commented Feb 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serg-bloim commented Feb 24, 2023

merni-ns commented Feb 24, 2023 • edited Loading

andythenorth commented Feb 25, 2023

serg-bloim commented Feb 25, 2023

serg-bloim commented Feb 25, 2023

serg-bloim commented Feb 26, 2023

LordAro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serg-bloim commented Feb 26, 2023

LordAro commented Feb 26, 2023

serg-bloim commented Feb 26, 2023

serg-bloim commented Mar 4, 2023

andythenorth commented Feb 20, 2023 •

edited

Loading

merni-ns commented Feb 24, 2023 •

edited

Loading