Event (and gui events)

[Nexela]

Both Events systems needs some generic .valid checks when called to be consistent with factorio events. events are not raised if the something is invalid

[Nexela]

For GUI refactoring this check if gui_element and gui_element.valid then

to be inside the handler calling loop would fix that

[Afforess]

Is this actually a valid issue? The stdlib is mostly a transparent pass-through of events so how would it ever receive an event for an entity or element that was not valid?

[Nexela]

It doesn't receive it directly but if there are multiple handlers registered for an event, handler 1 could invalidate the entity and now handler 2 would get passed the invalid entity.

Since Factorio does not raise the event for invalid entities checking for .valid on entities passed in the event is not required.

[Nexela]

Here is something I cooked up but I am not sure how much I like it or the impact it would have on performance.

function Event.dispatch(event)
    Core.fail_if_missing(event, "missing event argument")
    if Event._registry[event.name] then
        for _, handler in pairs(Event._registry[event.name]) do
            if not table.any(event, function(v) return type(v) == "table" and type(v.__self) == "userdata" and not v.valid end) then
                local metatbl = { __index = function(tbl, key) if key == '_handler' then return handler else return rawget(tbl, key) end end }
                setmetatable(event, metatbl)
                local success, err = pcall(handler, event)

[Afforess]

It doesn't receive it directly but if there are multiple handlers registered for an event, handler 1 could invalidate the entity and now handler 2 would get passed the invalid entity.

I agree that is an edge-case that occurs in stdlib events and might not occur in the regular event handlers. I am concerned that automatically "filtering" these events also introduces a hidden factor that could be confusing to developers. Filtering the invalid events with invalid entities could lead to events seeming to "disappear" after appearing for only one event handler. I fully expect to fail to read the documentation encounter this at some point. And I'll probably spend hours trying to debug it.

The opposite edge-case, letting events fire with invalid entities is also bad - but perhaps less confusingly so to developers. Likely this extra event will lead to an error when the developer did not check .valid, but this error is very visible in contrast to the hidden event edge-case. So barring a better third-solution here, I'm inclined to not filter invalid events out.

[Nexela]

Giving the complexity of both Factorio and stdlib events, reading the manual is pretty much required. I understand where you are coming from but following the same practices as Factorio itself by not passing along events for invalid userdata seems the best approach. A good compromise between both could be adding a log() event when stdlib Event tries to pass invalid event data.

[Afforess]

I understand where you are coming from but following the same practices as Factorio itself by not passing along events for invalid userdata seems the best approach.

Actually, how does Factorio handle this sort of behavior? I haven't checked, but something analogous to this should be possible to set up, at least between multiple mods. If two mods are registered to an event, and the first mod destroys the entity related to the event, does the second mod receive the event? Do you know what behavior happens here?

[Nexela]

The second mod will not receive the event. All Factorio raised events are guaranteed to be valid.
Mod raised events can still script.raise_event() invalid stuff

I can't find the exact post but this is slightly related https://forums.factorio.com/viewtopic.php?f=7&t=25473

[Afforess]

Well if that is how factorio is handling it, then stdlib will have to follow suit. I am vaguely concerned about the example condition you suggested though:

table.any(event, function(v) return type(v) == "table" and type(v.__self) == "userdata" and not v.valid end)

this concerns me because it broadly rejects any event that contains an invalid userdata table. The assumption here is that no game events pass in invalid userdata objects intentionally... and I'm not sure that's actually true.

[Nexela]

https://forums.factorio.com/viewtopic.php?t=32039#p202158

[Afforess]

Good to know... For future reference, can you link to a that forum post in a comment in the code as well?

[Nexela]

Still not sure if I like table.any for this maybe limiting it to entities would be the better choice. It won't catch them all but should catch the majority of events.

local entity = event.created_entity or event.entity or event.source or event.destination or event.last_entity
if entity.valid then

[Afforess]

I don't think the giant or clause is a great idea either (what if there are two entities and the first is valid and the second is not?).

If you want to make a table of fields, you could iterate that instead:

local fields = { "created_entity", "entity", "source", "destination", "last_entity" }
if table.any(fields, function(field) return event[field] and not event[field].valid end) then