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

Bug fix for bouy #6196

Closed
DorpsGek opened this issue Dec 29, 2014 · 5 comments
Closed

Bug fix for bouy #6196

DorpsGek opened this issue Dec 29, 2014 · 5 comments
Labels
flyspray This issue is imported from FlySpray (https://bugs.openttd.org/)

Comments

@DorpsGek
Copy link
Member

hackalittlebit opened the ticket and wrote:

Bug fix for bouy.
While testing I managed to crash the game.
This was due to the fact that in old safegames the waterclass and waypoint facilities flags were not correctly set for bouy's.
Buoy waypoint facilities flags shoud be FACIL_DOCK and FACIL_WAYPOINT see CmdBuildBuoy.
I the old savegame it only has FACIL_WAYPOINT.
You can test this by copying "..bin/baseset/opntitle.dat" to your gamesave folder and change extention into .sav
The provided patch solves the problems and has some debugging info to check the things.

I noted that while I was testing that ships orders to the bouy were 'non-stop' maybe that should be changed also.

Regards HackaLittleBit. (And thank you for your efforts)
And.... Happy new year.;-)

Attachments

Reported version: trunk
Operating system: All


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

hackalittlebit wrote:

And I forgot to tell you that when you save the old game and then open the newly saved game the problem disappears (somewhat).
I did not, however check if both flags and waterclass were correctly saved.


This comment was imported from FlySpray: https://bugs.openttd.org/task/6196#comment13667

@DorpsGek
Copy link
Member Author

Rubidium wrote:

I'm not sure this patch does anything, at least not for the specified case.

For the 1.4 opntitle.dat this doesn't trigger at all (savegame 188, so not before 188). Even then, when I change the 188 to 189 it just spits out identical results for before and after for each of the docks.

For the trunk opntitle.dat it does trigger, but they are correct as well.

I guess this has mostly to do with the method MoveBuoysToWaypoints which adds FACIL_DOCK for buoys and essentially line 1780 of afterload which sets the water class depending on its surroundings for buoys and docks.

So I am not really seeing where these wrong states are coming from; can you provide a savegame where this patch actually changes something?


This comment was imported from FlySpray: https://bugs.openttd.org/task/6196#comment13677

@DorpsGek
Copy link
Member Author

DorpsGek commented Jan 1, 2015

hackalittlebit wrote:

I am at loss.
You know I am experimenting with splitting up waypoint buoys into air waypoints and ship waypoints.
I got a crash with the title game.(pristine game)
Since I was messing around with buoys I started investigating.
I relied on my own code (patched version) and following is what it spits out on this side
dbg: [misc] TILE: 0x4483
dbg: [misc] before wc: 3, FACIL_DOCK = 0, FACIL_WAYPOINT = 128
dbg: [misc] after wc: 0, FACIL_DOCK = 16, FACIL_WAYPOINT = 128
dbg: [misc] TILE: 0x487b
dbg: [misc] before wc: 3, FACIL_DOCK = 0, FACIL_WAYPOINT = 128
dbg: [misc] after wc: 1, FACIL_DOCK = 16, FACIL_WAYPOINT = 128
dbg: [misc] TILE: 0x4483
dbg: [misc] before wc: 3, FACIL_DOCK = 0, FACIL_WAYPOINT = 128
dbg: [misc] after wc: 0, FACIL_DOCK = 16, FACIL_WAYPOINT = 128
dbg: [misc] TILE: 0x487b
dbg: [misc] before wc: 3, FACIL_DOCK = 0, FACIL_WAYPOINT = 128
dbg: [misc] after wc: 1, FACIL_DOCK = 16, FACIL_WAYPOINT = 128
dbg: [misc] TILE: 0x4483
dbg: [misc] before wc: 3, FACIL_DOCK = 0, FACIL_WAYPOINT = 128
dbg: [misc] after wc: 0, FACIL_DOCK = 16, FACIL_WAYPOINT = 128
dbg: [misc] TILE: 0x487b
dbg: [misc] before wc: 3, FACIL_DOCK = 0, FACIL_WAYPOINT = 128
dbg: [misc] after wc: 1, FACIL_DOCK = 16, FACIL_WAYPOINT = 128

This evening I applied my patch to clean trunk. And guess what, I get the same results as you.
So huge embarresment here. I am sorry I tried to help.
So now it also made me really curious.

Attatched is patch to split IsBouy proc in two.
With both patches applied (trunk_4_21949.patch and split Isbouy.patch) ) in clean trunk I was able to reproduce my result as above.
Now what is happening is that with method 'MoveBuoysToWaypoints' in line 104
" } else if (IsBuoyTile(xy) && GetStationIndex(xy) == index) {"
IsBuoyTile does NOT detect the bouy and conversion does not happnen.
That is because I change myself the detecting ability of the IsBuoyTile proc.
In fact I introduce a bug.
In the trunk_4_21949.patch I rely on "GetStationType(tile) == STATION_BUOY" to detect the Bouy.
Maybe you could change line 104 into:
} else if (IsTileType(tile, MP_STATION) && GetStationType(tile) == STATION_BUOY && GetStationIndex(xy) == index) {
In that way future programmers won't fall in the same trap as I did.;-)
Again sorry for wasting your time.
I will make a rectification to the post in the forum.

Hans

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/6196#comment13682

@DorpsGek
Copy link
Member Author

DorpsGek commented Jan 1, 2015

Alberth wrote:

??
There is no "tile" variable in that function.

If you mean "xy" instead of "tile", the code already does what you suggest, after unfolding IsBuoyTile and IsBuoy.

In trunk:
static inline bool IsBuoyTile(TileIndex t) { return IsTileType(t, MP_STATION) && IsBuoy(t); }
static inline bool IsBuoy(TileIndex t) { return GetStationType(t) == STATION_BUOY; }

"IsBuoyTile(xy) && GetStationIndex(xy) == index" is thus already your suggested
"IsTileType(xy, MP_STATION) && GetStationType(xy) == STATION_BUOY && GetStationIndex(xy) == index"

Am I missing something?


This comment was imported from FlySpray: https://bugs.openttd.org/task/6196#comment13686

@DorpsGek
Copy link
Member Author

DorpsGek commented Jan 1, 2015

Rubidium closed the ticket.

Reason for closing: Not a bug

Even though it may have confused you, the current code is right and even performs the check you ask for underwater. When you change the meaning of a function, you need to check its users in afterload and possibly amend those instead of adding duplicate wrapper code in afterload just in case someone might want to change the meaning of a wrapper function later on. If we would do that, we would still be working with magic values in afterload.


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

@DorpsGek DorpsGek closed this as completed Jan 1, 2015
@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