supermop wrote:Could the tool work more like the rail tool itself? Or maybe 'building' monorail over rail has the same effect as the convert tool? Or is that out of the scope of the patch?
It is not that it is out of the scope of the patch, it is simply that I'd rather address it later. First we should make sure that two railtypes per tile work and that the code has no bugs, and then we can discuss how to extend the existing tools to cope with the new possibilities.
cirdan wrote:Warning: HUGE patch
Indeed. There are two things I though after seeing that the combined diff is ~1.3MB. First one was "Who the hell would ever review that?" and the second was "Maybe the chosen approach is, ehh, suboptimal".
That is possible. I've been working on this patch for long, and I still consider it a work in progress (although some parts are already quite stable). I still have some ideas to better organise the sequence of patches, but I didn't want to hold the patch any longer because I thought it would be good to expose it for public review and, more importantly, to receive wider testing.
Michi_cc wrote:Of course, I just took some very tiny peeks so far, so I don't have any idea if the approach really is in any way suboptimal or not. Maybe you can write a few words about your chosen design.
As it usually happens, it all started with an underestimation. I thought that the main problem with not having two railtypes per tile, custom bridgeheads, custom tunnelheads, etc. was the constraints of the current map array, which makes it awkward to do any significant change because it is the result of unplanned expansion on top of the legacy TTD implementation and, as such, has a fairly inefficient representation. So I though that designing a new map array, better packed and planned in advance would go a long way into implementing all those things that I wanted.
Well, I was wrong. Rewriting the map array allowed me to implement two railtypes per tile very easily (in only 5 patches), but that was it. Custom bridgeheads and tunnelheads, although they badly required the map array rewrite as well, were still very far away. Adding custom road bridgeheads required lots of changes in the pathfinders and the vehicle controllers. Adding custom rail bridgeheads required additional changes in the signal code. And custom tunnelheads is something that, while taken into account into the new map array, still hasn't seen any actual work. If I had known how much work it would take to get here, I probably wouldn't have undertaken the task.
I'm attaching the map array I designed and which is (mostly) implemented in my patch. The file is gzipped as HTML files cannot be directly uploaded.
In my own map array rewrite experiments, I only need about 470KB of diff to arrive at multiple railtypes with multiple owners on one tile. I can't show any custom bridgeheads so far, because I've focused a lot more on the ground work with the intention to someday for example even have proper signals on bridges and tunnel (not like in some those current signals-in-tunnel hack patches). Who knows how big my patch would grow till that
In my patch sequence, having two railtypes per tile is achieved in patch 143/332, and the diff is 800K, which is closer to what you get. And there's a lot of code shuffling between files and changes that are there to make things easier down the road.
As for proper signals on tunnels and bridges, this is something that I've thought about. My map array rewrite has room for true signals on bridges, given some constraints, although I haven't coded them yet. For proper signals in tunnels, I'm afraid that we'll have to settle with a hackish implementation, if any, because a tile can have several tunnels underneath and there is no way we can store signal information for an arbitrary number of levels in the map array, especially not if we ever implement custom tunnelheads (which would reduce the available bits for tunnel data) or more height levels (which would increase the possible number of height levels under a tile).
(On a side note, my map array rewrite completely frees 2 bits per tile for all tiles, which could be used for storing more height levels.)
Michi_cc wrote:I don't want to demotivate you, but a 323-strong patch series, is, as Germans say, "eine Menge Holz". Maybe that's what it takes in the end, but I'd advise you to try looking for opportunities to cut down on the patch.
Believe me, I don't like bloat, either. But I truly think that this is what it takes. Note, however, that I am very keen on having patches as small as possible, so I may split what could be one single patch into four or five patches, to make my life easier when rebasing; this could partly explain the sheer number of patches. Also, there are a number of patches whose only purpose is to clean up code which I could very well not touch (the ship controller, for instance--ship code could really use some caring). So the 332 patches may not be as bad as it seems.
If you feel like reviewing some patches, though, I suggest that you try reviewing the first ten patches or so in the sequence. They are mostly optimisations and code cleanups (one of them just fixes a typo), and they should be fairly uncontroversial and improve the codebase whether the rest of the sequence is applied or not.