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

Distant-join-stations #2291

Closed
DorpsGek opened this issue Sep 11, 2008 · 18 comments
Closed

Distant-join-stations #2291

DorpsGek opened this issue Sep 11, 2008 · 18 comments
Labels
flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay

Comments

@DorpsGek
Copy link
Member

PhilSophus opened the ticket and wrote:

This is the distant-join-station patch which has been around for quite some time (http://www.tt-forums.net/viewtopic.php?t=30960) and allows you to build station parts which are separate to other parts of the same station. In the case of the adjacent station patch it also allows you to choose which station to join when multiple stations are adjacent instead of showing an error message (see http://www.tt-forums.net/viewtopic.php?p=683566# p683566 for the exact behavior depending on adjacent station setting)

It was first created by Frostregen (he gave his admission to post it here in http://www.tt-forums.net/viewtopic.php?p=727047# p727047), modified and bug fixed by me and Tiberius and kept up-to-date by multiple other people in the forum.

NOTE: This version of the patch does pass the station ID to join instead of an index to a dynamically built list and thus should be no source of desyncs. (In fact, I already changed that back in January). I also added a setting to disable this feature completely.

I hope, this patch is finally ready for trunk. If not, please comment here or in the forum post ASAP and I'll change it.

Attachments

Reported version: trunk
Operating system: All


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

PhilSophus wrote:

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


This comment was imported from FlySpray: https://bugs.openttd.org/task/2291#comment4736

@DorpsGek
Copy link
Member Author

PhilSophus wrote:

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...

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/2291#comment4779

@DorpsGek
Copy link
Member Author

DorpsGek commented Oct 5, 2008

batti5 wrote:

this patch dosent work for 14442 need an update


This comment was imported from FlySpray: https://bugs.openttd.org/task/2291#comment4823

@DorpsGek
Copy link
Member Author

DorpsGek commented Oct 5, 2008

PhilSophus wrote:

Update to current trunk (player->company stuff)

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/2291#comment4824

@DorpsGek
Copy link
Member Author

DorpsGek commented Oct 6, 2008

batti5 wrote:

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


This comment was imported from FlySpray: https://bugs.openttd.org/task/2291#comment4833

@DorpsGek
Copy link
Member Author

DorpsGek commented Oct 7, 2008

batti5 wrote:

it work even in r14444


This comment was imported from FlySpray: https://bugs.openttd.org/task/2291#comment4840

@DorpsGek
Copy link
Member Author

Swallow wrote:

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)


This comment was imported from FlySpray: https://bugs.openttd.org/task/2291#comment5006

@DorpsGek
Copy link
Member Author

PhilSophus wrote:

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

I'll have a look at it.


This comment was imported from FlySpray: https://bugs.openttd.org/task/2291#comment5042

@DorpsGek
Copy link
Member Author

PhilSophus wrote:

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)

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/2291#comment5043

@DorpsGek
Copy link
Member Author

DorpsGek commented Jan 3, 2009

Rubidium wrote:

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; }
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


This comment was imported from FlySpray: https://bugs.openttd.org/task/2291#comment5183

@DorpsGek
Copy link
Member Author

DorpsGek commented Jan 3, 2009

PhilSophus wrote:

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?


This comment was imported from FlySpray: https://bugs.openttd.org/task/2291#comment5188

@DorpsGek
Copy link
Member Author

DorpsGek commented Jan 5, 2009

PhilSophus wrote:

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.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/2291#comment5196

@DorpsGek
Copy link
Member Author

DorpsGek commented Jan 5, 2009

PhilSophus wrote:

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.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/2291#comment5204

@DorpsGek
Copy link
Member Author

DorpsGek commented Jan 5, 2009

Swallow wrote:

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. :)


This comment was imported from FlySpray: https://bugs.openttd.org/task/2291#comment5206

@DorpsGek
Copy link
Member Author

DorpsGek commented Jan 5, 2009

Rubidium wrote:

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.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/2291#comment5207

@DorpsGek
Copy link
Member Author

DorpsGek commented Jan 6, 2009

PhilSophus wrote:

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.


This comment was imported from FlySpray: https://bugs.openttd.org/task/2291#comment5213

@DorpsGek
Copy link
Member Author

DorpsGek commented Jan 8, 2009

PhilSophus wrote:

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)

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/2291#comment5240

@DorpsGek
Copy link
Member Author

DorpsGek commented Jan 8, 2009

Rubidium closed the ticket.

Reason for closing: Implemented

In r14919.


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

@DorpsGek DorpsGek closed this as completed Jan 8, 2009
@DorpsGek DorpsGek added Core flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay labels Apr 6, 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/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay
Projects
None yet
Development

No branches or pull requests

1 participant