OpenTTD
Feature: Industry production graph
Motivation / Problem
Finish the work started by @kiwitreekor in this pull request.
Description
This patch adds a window showing industry production history in last 24 months.
Most work was done by @kiwitreekor. What I've done:
- rebased on current master
- made requested changes
- fixed a bug that I found during making changes
- renamed
_legend_excluded_cargoto_legend_excluded_cargo_payment_ratesingraph_gui.cppfor more consistency
Closes #7575. (edit 2TT)
Limitations
When turning on/off cargos graph box can move a little as y axis labels are changing (like from 5 to 100) and therefore they take different amount of space.
Checklist for review
Some things are not automated, and forgotten often. This list is a reminder for the reviewers.
- The bug fix is important enough to be backported? (label: 'backport requested')
- This PR touches english.txt or translations? Check the guidelines
- This PR affects the save game format? (label 'savegame upgrade')
- This PR affects the GS/AI API? (label 'needs review: Script API')
- ai_changelog.hpp, gs_changelog.hpp need updating.
- The compatibility wrappers (compat_*.nut) need updating.
- This PR affects the NewGRF API? (label 'needs review: NewGRF')
- newgrf_debug_data.h may need updating.
- PR must be added to API tracker
I like this feature, and from my testing it seems to work.
In the previous PR for this feature (#7575), there was discussion about how saving industry production data might bloat savegame file size. Can you provide some data on this, testing a variety of map sizes and industry density, so we can get a better idea of how this will change?
There was also discussion about using a different interval to graph production. I think that's out of scope and shouldn't block this PR from being merged, assuming it won't have a tremendous effect on file size.
Sorry, didn't see your comment earlier. Rebased on current master and did some tests with default industries and temperate climate:
Master: 256 x 256 map, 50 industries, initial: 98,4 Kb 256 x 256 map, 50 industries, after 2 years: 102,5 Kb 256 x 256 map, 250 industries, initial: 110,2 Kb 256 x 256 map, 250 industries, after 2 years: 111,6 Kb 1024 x 1024 map, 500 industries, initial: 1548 Kb 1024 x 1024 map, 500 industries, after 2 years: 1600 Kb 512 x 512 map, 1000 industries, initial: 411,7 Kb 512 x 512 map, 1000 industries, after 2 years: 424.9 Kb 4096 x 4096 map, 2500 industries, initial: 27,3 MB 4096 x 4096 map, 2500 industries, after 2 months*: 26,5 MB
This PR: 256 x 256 map, 50 industries, initial: 96,6 Kb 256 x 256 map, 50 industries, after 2 years: 101,4 Kb 256 x 256 map, 250 industries, initial: 108,5 Kb 256 x 256 map, 250 industries, after 2 years: 114,3 Kb 1024 x 1024 map, 500 industries, initial: 1561 Kb 1024 x 1024 map, 500 industries, after 2 years: 1612 Kb 512 x 512 map, 1000 industries, initial: 429,5 Kb 512 x 512 map, 1000 industries, after 2 years: 445,7 Kb 4096 x 4096 map, 2500 industries, initial: 27,7 MB 4096 x 4096 map, 2500 industries, after 2 months*: 26,9 MB
* couldn't FF to 2 years as it was veeery slow process for me (game speed with FF was like 1.2x only) and also for whatever reason the later file is smaller
So I would say from these examples that there's nothing really bad in terms of savegame sizes.
Did you test the same seed? 256x256 is smaller with production which makes no sense. Also, there is not much point comparing initial sizes as production history is all zeros that compress extremely well. So would be nice to see some worst case comparison with high industry density and full year history with changing production. Ideally with pre-compression size and savegame generation time comparison.
Though having it being lost within the random noise in this test looks kind of promising.
No, seeds were random so tests are no 100% accurate but anyway.
Tried 512 x 512 map with 5000 industries:
Master: Initial: 480,5 Kb After 2 years: 484,7 Kb
This PR: Initial: 446,2 Kb After 2 years: 466,2 Kb
Saving game took less than a second for me in this case.
The unit tests show that last month production isn't being reported properly, possibly due to incorrect savegame changes, but maybe something else.
I've made a save before and after this PR and it seems that if I try to open older savegame in game based on this PR "last month production" for each industry became zero (therefore tests failed I think).
As I understand we need to get data from different fields/params depending on save load version. I'm not a big expert in all these compatibility things so not sure what to do in this regard at the moment but I'll try to figure it out.
That's what the SLV_CONDARR() stuff should do, but because the name of the variable has slightly changed it's upsetting it.
Might be we need to add a macro to allow overriding the used name (like for the SLEG versions.)
Seems like I've fixed it by adding SLE_CONDARRNAME macro (as you suggested if I understood you correctly + we already have SLE_CONDVAR and SLE_CONDVARNAME pair) and according changes in code. Now everything is OK including regression tests.
Should this addition go as another PR first?
Probably not another PR since it would be unused at that point, but a separate commit in this one.
Done! But I'm not sure about spacing in src/saveload/industry_sl.cpp - it's become a little bit even more mess
Draft PR#10853 restructures the accepts/produced data to make it easier to iterate through.
One effect of that is production/transported data is shuffled around in such a way that it can be extended from 2 months (this and last) to 24 months quite easily (and even monthly rotation of figures doesn't need to change), and without any (further) savegame changes.
I'm not sure if that PR is acceptable right now, but it's something to bear in mind.
Made it work with your latest changes, just need a little clean up! Will update this pull request in the next few days.
Sorry for smashing over it your PR, but the existing layout did need a proper restructuring before adding more deep arrays.
Finally got to do some proper apples-to-apples comparison on this one. And results are pretty interesting, while file size increase is pretty mild, time spent in SlSaveChunks is up almost 50%. That's the non-threaded part of save process so it means higher lag for everyone in multiplayer game when client connects and on autosave. I didn't measure compression time directly but judging by SaveFileToDisk and uncompressed size that's also up about 5-10%, meaning longer connection time. IMO that's a pretty heavy price to pay for just a few charts. I'd very much prefer the game to only save data I may actually need than waste resources on random noise.
Also, that's on default industries, it will be even worse on some newgrf with rapid production changes.
Screenshot from 2023-05-31 03-48-40 Relevant test settings: 2048x2048 map, high industry density, smooth economy
Massive map with slightly more data is slightly slower to save shock
The same amount of data is stored regardless of how many cargo types are involved and how many months has been played.
And due to updated the layout (in master), it isn't just saving an array of zeros when not filled in, either.
50% is not insignificant and that's not even the worst case. Servers I run have up to 10k industries and that's not a "massive" map, I sometimes get complaints from players that they run out of space. So for me this PR effectively doubles the already high lag on each save for basically nothing. As very little of what saved here is actual useful data. No one will ever look at these thousands of random fluctuation charts. It would be much better to save history for industries that matter instead. Can have more history with nearly zero performance impact. Because as it is currently I'd very much like a setting to disable this feature as it's just not worth it for me.
10k industries sounds like a really unusual & rare situation for me 🤔
Very high industry density works really well for multiplayer goal games. As you can get 2-3 industries with one station.
- It makes cargo start more viable and comparable to passenger+mail.
- It's easier to spot places with good production, especially without modified client.
- Allows for more players to play on a relatively small map without running out of resources.
- Makes networks more complex, without boring long stretches of rail.
@ldpl: It would be much better to save history for industries that matter instead.
So, industries with an amount transported over 0%? That's an idea.
I'm not aware of any production changes, vanilla, or NewGRF, that are based on previous production levels, so players do not need that information for non-served industries (whether they'll ask for it is another story 😛).
The Production Graph button could be greyed out if the industry's history is not enabled, possibly with a hover tooltip to say "Transport cargo from this industry to track production history" or similar.
Does the new storage format allow us to store either just last month's production, or the full history, depending on the industry?
IMO, the most interesting are industries that player takes cargo to (secondaries). For those I'd say even 10 years of history won't be too much. In custom clients this information is somewhat covered by delivery cargo graph with cargo selection and even that is good enough for most cases, at least in relatively short games with few secondary industries connected.
For primary industries it can be somewhat educational to see how it grows but ultimately it's just some random, so single production number is good enough for the gameplay. If anything, having a station rating history chart would be more useful in this case.
UPD. Also, chat of cargo received by station would be a cool feature.
So, industries with an amount transported over 0%? That's an idea.
What if player had transported something but then stopped? Should we check if he had transported anything in any of 24 months?
Does the new storage format allow us to store either just last month's production, or the full history, depending on the industry?
I think we can check if anything was transported here 🤔
What if player had transported something but then stopped? Should we check if he had transported anything in any of 24 months?
I'd say that's such a rare case it's irrelevant.
But what if we check only current and/or previous months and it takes more than month (or two) for train or truck to comeback? 🤔
Maybe it makes sense to add simple boolean for industry - something like used_at_least_once (couldn't come up with a better name at the moment) - and set it when something was transported for the first time? 🤔 🤔
The linked patch goes one step further than #11133 (actually makes that one redundant) and replaces the accepted and produced arrays with vectors. Although memory is used by the vector, overall for vanilla (and most other) industries (2 or 3 cargo types) this will use less memory, and again use slightly less disk space.
This one is more complex and there may be missing bounds checking elsewhere -- although most code touching this already uses iterators instead of array index access. In fact this may slightly improve performance, as code will no longer be checking a full 16 entries every time, but then again it might affect cache usage. But not benchmarked and probably not particularly noticeable.
To be honest with vanilla this doesn't gain all that much, but when the history length is extended from 2 to 25 entries, I would expect the saving to be more worthwhile -- without this each industry will use 1600 bytes just for (mostly empty) production history.
https://github.com/OpenTTD/OpenTTD/compare/master...PeterN:OpenTTD:industry-vector
So this PR should wait for the industry vector patch to be finished and merged?