[DRAFT PATCH] half-height bugs in the bridge building code

[Philip]

I'm running into problems with the bridge building code using half-height tiles (I'm using a nightly build of Simutrans standard and pak128.Britain 1.15) that I'm convinced are due to bugs in the code in brueckenbauer.cc, particularly finde_ende. Unfortunately, they're a little complicated to describe and fix, so I can't offer a patch that's ready to be applied, yet. I have attached a patch that hopefully demonstrates what's going wrong and how one might go about fixing it. The initial symptom is that in this block of code in baue_bruecke.cc:

Code Select Expand


if(  need_auffahrt  ) {
// not ending at a bridge
baue_auffahrt(sp, end, ribi_typ(-zv), gr->get_grund_hang()?0:hang_typ(-zv)*(pos.z-end.z), besch);
}


(pos.z-end.z) is 3, resulting in an invalid weg_hang and more problems down the line: bridge ramps can have height 1 or 2 in half-height tile sets, but never 3.

The first screenshot is just before the problem appears: it's important to build the bridges in that order, first the E-W bridge, then the N-S bridge starting at the N tile. The second screenshot is just after (while the real problem is an invalid weg_hang, the graphics don't work out particularly well either).

The problem is that finde_ende isn't strict enough when checking whether a bridge can be built: it checks that every tile individually is compatible with a bridge height of 1 or 2, but doesn't ensure that only one of those heights can be used for the entire bridge. If the low option and the high option are blocked at different tiles, no bridge should be able to be built, but finde_ende erroneously returns success.

A simple fix would be to call finde_ende twice, once for the low bridge option and once for the high bridge option, but that might cause performance problems when looking for the nearest end tile for a bridge. So I've started on the more complex task of fixing finde_ende without changing its behaviour: instead, it returns the suggested bridge height, defaulting to a high bridge if both possibilities are valid. That bridge height must then be passed to baue_bruecke for the right height bridge to be built.

If requested, I can submit the simpler fix as well.

Again, the patch I'm attaching hasn't been tested enough, so I wouldn't consider it for inclusion just yet. However, it would be great to get some feedback on whether to continue working on this or letting a more experienced developer fix it themselves.

Two more notes on the patch: I've tried to write the code so it would be easy to convert to triple/quadruple-height bridges, which I think it would be an improvement to require over large canals or rivers to let ships through.  I've also incorporated a minor change to simcity.cc that lets cities build bridges much more successfully, without which there are hardly any city-built bridges:

Code Select Expand


