News:

SimuTranslator
Make Simutrans speak your language.

Code refactoring of road vehicle overtaking

Started by THLeaderH, July 15, 2020, 05:56:18 AM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

THLeaderH

In the current simutrans code, convoi_t and private_car_t inherits overtaker_t. For road_vehicle_t, can_overtake_function() is called through convoi_t. I think this code structure has some problems.


  • Overtaking is the matter of the vehicle movement. The vehicle movements are generally handled by the classes derived from vehicle_base_t, but overtaking is currently handled by convoi_t.
  • convoi_t is the class of convoy and it should be way type independent. However, overtaking is a feature only for road vehicles.
  • convoi_t and private_car_t do not have a common ancestor class. Due to this, we have to write quite different codes for convoi_t and private_car_t.

I want to remove the overtaking related code from convoi_t and bring them to road_vehicle_t. road_vehicle_t should inherit overtaker_t instead of convoi_t. This makes all subclasses of overtaker_t being derived from vehicle_base_t, and the inheritance relationship of these classes becomes clearer.

If there is no problem for this code refactoring plan, I want to start the implementation. Please tell me your opinions.  :)

freddyhayward

For the congestion feature in extended, I implemented traffic_vehicle_t which is also inherited by both road_vehicle_t  and private_car_t. It would be good to adapt this refactor into extended too so that traffic_vehicle_t can be merged into overtaker_t. This would reduce the amount of duplicate code.

makie

Think about trailer, there can be one or more of it.
It is a  convoi that overtake

THLeaderH

In case of rail convoys, the leading rail_vehicle_t object of the convoy takes control of convoy movements. Thus, I think the leading road_vehicle_t objects should take the control of overtaking.

Ranran(Hibernating)

I'm afraid but you also have to face with many of the problems that your patch causes in Extended.
(´・ω・`)シミュトランスのアップデート履歴(日本語) (※更新停止中)
bit.ly/3AuKHHP

makie

Quote from: THLeaderH on July 15, 2020, 08:06:47 AM
In case of rail convoys,
Not rail train. I mentioned, trailers of trucks on the road, there can be also more than one.

freddyhayward

Quote from: makie on July 15, 2020, 10:35:41 AM
Not rail train. I mentioned, trailers of trucks on the road, there can be also more than one.
The issue is that there is only one convoi_t class, so overtaker_t is currently inherited by all convoy types, including trains, planes and boats.

THLeaderH

Yesterday I tried the implementation to bring can_enter_tile() to road_vehicle_t. However, convoi_t still has to call set_tiles_overtaking() in some places and I could not exclude overtaking related code from convoi_t. Also, I could not reduce the size of can_enter_tile() function with this refactoring. Although road_vehicle_t should inherit overtaker_t instead of convoi_t, the benefits of this refactoring is not so huge. So it seems that we do not have do this refactoring.

The refactoring patch is attached just for reference.

prissi

I think this is more logical than the current implementation.

THLeaderH

#9
I think this refactoring does not do a harm but does not bring so much benefit, so both accepting and denying this patch is appropriate.