Terkhen wrote:

I attached an old patch file by mistake, use this one instead:

Attachments

• train_reorganization.diff (20.41 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/3524#comment7346

Hirundo wrote:

I think this is a wise step, to separate the codechanges to the current acceleration code and provide them as a separate patch. This should pave the way for future modifications
Some comments:
- (t->First())->SomeFunction() ==> t->First()->SomeFunction()
- in UpdateAccelerationCache(), you use slope_resistance to cache u->GetWeight() which seems a bit fishy to me. Using a separate variable looks nicer. Also, current_power could be declared inside the loop.
- bool IsMaglev() seems somewhat inflexible to me. http://vcs.openttd.org/hg/openttd/trunk.hg/rev/3a2854631c63 defines three types and with RVs it could be more. Perhaps RailTypeInfo::acceleration_type should be enumified. This might be a job for a separate, small patch.
- You add 2 empty lines to train.h
- What is the point of updating the acceleration caches of free wagon chains? They won't go anywhere before an engine is added, and then the cache is recalculated anyways :)

Apart from this nitpicking stuff I see no reason to not include this patch. But fortunately for OpenTTD, I'm no dev ;)

This comment was imported from FlySpray: https://bugs.openttd.org/task/3524#comment7347

Terkhen wrote:

- (t->First())->SomeFunction() ==> t->First()->SomeFunction()

All of them corrected.

- You add 2 empty lines to train.h

They have been removed.

- bool IsMaglev() seems somewhat inflexible to me. http://vcs.openttd.org/hg/openttd/trunk.hg/rev/3a2854631c63 defines three types and with RVs it could be more. Perhaps RailTypeInfo::acceleration_type should be enumified. This might be a job for a separate, small patch.

I have renamed GetAccelerationType (returns AM_BRAKE or AM_ACCEL) to GetAccelerationStatus, as its name conflicted with the new concept of acceleration types. This means the AccelType enum would need a name change too, but that should be included at that separate patch you mention. IsMaglev is now called GetAccelerationType and it returns an int.

- in UpdateAccelerationCache(), you use slope_resistance to cache u->GetWeight() which seems a bit fishy to me. Using a separate variable looks nicer. Also, current_power could be declared inside the loop.

My implementation of the cache code was broken in many ways, and reestructuring it should be another patch anyways. It has been divided into CargoChanged and PowerChanged, which mirror trunk behaviour correctly. This new implementation includes your suggested changes.

- What is the point of updating the acceleration caches of free wagon chains? They won't go anywhere before an engine is added, and then the cache is recalculated anyways :)

As a result of the last point, I have reverted all calls to cache updates to the way they were in trunk. I don't know why the update is necessary since what you say is reasonable, but it should be work for another patch anyways.

As always, thanks for a helpful review :)

EDIT: I just realized what part of the code you meant in your last point. It will be corrected soon.

Attachments

• train_reorganization_v2.diff (20.29 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/3524#comment7350

Terkhen wrote:

Removed an unneeded update of cargo caches for free wagon chains.

Attachments

• train_reorganization_v3.diff (20.24 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/3524#comment7354

Terkhen wrote:

Update to trunk (see r18812). Removed some whitespaces at the start of the line. Corrected some comments.

Attachments

• train_reorganization_v4.diff (16.13 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/3524#comment7367

frosch wrote:

Hmm, I guess GetArticulatedPower() would be better named GetPoweredWagonPower() or so. I cannot see any articulated-specific there.

I am wondering about those Get* functions. Can they be made protected or private. Most code should use the cached values.

[Edit: Removed some wrong statements]

This comment was imported from FlySpray: https://bugs.openttd.org/task/3524#comment7370

Terkhen wrote:

The spaces at the start of some comments were not corrected. GetArticulatedPower has been renamed to GetPoweredPartPower (I prefer not to give these functions train-specific names). I have made all FORCEINLINE Get* functions protected; as frosch said they should not be called outside acceleration code anyways. As a result, I had to convert UpdateTrainSpeed into a class method of Train. I can provide this conversion as a separate patch if that is preferred.

Attachments

• train_reorganization_v5.diff (17.74 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/3524#comment7372

Rubidium closed the ticket.

Reason for closing: Implemented

In r18838

This comment was imported from FlySpray: https://bugs.openttd.org/task/3524