New map features

[3298]

Just crashed the game, and this time I have no clue how to fix it. (Not simply trains refusing to enter already visited platforms.) It might be something with the crashed trains the green AIAI company managed to produce, but the crash log mentions GetTileType, and I do not know how these fit together.
My last autosave (30 days of me doing nothing before the crash) reliably reproduces the crash. It is attached below.
My binary is r25064 with the latest patch I could find in this thread. (I miss YACD, but r25008 to r25013 broke it. So just this patch for now.)
I hope you can find out where this crash is coming from. Another thing I noticed: AIAI provoked the train crash with some signals facing the wrong way round, is that caused by signal-related changes in this patch or a bug in the AI?

[cirdan]

Just crashed the game, and this time I have no clue how to fix it. (Not simply trains refusing to enter already visited platforms.) It might be something with the crashed trains the green AIAI company managed to produce, but the crash log mentions GetTileType, and I do not know how these fit together.
My last autosave (30 days of me doing nothing before the crash) reliably reproduces the crash. It is attached below.
My binary is r25064 with the latest patch I could find in this thread. (I miss YACD, but r25008 to r25013 broke it. So just this patch for now.)
I hope you can find out where this crash is coming from. Another thing I noticed: AIAI provoked the train crash with some signals facing the wrong way round, is that caused by signal-related changes in this patch or a bug in the AI?

If the game crashes, then that is a bug; AIs should not be able to crash the game, no matter what. I will have a look as soon as I can.

Also I had a quick browse throught the first 15/20 patches or so and IHMO many of these patches belong in trunk instead of in your patchpack. .

Glad somebody notices...

I already told Michi_cc, more than a year ago, that the first ten patches or so in the branch were cleanups and general improvements, unrelated to custom bridgeheads or the new map array. Of course, that was when the branch had 332 patches and was not as polished as it is now. Currently the branch is 399 commits deep and the number of such general patches has grown to 71 (!). Sadly, none of them have been reviewed or applied, except for one that was independently rediscovered by frosch in r24328, even though some of them are very simple and at least one of them is a one-line patch that fixes a blatant bug in the documentation for a function.

Would you mind me extracting some stuff from time to time like codechanges and codingstyle that do not really affect your patch functionwise?

I am not sure about what you mean by 'extracting'. If you want to cherry-pick some patches and submit them for inclusion in trunk, feel free to do so. My advice would be to start at the beginning and go on from there, since the patches are more or less arranged by increasing complexity. Also, there may be interaction between them, so applying them out of order may be harder. As a bonus, the first 80 patches now have a descriptive comment with a rationale for the change or background information. I hope you have more luck attracting a dev's attention than I have had.

Or would it cause to much trouble for you to rebase and stuff if some things already went to trunk at this point in time?

Certainly not. git is extremely good at detecting patches already upstream when rebasing, and resolving simple merge conflicts otherwise. And, even if adjusting for upstream inclusion of a patch does eventually require manual intervention, I would rather do that once than keep the patch in my branch forever.

[planetmaker]

Cirdan,

if you believe that single patches from your queue are fixes for trunk on their own accord and independent of the giant patch queue: then please come forward, submit them each as separate trunk patches to our issue tracker. That then has several advantages:
* allows independent review of each of them
* and ensures that they're not somewhere deeply burried inside a patch pack queue in a topic which likely will not be thought of when someone decides to look at and review "what patches and improvement suggestions do we have suggested?".
* lowers the psychological entry barrier quite dramatically as opposed to "find what suits you in 300-odd patches"

EDIT: I see that you made a nice patch queue and all, attached here in the postings. Still, if you want something particular from within that queue in trunk and reviewed: consider to also make it easy for us. And easiest is if it's broken down to one issue per independent problem. And in our issue tracker. Esepcially for big projects it makes perfect sense to reduce the overall project by these means to their real core. Getting tiny parts done piece by piece ahead of them as far as possible.

[3298]