- if(err==NULL  &&   koord_distance( k, end.get_2d())<=3) {
- brueckenbauer_t::baue_bruecke(NULL, bd->get_pos(), end, zv, bridge, welt->get_city_road());
+ if((err==NULL||*err == 0)  &&   koord_distance( k, end.get_2d())<=3) {
+ brueckenbauer_t::baue_bruecke(NULL, bd->get_pos(), end, zv, bridge_height, bridge, welt->get_city_road());


[prissi]

I think that the routine finding correct ends is indeed quite difficult, since it need to handle vertical slopes where it should not build a bridge but just a ramp.

But I cannot reproduce your error. I do not manage to built the first bridge, neiter in pak128.britain nor in pak64.

[Philip]

I think that the routine finding correct ends is indeed quite difficult, since it need to handle vertical slopes where it should not build a bridge but just a ramp.

You're right, and that's why the patch really needs more testing. However, if you actually look at the existing code, it is clearly broken.

But I cannot reproduce your error. I do not manage to built the first bridge, neiter in pak128.britain nor in pak64.

Are you sure you're using pak128.britain version 1.15? Are there any other paksets that already support half-height tiles that I could try (pak64 doesn't appear to have them?) Also, are you using standard settings? Anyway, I've attached a save game with the first bridge built, but not the second one.

Thanks for your responses!
Philip

[prissi]

pak64 supports half heights quite well. Not all bridges support them, but quite a few road bridges do.

I rewrote this routine just for the many cases with pak64 bridges for testing. But I have to admid that I never though on using double height starting slopes. That are not included in pak64 bridges.

When last discussing bridges I though the agreement was that these double height starting slopes will not be used.

[kierongreen]

Bridges have the option of 2 heights when starting from a slope (if only one is defined then that is used for both steep and shallow slopes) and 2 heights when starting on the level (if only one is defined then steep ends are prohibited).

[Philip]

Bridges have the option of 2 heights when starting from a slope (if only one is defined then that is used for both steep and shallow slopes)

I don't believe that's entirely correct for the current code: if only one bridge height is defined, you cannot start the bridge on steep slopes, because of this code in finde_ende:

Code Select Expand


if(  hang_t::max_diff(slope)==2  &&  !besch->has_double_start()  ) {
error_msg = "Cannot build on a double slope!";
return koord3d::invalid;
}


That simplifies the code somewhat: we only have to check two options if the bridge starts on a flat tile. Are you suggesting that this might need changing, and that I thus need to change the code to remove that check?

I've modified the code to build ramps connecting to half-height vertical walls (rather than building a full-height up ramp directly followed by a half-height down ramp), and to handle correctly ramps connecting to full-height vertical walls, I believe. The new patch is attached.

One oddity that is still in the code is that way height clearance isn't enforced above bridges, so you can build (on a flat map) a low bridge and a high bridge crossing each other. Is that intentional? I'd favour changing that, and also enforcing clearance as measured above the highest corner of a tile rather than the lowest, but there might be gameplay reasons for not doing so.

Thanks for the clarifications, in any case!
Philip

[kierongreen]

I don't believe that's entirely correct for the current code: if only one bridge height is defined, you cannot start the bridge on steep slopes, because of this code in finde_ende:

Ok that must have been added at some point.

Clearance should be enforced above and below I probably just didn't think of it when adding double heights!

[Philip]

Clearance should be enforced above and below I probably just didn't think of it when adding double heights!

Okay, I believe this version of the patch does that, and also simplifies more of the code. Sorry it's so long, but a lot of the code did require modification. The main changes are:

• fix the triple-height weg hang error, by keeping track of valid bridge heights
• actually check way height clearance when building bridges
• allow building bridges down from elevated roads rather than only up to elevated roads
• allow building bridges up to slopes rather than only down from slopes
• prohibit bridges linking one elevated road to another
• build half-height ramps when connecting to half-height vertical walls rather than full-height up ramps followed by a half-height down ramp
• build city bridges when finde_ende returns a "" error message, rather than only when it returns NULL

The things I'd still like to change, but that can wait until the main part of the patch (or something equivalent if it's rejected) goes in:

• build low bridges with ctrl, when we can do so
• check way height clearance above the highest corner of a way tile, not the lowest

I'm still testing this patch, but it seems less buggy than the code in the SVN trunk, at least.

[DrSuperGood]

•prohibit bridges linking one elevated road to another

Does this mean you cannot connect two elevated ways with a bridge between them? I would have thought that behaviour may be useful, as long as any required art assets were present to avoid buggy visuals.

It would also be useful if the height of bridge built is based on the clearance. When crossing a valley with artificial walls (a trench?) on the edges it should not build an up ramp at all and instead bridge straight across. When building over flat ground or a low obstacle it should use a shallow up ramp. When building over a taller obstacle (where a shallow ramp would not suffice) then it should build a double ramp. Basically it tries to build the minimum elevation of bridge possible for the crossing between points. The code could eventually be extended so that multiple consecutive up ramps could try to be built when elevations above what is possible with a single ramp are needed to cross between points (think of a bridge over a city).

[Philip]

Does this mean you cannot connect two elevated ways with a bridge between them? I would have thought that behaviour may be useful, as long as any required art assets were present to avoid buggy visuals.

