Bug 163: Patch

StormDragoness · Post by **StormDragoness** » 14 Aug 2006 18:57

When applied this patch modifies economy.c - DeliverGoods()

It will deliver the goods to the station/city, however will not pay the player as the origin station of their cargo no longer exists.

hertogjan · Post by **hertogjan** » 14 Aug 2006 19:20

Don't you think that it would have been easier to just add the line

Code: Select all

if (s_from->xy == 0) return 0;

where it makes sense (i.e. where your if statement is).
Would have saved you from adding all those extra indentation

StormDragoness · Post by **StormDragoness** » 14 Aug 2006 19:34

Probably.. I am one of those people though, who like having return sit at the end of a function rather than scattered all over the place.

GreyHound · Post by **GreyHound** » 14 Aug 2006 21:07

every not executed program code is saving memory tho

Brianetta · Post by **Brianetta** » 14 Aug 2006 21:53

It's not about saving memory, it's about maintainable code. StormDragoness is engaging in good programming practice by putting the return in a predictable place. A good compiler would probably optimise it to the same machine code, anyway.

Expresso · Post by **Expresso** » 15 Aug 2006 02:18

why not change the source station into source tile? This would get rid of this bug, without the need for an ugly workaround.

StormDragoness · Post by **StormDragoness** » 15 Aug 2006 07:16

This would be a job for someone with more experience, since cargo only retains the stationID it came from, not the tile, or any other information about the station.

I believe I heard something about the cargo system being rewritten in due time.

This bug should be kept in mind for that time.

Maedhros · Post by **Maedhros** » 15 Aug 2006 07:43

How about turning it around and not deleting the station while there's still cargo that references it?

(Disclaimer - I haven't looked closely at this patch or the bug in question...)

MeusH · Post by **MeusH** » 15 Aug 2006 11:17

Maedhros wrote:How about turning it around and not deleting the station while there's still cargo that references it?

(Disclaimer - I haven't looked closely at this patch or the bug in question...)

It would be too resource consuming to do such a trivial task. To check whether there is some cargo from the station that is going to be removed, it would be required to check every wagon and every other station and compare the cargo roots.

why not change the source station into source tile? This would get rid of this bug, without the need for an ugly workaround.

That sounds reasonable if we store Tile ID instead of Station ID. Station can be accessed anyway as we have tile ID, and there will be no problem of ghost stations.

On the other hand, maybye we're making too much trouble around such a small trouble. It isn't often that station disappears completly and vehicles are still up and running.

MeusH · Post by **MeusH** » 16 Aug 2006 18:41

Anyone dares touching it?
It compiles, but half a second after running the game an assert pops up.

I know where it is, and I hope I'll find it, but I post this file for two reasons:
1)You know someone is working on it, so no doublework
2)Someone can give me tips about the code

I just lack in time right now to debug

gigajum · Post by **gigajum** » 16 Aug 2006 20:03

I think the the lowest tile number of a industry should be used instead of stations lowest tile number (as it is atm).

Darkvater · Post by **Darkvater** » 16 Aug 2006 21:14

So how does this actually compile?

Code: Select all

Station *s_from, *s_to;
+	s_from = GetStationIndex(source);

Huh? That won't really work.

MeusH · Post by **MeusH** » 17 Aug 2006 08:10

Well I told you it doesn't work.
But thanks for pointing this out. I just ran out of time and couldn't fix that.

MeusH · Post by **MeusH** » 17 Aug 2006 09:26

Allright, I know what's wrong with it.
It isn't savegame compatibile.

The assertion fails at start of the game because the vehicles from opntitle.dat still have StationID cargo_source instead of TileIndex.

The game works almost fine without opntitle.dat (there is an empty map visible on the menu screen).
It crashes, however, when the station is deleted... Too much coding, I'm discontinuing it, but if you really want, you can see the patch file.

In spite of that, I've got another idea how to fix that bug

Pendrokar · Post by **Pendrokar** » 17 Aug 2006 09:52

Damn I didn't think that fixing Bug 163 would be so hard.
I would help but I don't know how OpenTTD Code works or to what coding it is similiar.

And is Victor Fisher even working on it?

MeusH · Post by **MeusH** » 17 Aug 2006 10:44

This patch needs saving/loading TileIndex cargo_source_tile, and creating one from StationID cargo_source if old savegame has been loaded (how?).

I don't know if I'll continue work on this - it is possible if I find info on the savegame versions and developers won't have anything against making new savegame version.

Darkvater · Post by **Darkvater** » 17 Aug 2006 12:18

Something like this perhaps: http://bugs.openttd.org/task/163

The biggest problem was handling the bad/old cases of which I am not sure I have. The game might still crash when you load a game that already has a vehicle coming from a demolished station. Someone will have to take a deeper look into it.

MeusH · Post by **MeusH** » 17 Aug 2006 12:41

Excellent fix. Now I'll test it.

Edit: I have tested it:

Step 1: Save a game using the unpatched trunk
I created a small oil wells - oil rafinery line with one train. No sooner the train had left the station than I demolished it and saved the game. Just for testing purposes, I continued to play to see the payment - over 10000$$

Step 2: Load a game using patched trunk
I loaded the game and after a few seconds train approached the station and game crashed. Before entering the platform, I checked its properties and saw "30 000 litres of oil from unknown destination"

The game didn't crash in the gui part when checking cargo's destination.
It crashed, however, somewhere in economy.c. It could be DeliverGoods or LoadUnloadVehicle, I think

MeusH · Post by **MeusH** » 17 Aug 2006 12:57

in LoadUnloadVehicle

/* unload? */
if (v->cargo_count != 0) {
if (v->cargo_source != st->xy && ge->waiting_acceptance & 0x8000 && !(u->current_order.flags & OF_TRANSFER)) {
// deliver goods to the station

Has v->cargo_source been converted to TileIndex? I'm afraid it could be still StationID

------------------------------------------------

in DeliverGoods

// Get station pointers.
s_from = GetStationByTile(source);
s_to = GetStation(dest);

// Check if a subsidy applies.
subsidised = CheckSubsidised(s_from, s_to, cargo_type);

Maybye it is CheckSubsidised that crashes?

Darkvater · Post by **Darkvater** » 17 Aug 2006 13:29

Yes, s_from to CheckSubsidies is NULL, so that crashes.

The problem is now that behaviour has changed. You see that because v->source is not a StationID anymore, the moment you demolish the station the details window says 'unknown destination'. Even while in the old behaviour kept saying the old station as long as it was 'grey'.

This of course also removes any potential subsidies since those are handled by StationID as well... WTF, that's totally gay. Stations have nothing to do with subsidies, it's always town/industry <> town/industry.

Hmm..needs more thinking.

Ah, if a subsidy is serviced, it is given the Station as source and destination. Should this be turned into a TileIndex as well?

Code: Select all

	subsidised = (s_from != NULL) ? CheckSubsidised(s_from, s_to, cargo_type) : false;

Just do the above to fix the crash;

Transport Tycoon Forums

Bug 163: Patch

Bug 163: Patch

Who is online