Revise EventManager API

[myk002]

trafficstars

#2107 is already in progress for making significant changes to EventManager, but we should discuss exactly what should change and why before we merge any code. There are some questions that need answering, preferably by experienced users of the system (@warmist, @wolfboyft, @Rumrusher et.al.).

1. Should events fire exactly at the requested frequency that a client requests, or should we keep the current "at least as often as requested" behavior? If we want "exactly at requested frequency", who should be responsible for validating that referenced objects still exist? Should events fire for objects that only existed in the time between event deliveries?
2. Should the EventManager API return objects or IDs?
3. If we go the "exactly at requested frequency" route, should Lua events have the same timing characteristics as the cpp events? If so, eventful would need rewriting.

[Tachytaenius]

1: Let's do it at least as often as specified frequency but pass a ticks passed parameter at the end.

2: Objects, because one almost always works with those.

[warmist]

Also just to add to the pile: examples for uses would be nice.

[Tachytaenius]

...To be fair if backwards compatibliy is important enough to make dfstructs fields inconsistent i guess we should respect it here...

[myk002]

Yeah, we'll need a plan for not breaking existing users. How should event frequency be controlled? A new flag passed to eventful.enableEvent()? How can a client request a different event callback signature? Attach handlers to an eventful2 table?

[Tachytaenius]

I think handling frequency the way we do already is fine, and since it's lua if users don't put the "delta time" in their callback function signature it's of no concern, surely?

[Tachytaenius]

To be clear what I meant was stuff like eventful.onUnitNewActive[eventfulKey] = function(id) vs eventful.onUnitNewActive[eventfulKey] = function(id, deltaTicks)

[cppcooper]

What we need most of all is a testing strategy. At least from my point of view, having already finished many disparate refactors to the system [always seems like something gets tacked on at the end lol].

I'll say this to the frequency stuff, I personally want my events to trigger at the specified rate and not quicker than, and certainly not slower than. I don't mind performing other refactors, but that'll still be a PR of mine in the end ;) cause I want that feature. I'm a C++ programmer, I like control.

Take a look at some of the refactored scan code to see roughly how I envision the validation of entities [simple and quick]. https://github.com/DFHack/dfhack/blob/1d4af57f8a19d755d42e4a09e971c6f2d27d5213/library/modules/EventManager.cpp#L605-L693

This appears to be a simple implementation for cleaning up stales, and supporting as requested tick frequency. For this particular example [the job events] the performance has already been improved since we're only scanning lists once, and they can piggy back off one another in that and one or two other ways.

I won't speak to the eventful stuff, my ignorance of its implementation is suddenly palpable reading the comments about it. I guess I presumed it piggy backed on EM with listeners.

[warmist]

Eventful stuff (at least for the eventmanager) is a thing wrapper to expose to lua (and some lua code for e.g. enum). TBH not sure if it's even a good wrapper...

[cppcooper]

TBH not sure if it's even a good wrapper...

Just looked at the git blame.. So like, aren't you the primary dev of eventful? You'd know best if it's a good wrapper :S

What I know is it's too complex to look at improving before I'm done with EM, but if we're looking to replace it entirely with a new design that would could be mixed with the EM PR coding I've been doing.

[cppcooper]

So having discussed testing in discord/#ci I think we could write a unit test for EventManager once #2109 is solid. As per the suggestion @lethosor gave (of just simulating event generation by adding dummy data to the structures EventManager is scanning), we can manage to test the scan code and event dispatching. Which of course will help auto-detect bugs like we saw with #2189.

So writing EventManager tests [or using them at least] will be blocked for a little while until we actually have the ability to run them. In the mean time the simulation idea will at least simplify testing from eventful's perspective.

What I know is it's too complex to look at improving before I'm done with EM, but if we're looking to replace it entirely with a new design that would could be mixed with the EM PR coding I've been doing.

As per another discussion, eventful is totally fine as is. It dispatches the events from a single EventManager listener of the appropriate EventType, then fans out to the lua listeners here: https://github.com/DFHack/dfhack/blob/85d7489b3c66e3f61b418fe138b22da1da2ce032/library/LuaTools.cpp#L1520-L1531

It would be nice to merge eventful into EventManager, with the caveat that this fanning out would be done away with in favor of the lua events registering a callback with EventManager directly. This may not be possible, but if it is I [at least] won't be making time for it within the next few years.

[cppcooper]

1: Let's do it at least as often as specified frequency but pass a ticks passed parameter at the end. -@wolfboyft

I'd like to clarify this. Do you mean pass the ticks passed since eventful last dispatched the event type to lua listeners?

I think handling frequency the way we do already is fine, and since it's lua if users don't put the "delta time" in their callback function signature it's of no concern, surely?

Or were you just describing the current interface?

[Tachytaenius]

1: Let's do it at least as often as specified frequency but pass a ticks passed parameter at the end. -@wolfboyft

I'd like to clarify this. Do you mean pass the ticks passed since eventful last dispatched the event type to lua listeners?

I think handling frequency the way we do already is fine, and since it's lua if users don't put the "delta time" in their callback function signature it's of no concern, surely?

Or were you just describing the current interface?

I do mean pass the ticks passed to listeners, and was talking about lua listeners without that new argument being OK.

[cppcooper]

@warmist I believe this would require modification of the lua api integration point. Any idea how we'd pass the ticks? Could we get away with not touching the lua api, and only messing with the macros perhaps?

[warmist]

I'll hack something up in coming days. Current ideas for API look like this:

local f=function() end
--format 1, closest to current format
em.CallbackName(how_often).disambiguation_name=f
--format 2, the order could be either way?
em.CallbackName.disambiguation_name={f,how_often}
--format 3, named arguments ahoy
em.CallbackName.disambiguation_name={callback=f,ticks=how_often}
--format 4, lazy but might work
em.CallbackName.disambiguation_name=f
em.SetEventFrequency(f,how_often)

I would love to hear feedback for the suggested formats (and maybe votes?)

[cppcooper]

I think it was nearly a year ago that discussion about events steered towards integrating directly with the game. As far as I know this was a real plan that has presumably been implemented by now. If so, and maybe even regardless, we should close out this issue. Unless we want to steer it into @warmist 's API ideas which iirc were abandoned.

[myk002]

Getting event callbacks directly from DF is still on the roadmap, but we're not there yet. We just got function pointers for basic game functions. Active callbacks is a step beyond that.