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

News subsystem rework #2006

Closed
DorpsGek opened this issue May 12, 2008 · 15 comments
Closed

News subsystem rework #2006

DorpsGek opened this issue May 12, 2008 · 15 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

cirdan opened the ticket and wrote:

Hi,

I'm creating a thread for posting my work in reworking
the news subsystem, a task suggested by Rubidium:
http://www.tt-forums.net/viewtopic.php?p=681855# p681855

My goal is to make the news subsystem cleaner and simpler.

Reported version: trunk
Operating system: All


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

cirdan wrote:

The attached patch reduces the use of callbacks in the NM_CALLBACK news type. Specifically:

- Those news types using NM_CALLBACK no longer store their "magic" parameter in the string_id: for new vehicles, the engine id is now stored in data_a; for company news, the combined player+NewsBankrupcy data is stored in data_b. This frees the string_id parameter to AddNewsItem.

- The string_id parameter to AddNewsItem is now used to hand in the string id proper, after having set the dparams. This means that NM_CALLBACK news types have their strings constructed and used in the same way as the rest of news types.

- As a consequence, the string callbacks are no longer required, so they have been removed. The couple of places where they were used no longer require them.

- The drawing callbacks have been updated to reflect the change stated at the top.

I've tested the patch with each of the messages affected (new vehicles and all of the bankrupcy subtypes) and everything seems to go well.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/2006#comment4126

@DorpsGek
Copy link
Member Author

cirdan wrote:

Looking at the calls to AddNewsItem throughout the code, it can be seem that (almost) all of the calls have the same 'mode', 'flag' and 'callback' parameters for a given 'type' value, eg "NM_SMALL, NF_VIEWPORT | NF_VEHICLE, NT_ADVICE, DNC_NONE" for NT_ADVICE. Having to type them out every time is cumbersome and error-prone, and makes the code ugly and unnecessarily long. It would be nice to just type NT_ADVICE and have AddNewsItem guess the other parameters.

However, there are a couple of exceptions: NT_ACCIDENT (which can take NF_TILE or NF_VEHICLE for 'flag') and NT_COMPANY_INFO (which has four different subtypes). The attached patch solves this by defining a new enum NewsSubtype. For most of the news types, there is a one-to-one mapping from type to subtype, except for NT_ACCIDENT and NT_COMPANY_INFO, which have several subtypes. The AddNewsItem function has been modified to accept a single NewsSubtype argument instead of a tuple [mode, flag, type, callback] and properly fill in the right values for those, and all callers have been updated (which are quite a few).

Note that the patch is mildly large, since it touches files all over the place.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/2006#comment4131

@DorpsGek
Copy link
Member Author

cirdan wrote:

The attached patch updates the documentation for AddNewsItem, which is something that got missed in the previous patch.

Sorry for the inconvenience (although the documentation for that function was already outdated).

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/2006#comment4140

@DorpsGek
Copy link
Member Author

cirdan wrote:

Remove the NewsBankrupcy enum.

All calls to AddNewsItem with the NT_COMPANY_INFO type would use the data_b argument to pass data to the drawing callback, specifically a player index (bits 0-3) and a constant from the NewsBankrupcy enum (bits 4-7). This constant would be used by the callback to tell the different NT_COMPANY_INFO news subtypes apart. With the recent news subtypes system, the subtype can be directly fetched from ni->subtype, so there is no more need to encode it in data_b.

The attached patch makes the DrawNewsBankrupcy callback get the news subtype from ni->subtype. Callers of AddNewsItem using that callback (the former NT_COMPANY_INFO type) no longer encode the subtype in the data_b argument and, as a consequence, the data_b argument contains the plain player index, so DrawNewsBankrupcy does not have to bit-decode it.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/2006#comment4143

@DorpsGek
Copy link
Member Author

cirdan wrote:

Remove the NewsCallback enum

The NewsCallback enum is now used only once, in news_gui.cpp, as an index into _draw_news_callback (the array of special drawing callbacks). So, instead of storing the NewsCallback value in NewsSubtypeData and looking up the callback when needed, directly store the callback, reducing clutter and array lookups.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/2006#comment4150

@DorpsGek
Copy link
Member Author

cirdan wrote:

Ok, I've come to a point where I can't think of any more simple and direct ways of further improving the code without major restructuring. I can think of a few things that could be done, but the benefits can be marginal, so I would like to know the devs' opinion before proceeding. This are the changes I'm thinking of:

- Redo the news FIFO queue. The current FIFO queue implementation is somewhat messy. The idea of having a static buffer of NewsItem may be good if you don't intend to delete pieces from the middle, but now that news items are actually removed (for vehicles), we may be better off with a straight, non-wrap-around buffer. And perhaps the buffer could be used to store pointers to the actual structs, which would be allocated on the fly, minimising memory movement when shifting the array, at the cost of an overhead when allocating the structs. (May I ask why it was implemented as it is in the first place?)

- Make a class of NewsItem. Many of the operations done on NewsItem could be better implemented as member functions, with an abstract base class and multiple derived classes (one per news subtype?). The various flags (NF_TILE, NF_VEHICLE) could be implemented through multiple inheritance. This would make the news subsystem conceptually simpler. However, this would come at a price: since C++ does not (properly) support interfaces, we would have to use virtual inheritance, which implies a run-time performance penalty.

(In any case, making a class of NewsItem would require that the FIFO queue stored pointers and not the actual data, since the classes would no longer have the same storage size.)

Any ideas about what to do next?


This comment was imported from FlySpray: https://bugs.openttd.org/task/2006#comment4163

@DorpsGek
Copy link
Member Author

Rubidium wrote:

