Alberth wrote:

Use a common prefix for the constants, so it is easier to understand which constants are related to each other.
(That also makes the meaning of the value more clear, eg just SPEED can basically mean any speed in the game.)

Make one kind of change in one patch.
If you want to change these headers, make a separate patch at least, and most likely also a new issue number as this does not seem related to 'Remove magic numbers at callback 36'.
-uint GetEngineProperty(EngineID engine, uint8 property, uint orig_value)
+uint GetEngineProperty(EngineID engine, int property, uint orig_value)

PS Don't know whether these constants make sense, but another dev will know.

This comment was imported from FlySpray: https://bugs.openttd.org/task/3222#comment6703

frosch wrote:

The patch actually adds more constants, and moves vehicletype specific code into the vehicletype independent GetEngine/VehicleProperty(). Esp. the part about GetEngineProperty() makes things more bad IMO.

I guess it is better to nummerize all properties like PROP_{FEATURE}_{NAME} (e.g. PROP_TRAIN_CAPACITY). If code can be shared accross multiple vehicletypes, it should be moved into the Vehicle class, as it is already done for Engine. (everything IMO of course)

This comment was imported from FlySpray: https://bugs.openttd.org/task/3222#comment6708

Terkhen wrote:

Moving the code to Vehicle is better than the current solution. I will use a prefix for the enum too (PROP_ for example), but I can't define properties in different enums for each vehicle, as I want to be able to do something like this:

GetVehicleProperty(this, PROP_COMMON_PROPERTY, this->common_cache.common_property);

I think the best solution could be implementing a virtual method that get implemented in each vehicle class and calls GetVehicleProperty with the correct magical number for each property. This also removes all changes to GetEngine/VehicleProperty().

This comment was imported from FlySpray: https://bugs.openttd.org/task/3222#comment6713

Terkhen wrote:

After checking OpenTTD code again taking into account the feedback obtained here, I realized that merging the enums for properties has little use besides some particular cases (like the code unification I was implementing). As a result, I have just implemented frosch's suggestion. Each kind of vehicle has a enum with all the properties supported for callback 36. These values are used when calling GetEngineProperty and GetVehicleProperty instead of the magic numbers. The code is simpler now, and it doesn't alter existing functions parameters and behaviour.

Attachments

• magic_number_callback_36.diff (13.28 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/3222#comment6720

frosch closed the ticket.

Reason for closing: Implemented

in r17616

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