Just crashed the game, and this time I have no clue how to fix it. (Not simply trains refusing to enter already visited platforms.) It might be something with the crashed trains the green AIAI company managed to produce, but the crash log mentions GetTileType, and I do not know how these fit together.
My last autosave (30 days of me doing nothing before the crash) reliably reproduces the crash. It is attached below.
My binary is r25064 with the latest patch I could find in this thread. (I miss YACD, but r25008 to r25013 broke it. So just this patch for now.)
I hope you can find out where this crash is coming from. Another thing I noticed: AIAI provoked the train crash with some signals facing the wrong way round, is that caused by signal-related changes in this patch or a bug in the AI?

If the game crashes, then that is a bug; AIs should not be able to crash the game, no matter what. I will have a look as soon as I can.

Tha AI probably does not directly crash the game, I rather think the crashed trains do. But it also could be something different.
Also if you need them, I can provide autosaves from one and two months earlier, at least in the earliest of these the trains didn't crash yet. (And I watched the network also, so it's just a matter of waiting until the game crashes.)
On another note, the red-signal-on-bridgehead-stops-train issue can also be observed on the same map near the steel mill. (Just follow the AI's rail line, it ends very close to the steel mill.) There are path signals on the bridgeheads of two bridges over a river, and every train stops for a moment when leaving the bridge.

[ChillCore]

Currently the branch is 399 commits deep and the number of such general patches has grown to 71 (!). Sadly, none of them have been reviewed or applied,

I was not aware there are 71 of them.
Being able to make your patch queue so much lighter would already be a major step forwards.
Like planetmaker said 399 patches to review is a steep hurdle, even if you plan to take a few weeks to just read the pathes without thorough examination.

I am not sure about what you mean by 'extracting'. If you want to cherry-pick some patches and submit them for inclusion in trunk, feel free to do so.

I meant cherry-pick yes, bad choice of words on my behalf.
Thank you for the tips.
I am not going to give an estimate on when and how but I will try to do the first few this weekend.

Also, there may be interaction between them, so applying them out of order may be harder.

I did notice that you have split up patches to the extreme and perhaps the devs would want some of those patches bundled.
In general it is prefered to have patches split up, but maybe not to this point. Let's just wait and see what Devs say after submitting the first few.

[cirdan]

Cirdan,

if you believe that single patches from your queue are fixes for trunk on their own accord and independent of the giant patch queue: then please come forward, submit them each as separate trunk patches to our issue tracker. That then has several advantages:
* allows independent review of each of them
* and ensures that they're not somewhere deeply burried inside a patch pack queue in a topic which likely will not be thought of when someone decides to look at and review "what patches and improvement suggestions do we have suggested?".
* lowers the psychological entry barrier quite dramatically as opposed to "find what suits you in 300-odd patches"

EDIT: I see that you made a nice patch queue and all, attached here in the postings. Still, if you want something particular from within that queue in trunk and reviewed: consider to also make it easy for us. And easiest is if it's broken down to one issue per independent problem. And in our issue tracker. Esepcially for big projects it makes perfect sense to reduce the overall project by these means to their real core. Getting tiny parts done piece by piece ahead of them as far as possible.

From my point of view, it is not as if opening patch tickets at the bugtracker makes much difference, usually. Take, for instance, my patch to close airports: its flyspray task remained open for over 4 years, mostly ignored, until it was eventually applied. My patch to upgrade airports has not had such luck. It received some attention for as long as you could find bugs in it, but, after that, it has not seen any actual review activity: neither accepted nor rejected, and no replies to my comments, just cast into oblivion. A tiny patch I wrote to align towns took more than 2 months to be noticed, and I feel it was applied only because PhilSophus took it upon himself to champion it with you. Lastly, my depot-upgrade patch, which I published a year and a half ago, is still waiting for its first comment by a dev. It is easy to get the impression that you ignore flyspray tasks unless they are bug reports.

Now, you come up and ask me to post my patches there, so it is easier for you to review them. But the thing is, given my past experience, I wonder if it is worth the hassle. For me, posting a patch here or at the bugtracker implies a strong commitment to actually support it and address all issues raised (or, at least, give an explanation of why something cannot be done or is not worth the effort), and not drop it lightly. It is rewarding to see the response here at the forums, where people help by adding suggestions, reporting bugs, and generally contributing to make patches better. The situation at the bugracker, on the other hand, is disheartening at best. This makes me think that you do not give much value to contributions, and this feeling is backed up by the fact that the OpenTTD web page, unlike most free software projects, has no mention whatsoever of what to do to send in patches, only bug reports.

So, it strikes me as surprising that you now tell me that I should post my patches at flyspray. What would be different this time? Why are so many tasks, not just mine, ignored on flyspray? These are honest questions: I am trying to understand how you deal with the bugtracker, because at the moment I cannot.

Tha AI probably does not directly crash the game, I rather think the crashed trains do. But it also could be something different.

Now, that is a different matter. I never really tested crashing trains, so there may be bugs lurking in that area. I am still hunting for some NewGRFs in your game, but at least I know I will not have to also download the AIs.

I did notice that you have split up patches to the extreme and perhaps the devs would want some of those patches bundled.
In general it is prefered to have patches split up, but maybe not to this point. Let's just wait and see what Devs say after submitting the first few.

There is a simple rule for this: Squashing is easy, splitting is hard.

If at some point you decide that a few commits are so small that it would be better to merge them, then it is trivial to do so (git even has options for that). On the other hand, if you decide that a commit is too large and would rather split it, that is a lot harder. Therefore, I always tend to err on the side of making too many patches, rather than too few.

[3298]

Tha AI probably does not directly crash the game, I rather think the crashed trains do. But it also could be something different.

Now, that is a different matter. I never really tested crashing trains, so there may be bugs lurking in that area. I am still hunting for some NewGRFs in your game, but at least I know I will not have to also download the AIs.

Wait, what? I got all my NewGRFs from BaNaNaS, except dbsetxlw.grf (the DBSetXL), which is not active in this game. But for your convenience, I've attached the NewGRFs used in that game (I hope the creators do not mind), the AIs are Clueless and AIAI, and the script is NoCarGoal. All NewGRFs except OGFX+ Airports and Industrial Stations Renewal are the current ones.
Maybe I should have written "train collision" instead of "train crash".
Also attached is a manual save one day before the game crashes, to save you an in-game month of time.
The stuff I wrote about the possible bug in the AI was about a signal facing the wrong way round, causing a train to reverse, drive into the depot visible on the crash.png and getting hit there by the other train which left the station. I just don't know whether the bug that caused this signal to face away from the train is in AIAI or the signal placement code. Thinking about it a bit more, it's more likely an AIAI issue because it also places no signals at the station exit and has no consistent rule on which side to drive (parts of the rail line are right side traffic, other parts are left side traffic)...

Edit: NewGRF zip removed because I don't want any trouble with copyright violation and because it is not needed anymore. (3 downloads were shown, who else grabbed that zip?)

[cirdan]

Wait, what? I got all my NewGRFs from BaNaNaS, except dbsetxlw.grf (the DBSetXL), which is not active in this game. But for your convenience, I've attached the NewGRFs used in that game (I hope the creators do not mind), the AIs are Clueless and AIAI, and the script is NoCarGoal. All NewGRFs except OGFX+ Airports and Industrial Stations Renewal are the current ones.

Perhaps "hunting for NewGRFs" was an overstatement, I was just downloading them from Bananas

Also attached is a manual save one day before the game crashes, to save you an in-game month of time.

Thank you for all your efforts to help me. However, at the time of your post I had already identified the problem and was working on a fix, which I now present you with.

The stuff I wrote about the possible bug in the AI was about a signal facing the wrong way round, causing a train to reverse, drive into the depot visible on the crash.png and getting hit there by the other train which left the station. I just don't know whether the bug that caused this signal to face away from the train is in AIAI or the signal placement code. Thinking about it a bit more, it's more likely an AIAI issue because it also places no signals at the station exit and has no consistent rule on which side to drive (parts of the rail line are right side traffic, other parts are left side traffic)...

I am by no means an AI expert, but I guess that AIs will have to be taught about signals on bridgeheads to properly use them.

By the way, the bug about trains bumping full-speed into red signals should also be fixed.

[3298]

Thanks, it doesn't crash anymore, and the trains on the bridges with one-way PBS on the bridgeheads don't stop that abrupty anymore. They still stop, though, probably because they don't see the signal while they are in the wormhole, and PBS need that to turn green. I will replace them with block signals for now.

The AI's signal is on a normal rail, not a bridgehead. It's the one to the left of the red bridge, at the border of the crash.png. A properly written AI for vanilla OpenTTD shouldn't even try to place a signal on a bridgehead because it will not work. But I believe that you shouldn't need to teach the AIs how to place signals on bridgeheads. If the signal placement on bridgeheads works through the same command as the signal placement on normal rails, they should just need to try it. They should get an error in trunk - and a signal with this patch. Note: I am not exactly an AI expert either, just a player with a little programming experience.

[cirdan]

Thanks, it doesn't crash anymore, and the trains on the bridges with one-way PBS on the bridgeheads don't stop that abrupty anymore. They still stop, though, probably because they don't see the signal while they are in the wormhole, and PBS need that to turn green. I will replace them with block signals for now.

I'm not seeing anything strange with one-way path signals on bridgeheads. Could you describe the exact situation in which this happens?

[3298]

In the savegame I posted, search for the steel mill (there is only one on the whole map). I have a rail line feeding the steel to the ship line that does the final delivery. Close to the steel mill, there is a bridge in each direction over a river. The length is one tile plus the bridgeheads. There are one-way PBS on all four bridgeheads. When a train enters the bridge, all is fine. When it leaves the bridge, it brakes in front of the signal on the bridgehead.
Strangely enough, it does not happen always. The next trains on the bridges in before_crash.sav (trains 2 and 4) will show this behavior, but trains 1 and 3 (which are the first ones to pass the bridges in the autosave), will continue normally.

[cirdan]

Oh, I see it now. It happens at random and only on one-tile-long bridges. (That is why I never saw it--I always used longer bridges for testing). Here is a fix.

[3298]

Thanks, you are really fast at finding and fixing the cause of such obscure bugs. The trains behave as expected.
Also, I tried to be faster than Supercheese (apparently with success, despite the awful speed of MinGW on Wine) at producing a Windows binary. If I did everything right, it should have the same additional modification as his binaries, i.e. an increased NewGRF limit.
PS: The forum only allows attachments smaller than 6MB, and my zip was 6.9MB, so I repackaged it as 7z. That shouldn't be a big problem, though.

[AlexG]

God bless you!
Thank you for the great job that you did with this feature and for sharing it with ottd players and thanks for the binaries.

I have a small curiosity...could it be possible to do the same thing with other versions of ottd (exemple: 1.2.3 or the future 1.3.0) ?

[ChillCore]

You can but there is absolutely no point in doing that.

1. Binaries are not compatible anyway.
2. New features are only added to trunk.

[cirdan]

I have a small curiosity...could it be possible to do the same thing with other versions of ottd (exemple: 1.2.3 or the future 1.3.0) ?

Backporting the patch to 1.2.3 would be a lot of work (undoing a whole year of merges). Keeping the patch applying against 1.3.* would be easier, since that branch forked off trunk only recently, but it would be more work for me, depending on the amount of bug fixes that have to be ported (or for whoever does the porting).

(Also bear in mind that such requests are generally not looked upon favourably around here.)

[AlexG]

Ah ok I should have known that
Thanks for reply!

[Alberth]

The label "1.2.3" (or whatever label you start with) means that the program behaves in a certain way to what happens in the game.

Thus, when you take release 1.2.3, and change it by adding a patch, it's not release 1.2.3 any more, since at the point where you patched it, its behaviour has changed.

In other words, the very act of adding changes to a release causes it to lose its label, defeating any purpose of changing a release.

[Supercheese]

Thanks, you are really fast at finding and fixing the cause of such obscure bugs. The trains behave as expected.
Also, I tried to be faster than Supercheese (apparently with success, despite the awful speed of MinGW on Wine) at producing a Windows binary. If I did everything right, it should have the same additional modification as his binaries, i.e. an increased NewGRF limit.
PS: The forum only allows attachments smaller than 6MB, and my zip was 6.9MB, so I repackaged it as 7z. That shouldn't be a big problem, though.

Yes, you were indeed faster.

[3298]

Found another instance of the mass-replace missing a ReverseTrackdir (like with my trains in stations on page 4): Ships exit the wrong side of a shipyard. Fix: In src/pathfinder/yapf/yapf/yapf_ship.cpp, line 127 (last line of CheckShipReverse), insert a ReverseTrackdir around pos.td, making the line look like this:

Code: Select all

return best_trackdir == ReverseTrackdir(pos.td);