News:

Simutrans Wiki Manual
The official on-line manual for Simutrans. Read and contribute.

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

I'm afraid but you also have to face with many of the problems that your patch causes in Extended.

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.