The code currently requires that at least one end of each bridge either (a) starts on a slope or (b) has a ramp. The problem with doing things otherwise is that if you connect two elevated roads with a bridge, then remove the elevated ways, you end up with a bridge tile hanging in mid-air that you cannot remove. You can effectively build a bridge between two elevated roads by terraforming the first tile to be a slope facing in the right direction (see screenshot).

It would also be useful if the height of bridge built is based on the clearance. When crossing a valley with artificial walls (a trench?) on the edges it should not build an up ramp at all and instead bridge straight across.

Again, such a bridge couldn't be removed. In fact, it couldn't even be built, since you'd have to move the cursor to the mid-air position where you'd want to start the bridge. You can build it by replacing one of the trench wall tiles with a slope, though again you'd waste a tile.

When building over flat ground or a low obstacle it should use a shallow up ramp. When building over a taller obstacle (where a shallow ramp would not suffice) then it should build a double ramp. Basically it tries to build the minimum elevation of bridge possible for the crossing between points.

The problem with building bridges low by default is that if the way height clearance is set to 2, you can't build a road underneath a low bridge, so you'd have to remove the low bridge, build the road, then reconstruct the bridge (as a high bridge), which would get annoying very quickly. It's better to build high bridges by default if we can, at least if the way height clearance is set to 2.

The code could eventually be extended so that multiple consecutive up ramps could try to be built when elevations above what is possible with a single ramp are needed to cross between points (think of a bridge over a city).

Indeed, and I've written my patch with that future expansion in mind—we have an array of "okay" heights, for example, rather than variables for the low bridge and high bridge options.

ETA: I should have been clearer that I don't disagree with those extension requests, it's just that I think the patch should go in without them (or be rejected and have something equivalent applied). I still think high bridges should be the default, but that low bridges should be buildable; right now, the state of the code is that you cannot build a low bridge without constructing a conflicting bridge first, then demolishing that bridge afterwards, which is a nasty trick.

[Leartin]

The code currently requires that at least one end of each bridge either (a) starts on a slope or (b) has a ramp. The problem with doing things otherwise is that if you connect two elevated roads with a bridge, then remove the elevated ways, you end up with a bridge tile hanging in mid-air that you cannot remove.

If you remove the elevated way or slope wall where the bridge is supposed to end, it always results in something awkward that has no reason to exist, a bridge into nowhere. I think that should not be allowed in the first place, removing that tile should always deconstruct the bridge. Another solution would be to delete a bridge by using the delete tool on any part of it. Either method is better than restricting bridges. Actually, I'd rather have to use a workaround to deconstruct some levitating bridge tiles (reconnect them to an elevated way, then remove them) then have to use a workaround to connect two elevated ways that will always be visible.

Again, such a bridge couldn't be removed. In fact, it couldn't even be built, since you'd have to move the cursor to the mid-air position where you'd want to start the bridge. You can build it by replacing one of the trench wall tiles with a slope, though again you'd waste a tile.

It currently can't be buildt, but that's the request here, and it makes a lot of sense.

you'd have to remove the low bridge, build the road, then reconstruct the bridge (as a high bridge), which would get annoying very quickly.

No, it wouldn't. You don't build bridges on flat terrain for no reason, and if you build them on slopes, the height is decided by the slope already. If you want to build a bridge in order to cross another way, build the other way first. If you build a bridge for some other reason and want to go under it later, which I don't think is too common, you'll have to deconstruct the bridge - however you'd have to deconstruct a normal way tile as well if you did not build a bridge in the first place (again, why is there even a bridge if not to cross something?) so it's not like it get's more annoying then each and every change of a way is already.

[prissi]

On my short test with several pak64 bridges the patch seems to work properly. Thank you very much.

[DrSuperGood]

You could also raise other player's bridges when dragging a way under them. May be awkward initially but would solve some issues. Problem is this would probably need changes to the other way type code so may not be worth it.

