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

Broken gradual loading #5508

Closed
DorpsGek opened this issue Mar 18, 2013 · 10 comments
Closed

Broken gradual loading #5508

DorpsGek opened this issue Mar 18, 2013 · 10 comments
Labels
flyspray This issue is imported from FlySpray (https://bugs.openttd.org/)

Comments

@DorpsGek
Copy link
Member

V453000 opened the ticket and wrote:

Somehow my trains seem to load cargo very strangely when there is a lot of waiting cargo in the station.

Train arrives, starts loading.

In the Cargo window of the train, there instantly appears full load.
In the white % number above train head, loading stages happen as normally. (here loading speed is 5 while capacity is 45)
The loading stages from the loading spritegroup however adapt to the Cargo window, which means graphical visual stages are skipped.

When I skip order and force train to go to unload, it will stop loading and show proper values in the Cargo window, also making the sprites show appropriately.

Tested with 25071 and the attached newGRF, but it also works with the second train in the savegame which is from older versions of NUTS

Attachments

Reported version: trunk
Operating system: All


This issue was imported from FlySpray: https://bugs.openttd.org/task/5508
@DorpsGek
Copy link
Member Author

V453000 wrote:

I somehow cant upload the other 2 files so here they go

https://dl.dropbox.com/u/20419525/borkdloading.sav

https://dl.dropbox.com/u/20419525/nuts.grf


This comment was imported from FlySpray: https://bugs.openttd.org/task/5508#comment12101

@DorpsGek
Copy link
Member Author

V453000 wrote:

- it only seems to happen when there is a lot of cargo in the station
- the train unloads normally


This comment was imported from FlySpray: https://bugs.openttd.org/task/5508#comment12102

@DorpsGek
Copy link
Member Author

V453000 wrote:

1.3.0 RC2 does not have this issue while 25071 does, so it is somewhere in between


This comment was imported from FlySpray: https://bugs.openttd.org/task/5508#comment12103

@DorpsGek
Copy link
Member Author

V453000 wrote:

http://vcs.openttd.org/svn/?action=stop_on_copy&mode=stop_on_copy&rev=25016&stop_rev=25012&limit=100 Looks like some of those


This comment was imported from FlySpray: https://bugs.openttd.org/task/5508#comment12104

@DorpsGek
Copy link
Member Author

fonsinchen wrote:

It shows the reserved cargo in the vehicle window but it should only show the "onboard" cargo. The patch fixes that and some other places where I assume we'd rather want to see onboard count than count.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/5508#comment12111

@DorpsGek
Copy link
Member Author

frosch wrote:

I tried to get an overview when cargo.Count() and cargo.OnboardCount() is used.
* Vehicle::Crash(): I would expect pax to survive trains crashes during loading, if they have not boarded yet. So OnboardCount()?
* ScriptVehicle::GetCargoLoad(): Not sure here, since the station does not longer count the reserved cargo. But OnboardCount() still feels more logical.
* VehicleGetVariable(): NewGRF should only know about loaded stuff. So OnboardCount() everywhere.
* Train::GetWeight(): OnboardCount()
* RoadVehicle::GetWeight(): OnboardCount()

Overall, I think the behaviour of VehicleCargoList::Count() is not very intuitive and error-prone. How about trying to remove all usages of Count(), and replacing it with more explicit functions?
* VehicleCargoList::OnboardCount(): CargoPacket::Count() - action_counts
* VehicleCargoList::ReservedCount(): CargoPacket::Count()
* VehicleCargoList::Count(): Assert if action_counts != 0.

I don't know whether ReservedCount() would be the right wording. Also, I don't know about unloading. OnboardCount() seems to consider only MTA_LOAD, how does stuff work with MTA_TRANSFER?


This comment was imported from FlySpray: https://bugs.openttd.org/task/5508#comment12113

@DorpsGek
Copy link
Member Author

fonsinchen wrote:

Cargo isn't reserved when unloading, only when loading. The cargo marked with the other MoveToActions is "really" in the vehicle, potentially waiting to get out. So the actions of transferring, unloading or keeping cargo are not affected. It's true that OnboardCount() can be used in most places where Count() is used now. There are a few legitimate uses of Count() as "sum of onboard and reserved cargo", though:

* Vehicle::Crash(): As we don't return cargo when crashing it would be more logical to count the passengers waiting at the doors as being crushed by wagons falling over or similar effects. (or otherwise we could return them before crashing)
* IsArticulatedVehicleEmpty(): We don't want to autorefit a vehicle that has already reserved something.
* LoadUnloadVehicle(): In 5 places there, for different reasons.

We could make the non-asserting method a TotalCount(), analogous to the method in StationCargoList. ReservedCount in StationCargoList counts only the reserved cargo. We probably shouldn't add a method that does something else under the same name to VehicleCargoList.


This comment was imported from FlySpray: https://bugs.openttd.org/task/5508#comment12114

@DorpsGek
Copy link
Member Author

fonsinchen wrote:

As it is now VehicleCargoList::Count() and StationCargoList::Count() are designed to complement each other, just like they did before the reservation change. VehicleCargoList::OnboardCount() and StationCargoList::TotalCount() also complement each other. I'm not sure if it's a good idea to change that.

Consequently, if ScriptVehicle::GetCargoLoad() should return OnboardCount then ScriptStation::GetCargoWaiting() should return TotalCount.


This comment was imported from FlySpray: https://bugs.openttd.org/task/5508#comment12118

@DorpsGek
Copy link
Member Author

fonsinchen wrote:

What about this:
-> Count() is confusing for any client of any cargo list, so we keep it protected.
-> Both station and vehicle cargo lists provide
* StoredCount() for the amount of cargo without reservations
* TotalCount() for the amount of cargo with reservations
* ReservedCount() for the reservations alone
-> For each occurence of Count() we're forced to define what we actually mean. For my proposals see the patch.
-> Empty() is equally confusing but I haven't changed anything about that, yet.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/5508#comment12151

@DorpsGek
Copy link
Member Author

frosch closed the ticket.

Reason for closing: Fixed

in r25185


This comment was imported from FlySpray: https://bugs.openttd.org/task/5508

@DorpsGek DorpsGek added Core flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) labels Apr 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flyspray This issue is imported from FlySpray (https://bugs.openttd.org/)
Projects
None yet
Development

No branches or pull requests

1 participant