EventManager can miss sending events to handlers

[myk002]

If a handler is registered to an event
And the requested frequency is different from the requested frequency of an existing handler on that event
And there is state kept between invocations of the event triggering code
Then a handler can miss events that it should receive

An example is manageNewUnitActiveEvent at

dfhack/library/modules/EventManager.cpp

Line 608 in 07e96a6

static void manageNewUnitActiveEvent(color_ostream& out) {

It will check for new units and send events to handlers whose "cool down" timer has expired. Handlers that have had an event fire too recently (according to their freq value) will not get an event fired. This is working as intended.

However, when manageNewUnitActiveEvent finishes looping through the handlers, it marks all currently active units as processed. Therefore, the handlers that were not sent an event for the detected new units will never get an event for those units.

In order to continue to support different freq values for different handlers, this issue will need to be solved. Otherwise, we should revert back to the previous behavior of all handlers effectively having the lowest requested freq value.

[cppcooper]

Oh I see, that was by design. I can see the desire for it to send those events though. I see two approaches:

1. keep lists for each event handler
2. use a job system with timers to call those handlers with those data points after the remainder tick time has expired (could be tricky converting that to an actual time)

These are kind of the reasons why I chose to have the events ignored, but in hindsight I suppose that would just result in those event handlers getting called only on very rare occasion.

[myk002]

Either way would be a good bit of work. We'd also have to check that the delayed events still refer to valid entities when they are ultimately fired.

I do strongly feel that we need to ensure that handlers get no fewer events that they would have with the previous implementation. I think developers would be unpleasantly surprised to discover that their handler for new units that only gets called for a fraction of new units.

How important is it to guarantee that each event handler is called no more frequently that what it requests? If we relax that guarantee, we can avoid a lot of complexity.

[cppcooper]

Lists shouldn't be too difficult. I'm thinking something like std::unordered_map<EventHandler, std::vector<void*>> would keep things pretty simple, optimizing to save (read: share) space would be pretty complex quite fast though. These lists shouldn't get to an unreasonable size unless there are a lot of events with tick frequencies set.

I just thought of a cleaner way to do no. 2. Use the same methodology as tick events, set when it should fire.

I believe I primarily added the code to allow tick events to work in a flexible way, the others (I think) just came along for the ride to keep EventHandlers consistent in their behaviour. So if it is desired I could walk those changes back so just the tick events are kept, but then the EventHandlers would have unused members or inconsistent behaviour (tick vs the rest).

I'm personally in favour of the saving event data for later.

[myk002]

I absolutely see the value in tick events being individually controllable. If you're registering for a callback every N ticks, you don't want to be called at M<N ticks. The criteria for choosing a frequency for other events is a little different. Scripts and plugins just need to be called "often enough" for adequate response time. A script author choosing a callback frequency would choose the highest number possible to minimize the performance hit, but if processing was being done anyway, they wouldn't mind being called more often. At least, this is how I understand it (other viewpoints welcome).

How about if we do this in two PRs:

1. keep tick events individually controllable, but revert the others to firing at the fastest requested frequency; describe the enhanced guarantees of tick events in the docs

2. reinstate per-handler frequency tracking, add event buffering and trigger-time checks to ensure that references entities still exist. (Actually, it might be safer and easier to keep state for comparisons per registered callback instead of the current global state); document behavior.

I'm not convinced that (2) is required, or even beneficial (is it a bad thing to be called at a higher frequency than requested if the events are being generated at higher frequency anyway?), but I can be persuaded by a good use case.

[cppcooper]

I'm not convinced that (2) is required, or even beneficial (is it a bad thing to be called at a higher frequency than requested if the events are being generated at higher frequency anyway?), but I can be persuaded by a good use case.

I don't have a use case for it. I'm sure someone could make the argument, but the best I've got is consistency and freedom of choice. Let's just keep the ticks and revert the rest and leave it to the person with the use case to refactor the rest. With that said, if we just fix things I don't see the harm in that either cause I'm skeptical the performance hit would be noticeable.

[cppcooper]

Fixed both ways, take your pick. I did not write changelogs

Though I don't know what this meant:

(Actually, it might be safer and easier to keep state for comparisons per registered callback instead of the current global state); document behavior.