Alberth wrote:

Thanks for fixing the coding style issues, next step is to have a look at the actual patch.
I still like the idea that you have better control over what disasters are enabled.

* What is the use-case for querying the internal timer state from the console? I can see why you'd may want 'do disaster XYZ now', but why have the other commands?

* 'disaster list' is disabled if the cheat is off. The cheat description however says "Enable changing disaster internal values: ...". How is listing values a modification? (I do agree that the information should not be available without cheat, as I can adjust my plans based on that data.)

* When loading an old save game, all timers have the same value, which means they all go off at the same time. Doesn't sound like a good idea.

* Since you have several timers running now, did you re-balance the disaster frequency?

* For a 'framework', it seems a bit incomplete to me. There are plenty of other arrays in src/disaster_cmd.cpp that have a strong relation to a disaster. Why not make a class for all that information together? (probably a base class with a derived class for each specific disaster)

* In DisasterTimer::Startup() you instantiate a fixed number of disasters. I wonder why, how is this better than the array _disasters[] that we had?

* Also I don't exactly see why you need a dynamic SmallVector<> for a fixed number of timers.

* (and moving const _disaster[] data back to magic numbers in code seems the wrong direction to me, why do you do that?)

(Do you have a Java background? The patch code looks a bit Java-ish)

* You seem to save disaster information with a unique identifier. It looks like an awfully complicated way to save the information. You claim that is needed to add disasters later without save game version change. How, may I ask, are you going to do that, given the fact that you also introduce disaster settings, and these are saved as well?
In addition, we are a looong way from running out of savegame version numbers, so my suggestion is to simply drop all the complicated stuff, and keep things simple. (Unless you have other plans, I don't expect thousands of new disasters, so you may be fighting a non-relevant problem here.)

(Not exactly relevant if you toss the complicated system overboard, but I also have some reservations about the current implementation. I don't know the loading code exactly, but where is the loaded identifier saved?, DTimerObject only has a pointer.)

* Last but not least, your split of the patch was not the most optimal one. If you look at (series of) commits in trunk, you'll see that each commit makes a SINGLE KIND OF CHANGE. Introduce something, add something, rewrite something, etc. After each commit the entire OpenTTD program still works. As each commit makes only a small change, it is easier to understand that each commit does indeed keep OpenTTD working. If you want to make a large addition (or change), you'll have to make many such small steps.
Your split seems to be roughly chopping the final change in some code parts, rather than making semantical steps like described above. After applying the first 3 patch files (0001*, 0002*, and 0003*) the code does not even compile any more. That is definitely not acceptable.
(In my experience, splitting means another round where you copy parts of the complete change over to a fresh patch queue, one change at a time, to get a nice sequence of small semantical steps.)

In short, a nice first attempt, but it can be improved.
(Do not despair, it is normal with patches. It takes several tries to get it all quite right.)

This comment was imported from FlySpray: https://bugs.openttd.org/task/4286#comment9214

dev_null42 wrote:

What is the use-case for querying the internal timer state from the console? I can see why you'd may want 'do disaster XYZ now', but why have the other commands?

Setting a disaster timer directly could be used for custom scenarios, by scheduling events to occur at a specific date.

'disaster list' is disabled if the cheat is off. The cheat description however says "Enable changing disaster internal values: ...". How is listing values a modification?

The description could be changed to something like "Enable viewing and changing disaster internal values".

When loading an old save game, all timers have the same value, which means they all go off at the same time. Doesn't sound like a good idea.
Since you have several timers running now, did you re-balance the disaster frequency?

Mathematically, the number of disasters stays the same. In the old system, when _disaster_delay reaches 0, a disaster will definitely occur, based upon the year. In the new system, on average there are 4 disasters possible in any year, and the "normal" frequency setting results in a probability of 1/4. Thus, on average, the same number of disasters will occur.[*]

For a 'framework', it seems a bit incomplete to me. There are plenty of other arrays in src/disaster_cmd.cpp that have a strong relation to a disaster. Why not make a class for all that information together? (probably a base class with a derived class for each specific disaster)

I plan on introducing virtual class 'Disaster' in a subsequent set of patches, with a number of new disasters, and rework the existing disaster functions/variables into subclasses of 'Disaster'. A DisasterTimer would instantiate a new Disaster upon invocation.

In DisasterTimer::Startup() you instantiate a fixed number of disasters. I wonder why, how is this better than the array _disasters[] that we had?
Also I don't exactly see why you need a dynamic SmallVector<> for a fixed number of timers.

A dynamic pool of disasters allows one to add and remove them as the game progresses, and allows for scenarios. A future UI change could allow for a scenario writer to add multiple instances of a UFO disaster for a "Roswell" scenario, for example.

(and moving const _disaster[] data back to magic numbers in code seems the wrong direction to me, why do you do that?)

The original code had magic numbers as well. I could change the dates into # defines in disaster.h, if you prefer.

You seem to save disaster information with a unique identifier. It looks like an awfully complicated way to save the information.

It may be over-designed. When I wrote that part I had something else in mind, but then I changed the design.

Last but not least, your split of the patch was not the most optimal one.

I blame that on git not detecting my diffs correctly.

[*] Patch 0003 has the wrong years set. They should be:

coal mine - 1970 - 2020
factory - 1970 - 2020
refinery - 1960 - 2010
zeppeliner - 1890 - 1955
small ufo - 1940 - 1970
big ufo - 2000 - 2100
small sub - 1920 - 1960
big sub - 1970 - 2100

This comment was imported from FlySpray: https://bugs.openttd.org/task/4286#comment9215

Alberth wrote:

What is the use-case for querying the internal timer state from the console? I can see why you'd may want 'do disaster XYZ now', but why have the other commands?

Setting a disaster timer directly could be used for custom scenarios, by scheduling events to occur at a specific date.

Fair enough, why not make these settings available for the scenario developer without cheating then?

When loading an old save game, all timers have the same value, which means they all go off at the same time. Doesn't sound like a good idea.

I did not see an answer to this question. Please try to fix this in the code.

I plan on introducing virtual class 'Disaster' in a subsequent set of patches, with a number of new disasters, and rework the existing disaster functions/variables into subclasses of 'Disaster'. A DisasterTimer would instantiate a new Disaster upon invocation.

Perhaps start with those instead?
I don't see why you'd want to instantiate a disaster each time though, but the code will explain that more clearly.

A dynamic pool of disasters allows one to add and remove them as the game progresses, and allows for scenarios. A future UI change could allow for a scenario writer to add multiple instances of a UFO disaster for a "Roswell" scenario, for example.

Until those plans become a reality, I see no need for a smallmap<>.

In general, the aim of each patch should be to make it optimal for now.
Experience has shown that plans may never get realized for one reason or
another. If that happens, the code is more complicated than necessary. It may
look not that much of a difference, but imagine what would happen if we do this
everywhere. Unused possible extension points everywhere, eating away costly CPU
time for nothing.

(and moving const _disaster[] data back to magic numbers in code seems the wrong direction to me, why do you do that?)

The original code had magic numbers as well. I could change the dates into # defines in disaster.h, if you prefer.

The aim of the source code is maximal readability, and minimal CPU costs.
If you compare the current _disaster[] data with your code, I think you lose on both accounts.
If you introduce # defines, you will lose even more on readability.

Why not setup a const Disaster * pointer in your new timer class instead?

Last but not least, your split of the patch was not the most optimal one.

I blame that on git not detecting my diffs correctly.

Sorry, I was not blaming anybody or anything, just noting a fact that needs fixing.
(I mentioned patch queues, because you work on a large change, introducing many
semantical changes. Experience shows that the best way to show these changes is
to make each one a separate patch file. Doing that manually is cumbersome,
especially since trunk is also moving. A patch queue can be useful in this
situation.)

[*] Patch 0003 has the wrong years set. They should be:

Changing years is a semantical change, make it a separate patch file in the patch sequence.
Such changes are often difficult though.

This comment was imported from FlySpray: https://bugs.openttd.org/task/4286#comment9216

dev_null42 wrote:

Fair enough, why not make these settings available for the scenario developer without cheating then?

Because this is my first patch to OpenTTD, and I have not learned the entire code base yet.

When loading an old save game, all timers have the same value, which means they all go off at the same time. Doesn't sound like a good idea.
I did not see an answer to this question. Please try to fix this in the code.

On average, there will be 4 disasters available when the timer goes off. If disaster timers are set to normal, the odds of any single disaster occurring is 1/4. Thus, on average, upon loading a save game there will be one disaster occurring when the timer reaches zero.

I plan on introducing virtual class 'Disaster' in a subsequent set of patches,
Perhaps start with those instead?

Because this is my first patch to OpenTTD, and I have not learned the entire code base yet. It seemed much easier to change something not as visible to end-users (timers) then learning the entire vehicle, tile, etc. system.

The aim of the source code is maximal readability, and minimal CPU costs.
If you compare the current _disaster[] data with your code, I think you lose on both accounts.

The SmallVector class API has no way to append an object directly; it constructs the object and return a pointer to it. This requires a default constructor. What I need is something like

T &SmallVector::Append(T t, uint to_add = 1)

This is why DisasterTimer::Startup() is written the way it is, using the very odd double-pointer. Alternatively, I could have changed SmallVector or use std::vector (or std::dequeue).

I can change the code to use a fixed-size array instead of a SmallVector. Likewise, I can change the save file format to not save DisasterTimer identifications, though that introduces a dependency with ordering within the DisasterTimer collection.

This comment was imported from FlySpray: https://bugs.openttd.org/task/4286#comment9231

andythenorth closed the ticket.

Reason for closing: Won't implement

Mass closure of patch tickets with no commentary for >5 years. Goal is to reduce patch queue as an experiment to see if it aids faster reviewing and rejection/acceptance (it may not). If this offends you and the patch is maintained and compiles with current trunk, discuss with andythenorth in irc. (andythenorth has no ability to review patches but can re-open tickets).

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