Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: Industry production graph #7575

Closed

Conversation

kiwitreekor
Copy link
Contributor

@kiwitreekor kiwitreekor commented May 7, 2019

  • This patch adds a window showing industry production history in last 24 months. (which already exists in Chris Sawyer's Locomotion)
    production_history

src/graph_gui.cpp Outdated Show resolved Hide resolved
src/industry_cmd.cpp Outdated Show resolved Hide resolved
src/widgets/graph_widget.h Show resolved Hide resolved
@nielsmh
Copy link
Contributor

nielsmh commented May 7, 2019

Commit checker found some whitespace problems:

*** b/src/graph_gui.cpp:1667: Trailing whitespace: '					'
*** b/src/graph_gui.cpp:1750: Trailing whitespace: '			'
*** b/src/graph_gui.cpp:1752: Trailing whitespace: '			'

@kiwitreekor kiwitreekor force-pushed the industry-production-graph branch 2 times, most recently from 889a104 to af3332c Compare May 8, 2019 10:56
@kiwitreekor
Copy link
Contributor Author

Commit checker found some whitespace problems:

*** b/src/graph_gui.cpp:1667: Trailing whitespace: '					'
*** b/src/graph_gui.cpp:1750: Trailing whitespace: '			'
*** b/src/graph_gui.cpp:1752: Trailing whitespace: '			'

Should be fixed in 889a104.

PeterN
PeterN previously requested changes May 9, 2019
src/industry_gui.cpp Outdated Show resolved Hide resolved
src/saveload/saveload.h Outdated Show resolved Hide resolved
src/saveload/saveload.h Outdated Show resolved Hide resolved
@nielsmh
Copy link
Contributor

nielsmh commented May 11, 2019

This needs to be updated as the savegame version as been bumped.

@PeterN
Copy link
Member

PeterN commented May 11, 2019

Currently display units for weight and volume are ignored, which is not ideal. This will also affect how the graph scales, and I'm not sure how it should handle oil rigs which have passengers tens and oil in the tens of thousands of litres.

@PeterN
Copy link
Member

PeterN commented May 11, 2019

Another issue, graphs are not being redrawn properly on new a month, leading to visual glitches.

@kiwitreekor
Copy link
Contributor Author

This needs to be updated as the savegame version as been bumped.

Now rebased to e7f6f07.

@kiwitreekor
Copy link
Contributor Author

Another issue, graphs are not being redrawn properly on new a month, leading to visual glitches.

Now graphs are set dirty in UpdateStatisitics. This may resolve the issue.

@kiwitreekor
Copy link
Contributor Author

Currently display units for weight and volume are ignored, which is not ideal. This will also affect how the graph scales, and I'm not sure how it should handle oil rigs which have passengers tens and oil in the tens of thousands of litres.

hmm... maybe should I limit the number of showing cargoes to one at once?

@PeterN
Copy link
Member

PeterN commented May 12, 2019

I don't think that's useful. The player can do that themselves with the filter anyway.

@kiwitreekor
Copy link
Contributor Author

bump?

src/graph_gui.cpp Outdated Show resolved Hide resolved
src/graph_gui.cpp Outdated Show resolved Hide resolved
@kiwitreekor
Copy link
Contributor Author

Sorry for late reply...

@kiwitreekor
Copy link
Contributor Author

bump

@kiwitreekor kiwitreekor force-pushed the industry-production-graph branch 2 times, most recently from 30299e5 to d470426 Compare September 9, 2019 17:56
@2TallTyler 2TallTyler added this to the 13.0 milestone Oct 27, 2022
@DorpsGek DorpsGek temporarily deployed to preview-pr-7575 October 28, 2022 15:38 Inactive
@2TallTyler
Copy link
Member

Rebased for review. We can squash my merge commit when fixing stuff or merging.

@2TallTyler
Copy link
Member

Sorry @kiwitreekor, I rebased and it no longer works. (I think because of other changes to the codebase since you last updated this, not because of the rebase itself.)

Do you plan to update this for review, or should I try to update it myself?

@kiwitreekor
Copy link
Contributor Author

@2TallTyler Sorry for late reply. I don't have enough time to update code now.. so could you update the code?

@2TallTyler
Copy link
Member

Sure, I'll try to fix and update.

@2TallTyler 2TallTyler removed this from the 13.0 milestone Nov 24, 2022
@ldpl
Copy link
Contributor

ldpl commented Dec 19, 2022

Month isn't really a good period to measure production though. IIRC there can be 8 or 9 production cycles in one month making it look like there was a production change on the chart when there was none.
Also did anyone test how much does this add to the savegame? For example, last game I played had around 5k industries, afaict this would add 600Kb of data to the save before compression. It's kind of significant for a server especially since vast majority of this data is useless and is never going to be looked at.

@2TallTyler
Copy link
Member

What period would you suggest instead?

I have not tested savegame size increases, but 5,000 industries sounds like a stress-test more than a typical usecase.

@DorpsGek DorpsGek temporarily deployed to preview-pr-7575 December 19, 2022 17:46 Inactive
@ldpl
Copy link
Contributor

ldpl commented Dec 19, 2022

I don't really know what would be the best period. As I can see there are 3 options:

  1. Fixed number of prod cycles.
  2. "normalized" month. Count like a month but adjust the number to reflect the amount of observed cycles (multiply by DAY_TICKS * 30 / (num_cycles * INDUSTRY_PRODUCE_TICKS))
  3. Regular month (as it is.)
    Each has it's disadvantage: 3 introduces jitter, 1 has non-intuitive interval, 2 doesn't match the production number in industry window. I'd personally would prefer 1 or 2 though as it represents the production dynamic more accurately.

I quickly checked and a regular 2k*2k map on temperate is already around 5k industries on high density. So 4k*4k will be about 4 times that.

@ldpl
Copy link
Contributor

ldpl commented Dec 19, 2022

Thinking a bit more about it, maybe game can split produced cargo between two months when calculating production for cycles that happen to cover both.

@DorpsGek DorpsGek temporarily deployed to preview-pr-7575 December 19, 2022 18:51 Inactive
@DorpsGek DorpsGek temporarily deployed to preview-pr-7575 December 20, 2022 01:55 Inactive
@DorpsGek DorpsGek temporarily deployed to preview-pr-7575 December 20, 2022 02:09 Inactive
@glx22
Copy link
Contributor

glx22 commented Dec 20, 2022

I rebased from 4dd7f62 (to start with a clean base)
The differences between my rebase and @2TallTyler's rebase are minimal (ignoring the 3 extra commits from master).
So good job Tyler as it was not an easy one.

Comment on lines +1437 to +1445
/* Redraw box if lowered */
if (lowered) DrawFrameRect(r.left, y, r.right, y + this->line_height - 1, COLOUR_ORANGE, lowered ? FR_LOWERED : FR_NONE);

byte clk_dif = lowered ? 1 : 0;
int rect_x = clk_dif + (rtl ? r.right - 12 : r.left + WidgetDimensions::scaled.framerect.left);

GfxFillRect(rect_x, y + clk_dif, rect_x + 8, y + 5 + clk_dif, PC_BLACK);
GfxFillRect(rect_x + 1, y + 1 + clk_dif, rect_x + 7, y + 4 + clk_dif, cs->legend_colour);
SetDParam(0, cs->name);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a cleaner implementation of this in PaymentRatesGraphWindow::DrawWidget which should be used instead.

Comment on lines +1565 to +1570
NWidget(WWT_PUSHTXTBTN, COLOUR_ORANGE, WID_IP_ENABLE_CARGOES), SetDataTip(STR_GRAPH_CARGO_ENABLE_ALL, STR_GRAPH_CARGO_TOOLTIP_ENABLE_ALL), SetFill(1, 0),
NWidget(WWT_PUSHTXTBTN, COLOUR_ORANGE, WID_IP_DISABLE_CARGOES), SetDataTip(STR_GRAPH_CARGO_DISABLE_ALL, STR_GRAPH_CARGO_TOOLTIP_DISABLE_ALL), SetFill(1, 0),
NWidget(NWID_SPACER), SetMinimalSize(0, 4),
NWidget(NWID_HORIZONTAL),
NWidget(WWT_MATRIX, COLOUR_ORANGE, WID_IP_MATRIX), SetResize(0, 2), SetMatrixDataTip(1, 0, STR_GRAPH_CARGO_PAYMENT_TOGGLE_CARGO), SetScrollbar(WID_IP_MATRIX_SCROLLBAR),
NWidget(NWID_VSCROLLBAR, COLOUR_ORANGE, WID_IP_MATRIX_SCROLLBAR),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the similar Cargo Payment Rates GUI, these are all COLOUR_BROWN to match the window background.

@2TallTyler 2TallTyler added the work: minor details This Pull Request has some minor details left to do label Dec 20, 2022
@anatolyeltsov
Copy link

Was messing around with fixing requested changes locally and find out strange behavior: when you have both cargo payment rates and production graph windows opened clicking on cargo in an any legend will toggle it and it's graph on both windows (and it will be very laggy in a cargo payment rates window).

I don't think that it's an intended behavior so I fixed it by simply adding _legend_excluded_cargo_production alongside _legend_excluded_cargo.

But maybe it'll be a good idea to rename _legend_excluded_cargo to _legend_excluded_cargo_payment_rates accordingly to make it clearer what is what.

I also rebased it on a recent master.
The only thing that is not clear for me is how can I update this pull request? Or should I create a new one?

Original behavior:
Screenshot from 2023-03-05 02-33-11

Fixed one:
Screenshot from 2023-03-05 03-28-34

@2TallTyler
Copy link
Member

2TallTyler commented Mar 5, 2023

I agree that this is not intended behavior. Good catch.

Yes, you'll have to open a new PR to continue development. Since the original author hasn't touched this PR in two years, it shouldn't be a problem (like the source code, PRs are GPL-licensed so you wouldn't need permission no matter the timeline).

@kiwitreekor
Copy link
Contributor Author

superceded by #10541

@kiwitreekor kiwitreekor closed this May 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
candidate: most likely This Pull Request is probably going to be accepted preview This PR is receiving preview builds size: large This Pull Request is large in size; review will take a while work: minor details This Pull Request has some minor details left to do
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet