OpenTTD

And another acknowledgment I overlooked: Credits to Wolf01 for the idea.

Is there any reason, this is not accepted to trunk? If so, it would be nice to know, so it can be fixed.

People keep asking about this feature again and again. And unless many other features there is even an implementation, which has been in any patch pack for over a year and can be considered well tested.

The desync issue of which rumor is still spread has been solved since January!


Update to trunk (savegame bump) attached...

this patch dosent work for 14442 need an update

Update to current trunk (player->company stuff)

Ok i patched it, but how do i find it in the game?

it work even in r14444

bool distant_join = !(GetCompany(_current_company)->is_ai) && (station_to_join != INVALID_STATION);

The AI part of the above check doesn't really make sense to me. Shuffling the parameters around breaks AIs anyway, so this check isn't really of much use. Instead I'd recommend fixing both AIs (default and trolly) by modifying their code so the DoCommand parameters are set correctly. This may be done in a separate patch, though. (mercurial queues)

I see what you mean. This was probably introduced by https://www.tt-forums.net/viewtopic.php?p=712255#p712255.

I'll have a look at it.

Okay, I repaired this AI issue basically as Swallow suggested: The checks for AI in station_cmd.cpp have been removed, the train station building commands by AIs have their bits reordered and all station building commands pass INVALID_STATION as the station to join.

Thank you for pointing it out.

Patch against hc733b3e4f97b (SVN 14600)

a) don't make variables needlessly global and don't add VARDEFs if not needed.
b) please share CommandContainer with CommandPacket where possible, i.e. make CommandPacket and subclass of CommandContainer
c) don't add needless parenthesis: if ((st == NULL) && (distant_join))
d) make the order of CommandContainer the same as for DoCommand(P); it got changed
e) variables should be names _bla_foo_bar, not _blaFooBar
f) use SmallVector instead of your own vector implementation (doesn't limit you to 15 or any arbitrary number of tiles)
g) use lengthof instead of your own lengthof implementation
h) us CircularTileSearch instead of your own implementation (e.g. FindStationsNearby)
i) it's y += 10, not y+= 10
j) use const Type * where applicable
k) use if (!<expression) return instead of if (expression) { vast piece of code; } <nothing>
l) document newly added functions
m) don't needlessly capitalise words in strings (it's not American!)
n) very short if functions are generally written on one line, unless it's part of an if-else
o) declare variables in the scope you actually need them and not in the highest scope of a function
p) use BringWindowToFrontById instead of FindWindowById in ShowSelectStation; it'll shortly flash the window when it already existed in the same way as all other windows do
q) use ///< comment instead of //comment when commenting variables of a struct/class
r) the rest looks okay

Wow, that's a long list. Seems as if I have a lot of homework this weekend ;-)

Thank you for your effort. I assume with those (mainly code style) issues fixed it will have a good chance of going into trunk?

As a preparation for h) I reworked CircularTileSearch a bit so that there is a generic variant that allows for rectangular searches and for leaving out a rectangular area in the middle.

The old CircularTileSearch forwards to the generic one in an appropriate way.

I did some hand-simulation to avoid obiwans and had it run with the OpenTTDCoop game #123 for some months and the autosaves matched with those from an unpatched OpenTTD (except for the revision string of course). Nevertheless, I would be grateful for a second look at the patch (I would recommend reading the patched files instead of the patch itself in this case). This kind of algorithms always produces knots in my brain ;-)

The revised "distant-join" patch will follow soon...

EDIT: Found a bug in the patch (hardcoded w instead of referencing extent[dir] which is either w or h). Fixed version is generic-circular-search-2.patch.

Phew, now I know, why I adhere to the coding standard in my own patches from begin on...

The attached patch solves all issues in the list except for b):
Both CommandContainer and CommandPacket have a field callback which is the callback function in one case and an ID (index into an array I assume?) in the other. So, we would need a common base class for both of them, making CommandContainer non-POD and thus disallowing direct initialization. Seems a bit of overkill for just four common data fields.

