[PATCH] Fix demasking of existing signals during new signals preview

[ij]

While debugging double diagonal signal issue, I came across bug in the existing code which corrupts signal masks and flag bits during signal building using drag interface.

I'm bit unsure if dir != ribi_t::keine check in the destructor is really safe in all conditions, that is, is there a case where a valid signal/roadsign could have its dir set to ribi_t::keine.


--
i.

[Ters]

I wonder if it would be better to alter the behaviour of set_dir somehow. The code wasn't the most obvious to begin with, and doesn't get any better with this fix. There are multiple ways of modifying the behaviour of set_dir, such as adding a preview flag (there is an unused bit between automatic and ticks_ns) or special subclasses for preview signs and signals.

[ij]

I wonder if it would be better to alter the behaviour of set_dir somehow. The code wasn't the most obvious to begin with, and doesn't get any better with this fix. There are multiple ways of modifying the behaviour of set_dir, such as adding a preview flag (there is an unused bit between automatic and ticks_ns) or special subclasses for preview signs and signals.

I think that the main design decision is about whether to undo or not, and your response doesn't indicate any opinion on that point. I mean, whether the set_dir code or its caller should be undoing something happening outside the roadsign_t class. The current non-patched code shoots itself to its foot by trying to do that. The undoing, while possible, breaks the encapsulation as then set_dir (or its caller like in the current code) needs to be very much aware of the weg_t internals and also the dynamic state (the other roadsigns) in order to restore the state correctly during such undo.

I can certainly replace the set_preview_dir function with a flag to set_dir with the identical effect (skipping the weg_t side-effect things in set_dir+restoring dir afterwards) and/or add the preview bit to the class to be sure about the ribi_t::keine case, however, I think that the undoing things in the other class' state approach is a bad mistake (if that's what you meant?) and not even needed here really.


--
i.

[ij]

...Something along these lines...

[Ters]

I mean, whether the set_dir code or its caller should be undoing something happening outside the roadsign_t class.

I don't understand what you mean. set_dir doesn't undo anything, it does. The destructor undoes whatever affect the sign/signal had on the world, so that no other code has to think about what effect a sign/signal has on anything. In fact, the code "calling" the destructor might not even know it's a sign or signal at all.

[ij]

I don't understand what you mean. set_dir doesn't undo anything, it does. The destructor undoes whatever affect the sign/signal had on the world, so that no other code has to think about what effect a sign/signal has on anything. In fact, the code "calling" the destructor might not even know it's a sign or signal at all.

Your are half right and half wrong.

See the what the caller of the set_dir does, it recalls second time into set_dir in order to "fix" the issue first set_dir caused (that second call is even in the patch, removed)! That second call to set_dir is commented with a word of warning but even that call fails to get the flags set correctly although other code might behave correctly (I've not checked this) with a signal having ribi_t::keine mask regardless of SIGNAL flag!

The destructor tries to undo something more but it will fail since it tries only to undo (i.e., it won't consider pre-existing signal in the world in the same tile) rather than force recalculation all the modified bits. Those modified bits actually reside inside another class weg_t (which is not that nice encapsulation and all this just sort of proves how complicated it gets and easily causes bugs).

--
i.

Edit: the signal/sign is destructed by the preview, not remaining as I said first (although with non-patched code the destructor still incorrectly clears the SIGN/SIGNAL flags regardless of pre-existing signals).

[Ters]

See the what the caller of the set_dir does, it recalls second time into set_dir in order to "fix" the issue first set_dir caused

Yes, I see that. And if set_dir didn't cause those issues in the first place, there would be nothing to undo when previewing (except remove the sign/signal from the map when previewing is over). Your second patch seems to have done this.

The destructor tries to undo something more but it will fail since it tries only to undo (i.e., it won't consider pre-existing signal in the world in the same tile) rather than force recalculation all the modified bits. Those modified bits actually reside inside another class weg_t (which is not that nice encapsulation and all this just sort of proves how complicated it gets and easily causes bugs).

It won't consider a pre-existing signal, because apart from previewing (a relatively new feature) there can't be two signs/signals on the same way tile. If preview signs/signals simply stopped modifying and unmodifying ways, it's no problem that a sign/signal removes the signed/signaled state from the way. For the flags, the destructor could choose to only remove those bits in the mask it itself set (still assuming previews don't tamper with them), but you'd need to turn the has_signal flag into a reference counter, or spend time iterating through all objects on the tile to check for other signals. Both are more costly. As for encapsulation, either the way has to know how to adapt to each type of sign and signal, or the signal has to modify the way to behave the way it wants. The former is also more expensive, since it is faster for signals to look up ways, than for ways to look up signals.

[ij]

Yes, I see that. And if set_dir didn't cause those issues in the first place, there would be nothing to undo when previewing (except remove the sign/signal from the map when previewing is over). Your second patch seems to have done this.

It won't consider a pre-existing signal, because apart from previewing (a relatively new feature) there can't be two signs/signals on the same way tile. If preview signs/signals simply stopped modifying and unmodifying ways, it's no problem that a sign/signal removes the signed/signaled state from the way. For the flags, the destructor could choose to only remove those bits in the mask it itself set (still assuming previews don't tamper with them), but you'd need to turn the has_signal flag into a reference counter, or spend time iterating through all objects on the tile to check for other signals. Both are more costly. As for encapsulation, either the way has to know how to adapt to each type of sign and signal, or the signal has to modify the way to behave the way it wants. The former is also more expensive, since it is faster for signals to look up ways, than for ways to look up signals.

I think we pretty much agree on everything, just with quite many words :-). My point about encapsulation was mostly just about the preview wanting something else than set_dir interface provided and then that caller trying to get the damage undone (in the non-patched code). Anyway, thanks for taking look on the patch(es).

--
i.

[prissi]

So what conclusion: Does this patch fix something or not?

[ij]

So what conclusion: Does this patch fix something or not?

Definately. I don't think it was questioned by Ters at all, the main discussion with Ters was about which way to implement the fix.

The bug is quite easy to reproduce:

   
• Add a straight track and build a signal with only one allowed direction on that track.
• Drag signal builder over/across that signal and retract before building.
• Check the mask of the tile with the old signal (it gets cleared).

...but I can hardly convince anyone beyond this with reasonable effort.

--
i.

[Ters]

The bug is quite easy to reproduce:

   
• Add a straight track and build a signal with only one allowed direction on that track.
• Drag signal builder over/across that signal and retract before building.
• Check the mask of the tile with the old signal (it gets cleared).

One more thing: the starting point for the drag and the signal spacing must cause a signal to be previewed on the tile contained the pre-existing signal. A signal spacing of 1 is probably the easiest.

So what conclusion: Does this patch fix something or not?

I haven't actually tried to build Simutrans with the patches (not yet at least). From just reading the patches, both seem to do something that cures the bug, although I find the first more complicated, by building on the hack-ish code that caused the problem to begin with, rather than make preview signals aware that they are just that and stop messing around with the world, like the latter.

[ij]

I haven't actually tried to build Simutrans with the patches (not yet at least). From just reading the patches, both seem to do something that cures the bug, although I find the first more complicated, by building on the hack-ish code that caused the problem to begin with, rather than make preview signals aware that they are just that and stop messing around with the world, like the latter.

From my point of view, either of them is ok. The v2 one costs the eight (unused) bit, whereas the first one had to overload to ribi_t::keine meaning in order to skip world modifications in the destructor. Both versions avoid messing with the world while previewing.

I've tested both versions myself and both do cure the bug (but feel free to test yourself too if my words aren't convincing enough ).

--
i.

[prissi]

Incorporated v2 ..