News:

The Forum Rules and Guidelines
Our forum has Rules and Guidelines. Please, be kind and read them ;).

[r9170] crash when replacing a railway signal with another type

Started by THLeaderH, July 20, 2020, 03:18:23 PM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

THLeaderH

If you try to put a railway signal A on a tile where the railway signal B already exists, simutrans should deny the placement and warn that there are already a signal. However, in r9170, simutrans crashes when a player try to do so. This crash cannot be seen when you try to put a road sign where another type of road signs already exists.

The direct cause of the crash is an assertion failure caused by simmenu.cc:L1092. As far as I know, two_click_t::work() should not be called if two_click_tool_t::is_valid_pos() returns status 0. In this case, two_click_tool_t::is_valid_pos() correctly returns 0 because there is already another railway signal on the tile, but two_click_t::work() is called and that causes the assertion failure.

kierongreen

Confirmed - investigating

Edit 1: bug introduced in commit 9154

kierongreen

Committed a fix in 9171 - simply removing the assert added in 9154.

There's a distinction here between check_pos and is_valid_pos. tool_t::work should only be called if tool_t::check_pos doesn't return an error. For roadsign_t (which includes signals) there is no check_pos so two_click_tool_t::check_pos is called, which also doesn't exist so tool_t::check_pos is used:

const char *tool_t::check_pos(player_t *, koord3d pos )
{
    grund_t *gr = welt->lookup(pos);
    return (gr  &&  !gr->is_visible()) ? "" : NULL;
}

As this just checks whether there's not an invisible ground at this location this returns NULL so work goes ahead which then triggers the assertion when the more detailed checks in is_valid_pos are made.


I believe this was just an oversight when the original patch was made hence am just removing the assert.


As an aside the commit 9154 had the explanation

r9154 | dwachs | 2020-06-25 17:47:25 +0100 (Thu, 25 Jun 2020) | 1 line

FIX: correctly null error messages in case of success (link factory tool affected)


It's worth noting however that in case of link factory tool failure currently no error message is generated - possibly something like below would handle cases when it's not used on a factory, however it might be desired for an error message to be displayed when a link cannot be formed or removed as well.

===================================================================
--- simtool.cc    (revision 9170)
+++ simtool.cc    (working copy)
@@ -5705,7 +5705,7 @@
{
     fabrik_t *fab = fabrik_t::get_fab( pos.get_2d() );
     if (fab == NULL) {
-        error = "";
+        error = "No factory";
         return 0;
     }
     return 2;

Dwachs

The assert was there for a reason: to enforce that is_valid_pos gives consistent return values. I do not see anything wrong with the code in is_valid_pos, will check.
Parsley, sage, rosemary, and maggikraut.

kierongreen

To ensure that is_valid_pos returns consistent values it needs to be either returning error == NULL and value > 0 or errror != NULL and value == 0. To enforce that you could use assert((error == NULL)  ==  (value > 0))

This would be truth table:

error == NULLerror != NULL
value == 0falsetrue
value > 0truefalse

With assert(error == NULL  ||  value > 0) we get this truth table though:

error == NULLerror != NULL
value == 0truefalse
value > 0truetrue

Dwachs

Parsley, sage, rosemary, and maggikraut.

kierongreen