When switching to CircularTileSearch I reworked the building of the list in such a way that only stations are included that are completely within the station spread. So, unless the previous version, it should no longer be possible to get the "station too large" error when clicking on an offered item.

Patch against b7ae9c05bb61 / svn14847 (The one with "generic CircularTileSearch", thanks for the fast include, anyway :-)).

Please don't forget to mention Frostregen in the commit log, as there is still much of his patch inside.

Please forgive me, I'm going to do some nitpicking:

- Are you checking the validity of station_to_join in the various DoCommands?
- static SmallVector<uint16, 8> _stations_nearby_list; ///< shouldn't uint16 be a StationID?
- You use 'unsigned' as variable type in some places, uint(xx) may be better.
- DeleteWindowById(..) may be better than 'delete FindWindowById(..)', but I realise that the latter is already being used

And the patch will also need a small update after r14853

Other than that, great patch and I hope it will be included soon. :)

I let loose some of the other devs and well... it wasn't pretty.
- the commands didn't check for validity of the to-join-to-station, which can cause client crashes in MP! For next time: assume the input data (p1 and p2) are filled with garbage.
- the joiner window didn't get updated when a nearby station was removed (or added)
Both of the above I fixed, together with a few minor style things.

But then there's a few other things that are more complex than just changing a line here and there that needs to be fixed.
- the join station window misses a scrollbar and thus overflows downwards with many stations. It furthermore overflows to the right with long station names, so adding resizing and truncating DrawStringTruncated should be added too.
- the station picker windows stay open during the selection of another station, but any change made there will not result in the new selection to be build. So close the window, or update the select stuff accordingly.
- closing the (e.g.) road toolbar will remove the selection, but not close the select station window. Even worse, trying to build another station will still give you the same station selecter with the previous station data, so you can build a road stop with the rail toolbar opened.
- recently removed stations (when they're still gray and building a station near takes over the flag) don't show up in the list, even though you can still "join" with them; stations that are on the list and get removed stay on the list (while still gray) though.

This really makes me wonder how well "people" (i.e. the end users) have tested this patch and it enforces my believes that even when someone says it has been tested for months in patch packs it is still full of very basic bugs.

I hope that you can find the time to fix these problems.

> And the patch will also need a small update after r14853

Note to self: Whenever thinking "Should I..." (in that case "...make that a separate patch."): Just do it ;-)

> - recently removed stations (when they're still gray and building a station near takes over the flag) don't show up in the list, even though you can still "join" with them; stations that are on the list and get removed stay on the list (while still gray) though.

Deleted stations don't have a station tile, I guess. I just wonder how the GUI knows when to draw the gray sign. It doesn't loop over all stations like GetClosestDeletedStation(), does it? Will have to look at this tomorrow.

> This really makes me wonder how well "people" (i.e. the end users) have tested this patch and it enforces my believes that even when someone says it has been tested for months in patch packs it is still full of very basic bugs.

Well, end users do not try to break something intentionally as good testers do. I didn't notice any of those, either, though I used it in every (singleplayer) game for ages (as an end user, not as a tester). And I must admit that at least one bug is due to a fix of this weekend (at least I suspect the scrollbar wasn't needed before fixing item f). Actually, that proves that there is no need to wait long for being tested by the community as that can not substitute good code review and systematic testing anyway.

> I hope that you can find the time to fix these problems.
I hope I'll get at it tomorrow (well, actually today im MET :D). I already merged your changes into my local repository.

Okay, here we go again:

- Once again updated to trunk changes
- Station-to-join selector window is now resizable, has scrollbars and truncates strings.
- Whenever another station type is selected or the build station window is closed (directly or through deleting the toolbar), the window is closed.
- Recently deleted stations within station spread are now included in the list. This is a bit inconsistent to the checks in the command handlers, where the check is for DistanceManhattan(...) < 8, but this way it is consistent with stations deleted while the window is open. If that is not wanted the condition in station_gui.cpp:1056 needs to be changed.

(Mercurial patch against svn14905)