Refit at purchase time when filtering by cargo type

[Dewin]

I actually was going to try my hand at writing a patch for this but -- since I'm giving up for the time being (I don't know my way around the code yet), I figured I'd post it as a suggestion.

It would be nice if there was an option to refit vehicles as they are purchased when filtering by a specific type of cargo -- e.g. if I'm filtering by "iron ore", a wagon that carries coal by default but can be refitted to carry iron ore will be purchased in the already-refitted-to-carry-iron-ore state.

There's a few vehicles this may not work well with (The long trams refittable to have 3, 6 or 9 wagons in one of the GRFs come to mind), but I figure it would still be an improvement overall. Thoughts?

[hgnmu128]

If you could do that, what would be the point in refitting? Apart from vehicles that refit once or twice every round trip...


EDIT: /me = What an idiot.

[planetmaker]

If you could do that, what would be the point in refitting? Apart from vehicles that refit once or twice every round trip...

Having nevertheless less vehicles in the purchase screen. And as vehicle set author less trouble to cater for all possible cargos.

[Dewin]

I'm thinking particularly in cases of GRFs like FIRS, in conjunction with URKS where there's a multitude of wagons and engines to sort through.

Right now, my process is:
1. Build engine.
2. Filter by cargo type to remember which of the two dozen wagon options or so holds the cargo I want
3. Build some wagons
4. Repeat steps 2&3 for each different cargo type. (I like to have my primary supply (e.g. ore, coal, etc. trains pick up a wagon or two of engineering supplies for their return trip.)
5. Switch to the train's window.
6. Click Refit
7. Scroll through the (long-ish) list of cargo types until I find the correct one
8. Repeat steps 6 & 7 for each cargo type, with additional funsies if I'm carrying multiple cargoes that the same wagon can be refitted to. (I'm using Chill's Patchpack, so I can at least do Split Refit)

If vehicles could be pre-refitted at creation type, it eliminates steps 5 thru 8, and also makes it easier to construct trains that use multiple cargo types that are assigned to the same wagon (by building half of the wagons, changing the filter to the other cargo type, and then building the rest). It's not intended to replace Refit altogether, it's just intended to accomplish the same result in fewer clicks.

[ChillCore]

I'm using Chill's Patchpack, so I can at least do Split Refit

I have removed that patch from my patchpack as it is in trunk since r21567.
The only part that has not (yet) been added to trunk is the "all" button to select all wagons at once (which can also be done by clicking in front of the engine IIRC).


I agree that if the wagons are filtered by cargotype before buying them, that they should be autorefitted at purchase.

Hmm, should not be too hard for a first patch, me thinks.
The filter option is already selected at the time you buy the vehicle, so you just have to fetch its value while executing the refit command (-> after buying).
I have never looked at this code myself but you may want to start searching for what you need in build_vehicle_gui.cpp and go from there.

As extra encouragement ... if you succeed, I'll add it in my patchpack for testing before it goes to trunk. (Unless Devs add it to trunk before I have the oppertunity)

[Dewin]

I did look at the code for quite awhile -- just haven't written any yet. My concern was not being sure how to accomplish things in a multiplayer-safe way.

For instance, I did find this snippet in build_vehicle_gui.cpp:

Code: Select all

			case BUILD_VEHICLE_WIDGET_BUILD: {
				EngineID sel_eng = this->sel_engine;
				if (sel_eng != INVALID_ENGINE) {
					CommandCallback *callback = (this->vehicle_type == VEH_TRAIN && RailVehInfo(sel_eng)->railveh_type == RAILVEH_WAGON) ? CcBuildWagon : CcBuildPrimaryVehicle;
					DoCommandP(this->window_number, sel_eng, 0, GetCmdBuildVeh(this->vehicle_type), callback);
				}
				break;
			}

What I don't know yet is:
* How to get know if the vehicle successfully built at all at that point in execution
* If it is, a handle to the vehicle for CmdRefitVehicle (though I think I found this -- _new_vehicle_id )
* How to figure out the correct values for cargo type and subtype.
* Whether any of this will cause desyncs. (Though, if implemented correctly, patched clients should be able to play on unpatched servers.)
* How to handle the case of the player having enough money to build the vehicle but not enough to refit it afterwards.

Other then all of that, it looks like it's just a matter of DoCommandP(this->window_number, _new_vehicle_id, refit_cargo_bits) after the first DoCommandP.

[Rubidium]

In multiplayer DoCommandP sends a message to the server that the command should, eventually, be executed (if it passes an initial test). That means that e.g. _new_vehicle_id cannot be trusted to be of the vehicle that was created with the DoCommandP. Only in CommandCallbacks you can be reasonably sure that the value is of the just executed DoCommandP.

So, any DoCommandP depending on it must be executed from the callback, and as such the required parameters for the secondary DoCommandP need to be passed to the first DoCommandP so they end up in the CommandCallback. However... it would probably be less cumbersome (chaining wise) to call the refit DoCommand from the vehicle building code, except... that'll mean you might have to think about whether to apply to cost of the refit or not. That's tricky as you'd need to "predict" the cost of that.

Finally you need to figure out which cargo subtypes are available at the moment and choose one of those, however if you build near a year change (the usual moment where refit options change) on a network server you might get into the situation where the vehicle could be build, but not refitted to the chosen cargo (subtype) anymore. So, what to do in that situation? Not building the vehicle, building the vehicle but not refitting, showing an error, but what kind of error?

[bokkie]

Maybe useful: http://www.tt-forums.net/viewtopic.php?&t=35805

[peter1138]

Except you can't filter by cargo subtype (currently), so that's irrelevant...

[Dewin]

In multiplayer DoCommandP sends a message to the server that the command should, eventually, be executed (if it passes an initial test). That means that e.g. _new_vehicle_id cannot be trusted to be of the vehicle that was created with the DoCommandP. Only in CommandCallbacks you can be reasonably sure that the value is of the just executed DoCommandP.

I had a hunch this was the case.

So, any DoCommandP depending on it must be executed from the callback, and as such the required parameters for the secondary DoCommandP need to be passed to the first DoCommandP so they end up in the CommandCallback. However... it would probably be less cumbersome (chaining wise) to call the refit DoCommand from the vehicle building code, except... that'll mean you might have to think about whether to apply to cost of the refit or not. That's tricky as you'd need to "predict" the cost of that.

The problem then is finding room for the cargo type/subtype information. It looks like the entirety of p1 and p2 are used in CmdBuildVehicle, so there's not room to pass along the "try to refit to this" data (unless p2 = User can be fit into 19 bits or less -- I haven't delved this much yet.

Finally you need to figure out which cargo subtypes are available at the moment and choose one of those, however if you build near a year change (the usual moment where refit options change) on a network server you might get into the situation where the vehicle could be build, but not refitted to the chosen cargo (subtype) anymore. So, what to do in that situation? Not building the vehicle, building the vehicle but not refitting, showing an error, but what kind of error?

Are subtypes actual types of cargo (e.g. Coal, but extended to support more than 32 types of cargo), or are they more specialized things (e.g. I refit trams to have 3, 6 or 9 cars)? If the latter case, it's not necessary to pass the subtype information at all (you can't choose/filter by it at build time anyways) and the first applicable subtype can be determined on the server when the command actually runs. This would also limit me to only needing to hijack 5 bits instead of 13.

I could also hijack *text (having my data masquerade as a one-byte string), but this seems like an amazingly bad idea and presumably not the right implementation to use if I'm seeking inclusion into trunk.

EDIT: I could be wrong, but it doesn't look like CmdBuildVehicle's p2 actually does anything. It's documented as "user" and passed to some other functions, but I can't find any instances where it's ever anything ever than 0.

[Dewin]

Maybe useful: http://www.tt-forums.net/viewtopic.php?&t=35805

Looking at the source shows that it relies on _new_vehicle_id, which makes it unreliable for multiplayer for the reasons Rubidium explained above.

[frosch]

Btw. when the cargo-type filtering was added to trunk, the refit-feature was explicitly left out. Mainly because it is not possible to display the capacity in advance due to NewGRF limitations. (Same holds for cargo subtypes, i.e. what HEQS uses for cargo trams of different length)

[Zuu]

To me it sounds that it would be best to if possible extend the buy vehicle DoCommand to take an optional refit cargo parameter or create a new DoCommand that does Buy + Refit in one atomic command with respect of multiplayer games.

Another issue that could possible occur with separate DoCommands is that the bank balance is reduced in the time between the commands due to other reasons that the money used to buy the vehicle in question.