FIFO:
implemented that way due to ``hysterical raisins''. Removal of vehicles came later on, and removal of vehicle is not the only thing; some news messages are outdated earlier than others. I myself see more benefit in making it a linked list within the NewsItem structs. The history GUI would just store the pointer to the first message to show in the list and the rest should be fairly easy I guess. No memory moves, simple removal of NewsItems and the GUI and news popups should be even easier.

NewsItem class:
Technically a nice idea, but ... it'd mean lots of communication between the NewsWindow and the NewsItems for especially drawing the window and I'm not quite sure that's a good idea. On derived class per news type sounds better with the exception of the accident type, otherwise you're getting into pointless duplication land again.
In what way does C++ not support interfaces? Okay, the syntax is different from e.g. Java's interfaces, but technically they are completely the same (actually a fully abstract Java class is the same as an interface except that you can't do multiple inheritance of abstract classes).
Subclassing in C++ with virtual functions whatsoever does imply run time performance penalties, though that isn't that bad because we run it almost never.

So the NewsItem class doesn't really sound like it'd make stuff easier, the linked FIFO list on the other hand would most likely.


This comment was imported from FlySpray: https://bugs.openttd.org/task/2006#comment4166

@DorpsGek
Copy link
Member Author

cirdan wrote:

FIFO:
Ok, I'll give a try to the linked list idea.

NewsItem class:
My idea was, actually, to make exactly one ultimately derived class per news subtype, but multiple inheritance would allow to define a NewsItem base class, a NewsItemThin subclass, another NewsItemTile subclass (that only overloads the 'click' member function) and then have NewsItemOpenClose inherit from both of them.
What I meant about C++ interfaces is that there is no way of doing this without virtual base classes (ie, a derived class that only overloads functions still brings along a copy of the structure data).

But I won't push this too hard, I myself am not too convinced that its benefits are worth the effort.


This comment was imported from FlySpray: https://bugs.openttd.org/task/2006#comment4175

@DorpsGek
Copy link
Member Author

cirdan wrote:

Convert the news FIFO queue into a doubly-linked list

The attached patch removes the static NewsItem array that used to hold all active news items, and replaces it with a doubly-linked list. News items are allocated on the fly and appended to the list. This scheme allows for easier removal of items, particularly when removing vehicle-related items.

I've finally chosen a doubly-linked list because a simple linked list only allows direct traversal in one direction, while we may have to run through the list in both directions, eg forward when regularly showing news and backward when the user clicks on the statusbar.

On a side note, I've also rearranged the code in the file, gathering all list-related functions together.

I've done (limited) testing of the patch and everything seems to go well, but the interaction between forced news, current news, the statusbar, etc. is complex so the behaviour in corner cases may have been altered. Please report anything that can be seen as a bug.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/2006#comment4179

@DorpsGek
Copy link
Member Author

cirdan wrote:

The duration field in NewsItem is only used to store the time remaining for a shown item. However, only one such item can be shown at a time, so having a field in each item, all but one of which are zero, seems an overkill.

Remove the duration field in NewsItem, and add a _news_duration variable to hold the time remaining for the currently shown item.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/2006#comment4211

@DorpsGek
Copy link
Member Author

DorpsGek commented Jun 1, 2008

cirdan wrote:

Remove the NF_FORCE_BIG flag from NewsFlag.

The NF_FORCE_BIG flag is allegedly use to mark a news item that is to be shown fully, and not in the ticker, irrespectively of the default news settings. However, the flag is only set at one place, in news_gui.cpp:ShowNewsMessage, just before calling ShowNewspaper, which immediately clears it. So the flag is effectively unused.

While at it, remove an unnecessary setting of NewsWindow::duration, again in ShowNewsMessage, since it will be unconditionally set in the constructor for NewsWindow called from ShowNewspaper.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/2006#comment4219

@DorpsGek
Copy link
Member Author

DorpsGek commented Jun 3, 2008

cirdan wrote:

Remove NM_CALLBACK

The NM_CALLBACK NewsMode only differs from NM_NORMAL in the way drawing is handled. However, news items of mode NM_CALLBACK can be perfectly told apart by the fact that the callback field in _news_subtype_data is non-null. So, in the drawing function, check that for NULL instead of relying on the mode, and we can convert NM_CALLBACK to NM_NORMAL and get rid of the former.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/2006#comment4231

@DorpsGek
Copy link
Member Author

DorpsGek commented Jun 5, 2008

cirdan wrote:

I can't think of anything more that could be (clearly) improved in the news code. Any suggestions? Or maybe I should switch my focus to somewhere else?


This comment was imported from FlySpray: https://bugs.openttd.org/task/2006#comment4238

@DorpsGek
Copy link
Member Author

DorpsGek commented Jun 8, 2008

Rubidium wrote:

I can't think of anything that can be improved in the news code and when I try to find something similar to improve I can't really find it except stuff that'd force the need to create enormous patches or so many patches that it's going to be a hell applying.

The only really useful thing I can think of is fixing the bugs in the range #1400-#2000. These are generally bugs that we know about but haven't had enough time for to actually fix them. Bugs < #1400 are very difficult (if not impossible) bugs or bugs that cause rewriting of a substantial part of OpenTTD. Bugs > #2000 are that new that we didn't have had the time to properly look at them and fix them (you are allowed to do so though).


This comment was imported from FlySpray: https://bugs.openttd.org/task/2006#comment4245

@DorpsGek
Copy link
Member Author

DorpsGek commented Aug 1, 2008

Rubidium closed the ticket.

Reason for closing: Implemented


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

@DorpsGek DorpsGek closed this as completed Aug 1, 2008
@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