I've got a patch here to allow gradual loading at stations. which is a really nice feature of ttdpatch. It's controllable by a patch setting, so if you don't want it after all it's easy to turn off.
Historical comments: In theory, this supports the loading speed newgrf attribute (Action0, property 7), but I'm not convinced it actually does. As far as I can work out, the code in newgrf.c around line 1198 isn't actually doing anything, but it might just be the bit I changed that's wrong.
Edit: New version attached, which actually uses the load_amount of the wagons rather than that of the engines. The values are still confusing me a bit though - those from the UK Replacement Set seem reasonable, but those from the DBSetXL seem to be doubled, and somehow the Patch knows what to do with both of them...
Good patch Maedhros I've only given it a quick test with UKRS and so far it seems to work ok.
Having had a bit of experience with the NewGRF code, I had a look at your change in newgrf.c. It seems to work fine, action 0 property 7 is being set correctly.
mart3p wrote:Good patch Maedhros I've only given it a quick test with UKRS and so far it seems to work ok.
Having had a bit of experience with the NewGRF code, I had a look at your change in newgrf.c. It seems to work fine, action 0 property 7 is being set correctly.
Thanks! I was using the value for the engine before, rather than for every wagon, which is why I wasn't seeing any changes happening.
Patchman wrote:Loading speeds may be modified by callback 12, this is what you need to take into account if you want full compatibility.
Ooh, thankyou. And here I thought I could get away without knowing much about newgrfs... So much for that idea, huh. ^_^
I've updated the patch again by the way - I've added MS and MA macros in table/engines.h rather than hardcoding all the default values. It's also halved the size of the patch.
Patchman wrote:Loading speeds may be modified by callback 12, this is what you need to take into account if you want full compatibility.
Ooh, thankyou. And here I thought I could get away without knowing much about newgrfs... So much for that idea, huh. ^_^
I don't know how Open does callbacks, but it seems like (especially for 12, which is called on a vehicle, with no special variables) a simple call to GetCallbackResult(0x12, vehicle); ought to do the trick, followed by a check for whatever flag value means "callback failed"
DaleStan wrote:I don't know how Open does callbacks, but it seems like (especially for 12, which is called on a vehicle, with no special variables) a simple call to GetCallbackResult(0x12, vehicle); ought to do the trick, followed by a check for whatever flag value means "callback failed"
The function you want is GetVehicleCallback in newgrf_engine.c . You'll also need to add a new value to the CallbackID enum in newgrf_callbacks.h. The callback failed value, is the constant CALLBACK_FAILED.
As an example, have a look a peter1138’s changes at r5822 where he added support for callback 31.
Maedhros, be aware that I think there's a bug in there. Callback 31 is an 8-bit callback, but I don't see any code that obviously masks off the high 7 bits of the return from GetVehicleCallback.
Callback 12 is also 8 bits, so make sure you do an "& 0xFF" before using the return value (but after checking for CALLBACK_FAILED).
/me wonders how hard it would be to create a patch to fix this class of bugs once and for all; several people have had trouble with it.
Thanks a lot, mart3p and DaleStan, with your help it was much easier than I was expecting. I've updated the patch in the first post again so it now supports callback 12.
DaleStan wrote:/me wonders how hard it would be to create a patch to fix this class of bugs once and for all; several people have had trouble with it.
Yes, me being one of those "several people". I see you've already posted a fix, nice one! While looking again at the r5288 changes, I spotted another problem. See my bug report here.
Maedhros wrote:I've updated the patch in the first post again so it now supports callback 12.
iNVERTED wrote:Maedhros, the patch breaks any "Transfer and leave empty" order.
So it did.
The patch has been updated again, which now fixes the following:
Unloading to a station that didn't accept the cargo didn't work past the first $load_amount, and then it added the wrong amount to the station anyway.
If the "Improved loading algorithm" patch was enabled, vehicles wouldn't load while another train was in the station. It now checks to see if Full Load is in the orders so vehicles without it can load if another is in the station.
If the "Leave station when any cargo is full" patch was enabled, vehicles would leave the station when one cargo type was full. It now checks if Full Load is in the orders so they carry on loading the other cargo if it's available and they're not set to Full Load.
BTW, iNVERTED, please can you provide some more information next time you report a bug, like for instance what breaks, and what was supposed to happen...
iNVERTED wrote:I usually do, but this one was so obvious that I only needed to tell you what had broken.
Hehe, this is true.
Right. I think I've sorted out the transfers once and for all. There is now a cargo_unloading boolean in the Vehicle struct, so once the vehicle has unloaded all its cargo it won't try to unload anything that was just loaded.
Maedhros: while looking at your code, I noticed you are calling GetVehicleCallback for callback 12 from LoadUnloadVehicle in economy.c. This means the callback is being repeatedly called for all vehicles (and wagons) while they are loading. Although this works, it is unnecessary. The NewGRF spec says:
This callback is used instead of property 07 when loading a game or rearranging a train in a depot (or when buying other vehicles).
I think it would be better if you only used the callback at these times. Although it probably means adding a byte to the Vehicle structure to cache the callback result.
mart3p wrote:Maedhros: while looking at your code, I noticed you are calling GetVehicleCallback for callback 12 from LoadUnloadVehicle in economy.c. This means the callback is being repeatedly called for all vehicles (and wagons) while they are loading. Although this works, it is unnecessary. The NewGRF spec says:
This callback is used instead of property 07 when loading a game or rearranging a train in a depot (or when buying other vehicles).
I think it would be better if you only used the callback at these times. Although it probably means adding a byte to the Vehicle structure to cache the callback result.
I'm going to have to go back and read the newgrf spec more closely, obviously. It'll make the patch a bit more complicated to calculate the value for each type of vehicle when it's being bought or rearranged, but I do agree that's what should be done.
I should note that I wrote this "limitation" in the very beginning of the callback system, this was only the third callback, after all. Back then I overestimated the performance impact of doing a callback... but from what I can tell, at least in the patch, the cost of a callback is negligible unless it's called for every vehicle in every engine tick.
So unless callbacks are much more expensive in OTTD, it's probably fine to leave it as is.
The values are still confusing me a bit though - those from the UK Replacement Set seem reasonable, but those from the DBSetXL seem to be doubled, and somehow the Patch knows what to do with both of them...
I've updated the patch in the first post again so it now supports callback 12.
