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

Industry production changes in several news categories #1326

Closed
DorpsGek opened this issue Oct 10, 2007 · 4 comments
Closed

Industry production changes in several news categories #1326

DorpsGek opened this issue Oct 10, 2007 · 4 comments
Labels
flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay

Comments

@DorpsGek
Copy link
Member

Alberth opened the ticket and wrote:

The purpose of this patch is to reduce the number of news messages about
industry production changes that the player sees.
To this end, news about these changes has been split into three categories:
NT_INDUSTRY_PLAYER when the local player is servicing the industry,
NT_INDUSTRY_OTHER when only competitors are servicing the indsutry, and
NT_INDUSTRY_NOBODY when nobody services the industry.

The decision of what category the production change is in, is made by checking
whether vehicles of the player/competitors visit (and load/unload) a station
closeby the industry.

A few notes:

  1. The NT_ECONOMY category is now reduced to general economic news (global recession, end of recession, and introduction of the euro).
  2. There are also no doubt several other news-items that could be split in this way, but I haven't yet thought about them.
  3. This patch is a generalization of Prevent displaying production changes news from idle industries #285 that dealed with idle industries only,
  4. the idea of splitting off idle industry comes from him ("Amadeusz Wieczorek (Meush)").
  5. Also the patch was most helpful in telling me how to extend the message settings window.
  6. Rubidium steered me in the right direction of examining vehicles and stations rather than just the industry.

Patch is against revision 11224

Attachments

Reported version: trunk
Operating system: All


This issue was imported from FlySpray: https://bugs.openttd.org/task/1326
@DorpsGek
Copy link
Member Author

Rubidium wrote:

+static void CanCargoServiceIndustry(CargoID cargo, const Industry *i, bool *c_accepts, bool *c_produces)
This function is not "newindustries"-compatible. Whether cargo is accepted at a moment, and the accepted cargos, can be affected by callbacks, which is something the function should take care of too.

+ const Station *stations[64]; // 64 is a arbitrary probably high enough number to fit all stations in
Why not a stl list or set?

+ if(!c_accepts && !c_produces) continue; // Wrong cargo
+ // Check orders of the vehicle.
+ // We cannot check the first of shared orders only, since the first vehicle in such a chain
+ // may have a different cargo type (if never, uncomment the following line:)
+ // if (v->prev_shared != NULL) continue;
Please check you patch on coding style. Spaces after ifs, whiles and fors, /* ... / for comments that span a whole line (and / ...n *... */ for multiline); // for comments that have code in front of them.

+ const Station *st = GetStation(o->dest);
+ assert(st != NULL);
GetStation will never return NULL. It'll crash, return or a pointer to an "invalid" station. So I guess the assert should do st->IsValid().

If you document a function with /** .. */, please add full comments about the variables and return values etc.

+{ WWT_PUSHIMGBTN, RESIZE_NONE, 3, 4, 12, 26+1312, 37+1312, SPR_ARROW_LEFT, STR_HSCROLL_BAR_SCROLLS_LIST},
+{ WWT_PUSHTXTBTN, RESIZE_NONE, 3, 13, 89, 26+1312, 37+1312, STR_EMPTY, STR_NULL},
+{ WWT_PUSHIMGBTN, RESIZE_NONE, 3, 90, 98, 26+1312, 37+1312, SPR_ARROW_RIGHT, STR_HSCROLL_BAR_SCROLLS_LIST},
+{ WWT_TEXT, RESIZE_NONE, 3, 103, 409, 27+1312, 39+1312, STR_020F_GENERAL_INFORMATION, STR_NULL},
Better change the 12 with some constant. And add spaces between the numbers and operators.

+uint FindStationsAroundTile(TileIndex tile, int w, int h, const Station **st_list, uint num_st)
Passing a stl set makes that function a little simpler as you do not have to check for duplicates yourself, you don't need to limit the amount of stations etc.
It also creates quite some code duplication, which I think should be easily solvable.

-static inline bool IsFrontEngine(const Vehicle *v)
+inline bool IsFrontEngine(const Vehicle *v)
What were you thinking of when doing that?


This comment was imported from FlySpray: https://bugs.openttd.org/task/1326#comment2471

@DorpsGek
Copy link
Member Author

Alberth wrote:

Changes:

- Added handling of CBM_IND_REFUSE_CARGO for new industries. The basic setup of accepting/producing cargo seems to be covered by the cargo arrays of the new industry in the constructor.
- Switched to returning a set of stations rather than a (hopefully) large enough array. Good suggestion! (still thinking too much C)
- Fixed comment style and other glitches (started using a sed script to check my diff). Note: style of comment is not listed at the coding style wiki page (http://wiki.openttd.org/index.php/Coding_guidelines). Also, in the 'Statements' section at the page, // is placed at the start of the line.
- Added a check on st->IsValid() rather than an assert()
- Re-factored the news-settings window widgets to use NT_* constants for vertical placement. Note that except for the addition of the three new news-categories, the layout itself has not changed.
- Moved station catchment code into the FindStationsAroundIndustryTile(), and switched to using that function to obtain nearby stations in the MoveGoodsToStation(), thus eliminating the double TILE loop.
- Also removed the around[8] array in MoveGoodsToStation(). Instead, found stations are now directly ordered by rating.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/1326#comment2517

@DorpsGek
Copy link
Member Author

Alberth wrote:

Updated to new revision

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/1326#comment2518

@DorpsGek
Copy link
Member Author

Rubidium closed the ticket.

Reason for closing: Implemented

In r11442.


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

@DorpsGek DorpsGek added Core flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay labels Apr 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay
Projects
None yet
Development

No branches or pull requests

1 participant