News:

Simutrans Forum Archive
A complete record of the old Simutrans Forum.

Bug: Station signals not properly recognized

Started by Ves, May 20, 2019, 07:52:14 AM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

Ves

Hi James,
poking around in the signal.cc I found that a station signal everywhere where identified as:

if(desc->is_longblock_signal() && (desc->get_working_method() == time_interval || desc->get_working_method() == time_interval_with_telegraph || desc->get_working_method() == absolute_block))
However, at least two places in signal.cc the the last working method (absolute block) was forgotten, potentially leading to bugs.
That being, of corse, if that was not on purpose, but judging from the code around it, it seems to not be the case.

So I took the liberty to make this new entry in roadsign_desc.h:
// This is currently the definition of a station signal
bool is_station_signal() const {return is_longblock_signal() && (get_working_method() == time_interval || get_working_method() == time_interval_with_telegraph || get_working_method() == absolute_block);}

and changing the checks in signal.cc to desc->is_station_signal() instead.

https://github.com/VictorErik/Simutrans-Experimental-Ves/tree/signal-bug

What file is the famous "block reserver" residing in? Perhaps that could benefit from this redo as well.

jamespetts

Thank you for that - I must have forgotten about adding the ability to have absolute block station signals. This is probably a better way of organising the code, so I have incorporated this.

The block reserver is in simvehicle.cc.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

Ves

Im amazed by the complexity of the block reserver! It is truly massive and very impressive that you have been able to cope with all that, let alone being able to debug it!
I found only one instance of a station signal in there that I felt secure enough to change, but alot of instances where the parameters where nested into each in "if/else" other and it was in no way clear if it was station signals in general or specific station signals with the time_interval working methods.

The new commit is on the branch I mentioned in the OP!

jamespetts

Thank you very much for that: that is most helpful. I have now incorporated this.

The complexity of the block reserver is not good code design, and I should not have done it that way had I realised how complex that this would have needed to have been: I started with the premise that the existing signalling code already worked very well for cab signalling, so it should not be too hard to extend that code to work with the other methods. In hindsight, it would have been better to have started from scratch and built a more modular system, which would have been easier to maintain and extend and more reliable.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

Ves

Great!
Incidentally, how difficult would you think it might be to rebuild that?
Reading through the code it looks like it gets a list of coordinates which make up its route, and then it checks what is on each coordinate, wether there is a signal or something else.

jamespetts

Quote from: Ves on May 21, 2019, 10:04:55 PM
Great!
Incidentally, how difficult would you think it might be to rebuild that?
Reading through the code it looks like it gets a list of coordinates which make up its route, and then it checks what is on each coordinate, wether there is a signal or something else.

What do you mean by "rebuild that"? If you mean totally refactor the whole signalling code, I should imagine that this would take a very large amount of work indeed, as there is great complexity that needs to be simulated in very precise ways.

The route is indeed a list of co-ordinates, and the basic mode of operation of the block reserver is indeed as you describe, in very simplified terms indeed.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.