Could you possibly look at elevated roads please? They also suffer from incorrect build behaviour in the form of dragging an elevated way over another player's stop. If you own the stop you can do this, but if another player owns it (and the stop has clearance, it is low or even flat) then it will refuse to build elevated ways over it. This has been bugging experimental and standard pak128 for ages. Not exactly related to bridge building but it is related to incorrect construction logic (probably some condition somewhere).

There is also the opposite case where towns refuse to acknowledge the existence of bridges and elevated ways in the form of them not making houses under them (bridges can be made over houses but no new houses spawn under them in the case of a bridge over clear land) and upgrading of houses so that clearance rules are violated (a low house upgrades to a high house invalidating the above elevated way/bridge yet it still operates fine but the way cannot be replaced. Basically a consistency with buildings and height restrictions.

Maybe these would best be suited to some other topic.

[Philip]

If you remove the elevated way or slope wall where the bridge is supposed to end, it always results in something awkward that has no reason to exist, a bridge into nowhere. I think that should not be allowed in the first place, removing that tile should always deconstruct the bridge. Another solution would be to delete a bridge by using the delete tool on any part of it. Either method is better than restricting bridges. Actually, I'd rather have to use a workaround to deconstruct some levitating bridge tiles (reconnect them to an elevated way, then remove them) then have to use a workaround to connect two elevated ways that will always be visible.

It currently can't be buildt, but that's the request here, and it makes a lot of sense.

No, it wouldn't. You don't build bridges on flat terrain for no reason, and if you build them on slopes, the height is decided by the slope already. If you want to build a bridge in order to cross another way, build the other way first. If you build a bridge for some other reason and want to go under it later, which I don't think is too common, you'll have to deconstruct the bridge - however you'd have to deconstruct a normal way tile as well if you did not build a bridge in the first place (again, why is there even a bridge if not to cross something?) so it's not like it get's more annoying then each and every change of a way is already.

I've thought about this for a while, and I now believe you're right. Defaulting to the lowest possible bridge, and making that a "trench bridge" when we can, seems to work quite well, even with tunnels.

You could also raise other player's bridges when dragging a way under them. May be awkward initially but would solve some issues. Problem is this would probably need changes to the other way type code so may not be worth it.

Could you possibly look at elevated roads please? They also suffer from incorrect build behaviour in the form of dragging an elevated way over another player's stop. If you own the stop you can do this, but if another player owns it (and the stop has clearance, it is low or even flat) then it will refuse to build elevated ways over it. This has been bugging experimental and standard pak128 for ages. Not exactly related to bridge building but it is related to incorrect construction logic (probably some condition somewhere).

There is also the opposite case where towns refuse to acknowledge the existence of bridges and elevated ways in the form of them not making houses under them (bridges can be made over houses but no new houses spawn under them in the case of a bridge over clear land) and upgrading of houses so that clearance rules are violated (a low house upgrades to a high house invalidating the above elevated way/bridge yet it still operates fine but the way cannot be replaced. Basically a consistency with buildings and height restrictions.

I haven't been able to reproduce this behaviour: I can build elevated ways over level 2 stops (no matter whether they belong to me or another player) but not level 3 stops (again, in both cases). However, it does seem odd that you can build the stop after building the elevated way and all's fine. I think, but I'm not totally sure, that the building rules should be symmetric.

[Philip]

The code for Standard, with some new bug fixes, is at https://github.com/pipcet/simutrans-experimental/compare/aburch:master...standard-low-bridges . Please let me know if you want only part of the patch or to suggest changes, I'll be happy to make any requested modifications.

In particular, the code allowing bridges to connect to tunnels directly is isolated in one commit, so you can hopefully revert that if you don't like that feature (I do, though! If we add natural vertical slopes and waterfall tiles we could simulate Switzerland.)

Thank you again for the comments and review.

[prissi]

Sorry for the delay. Comitted in r7337. Thank you.

It my cause some issues in pak128.britain, as this is the only one where a real choice of bridges is available. But in (my short) tests it worked as intended.