factoriolib / flib

A set of high-quality, commonly-used utilities for creating Factorio mods.
https://mods.factorio.com/mod/flib
MIT License
64 stars 15 forks source link

Event Filtering #2

Closed JanSharp closed 4 years ago

JanSharp commented 4 years ago

I wrote a function generator for event filters, because raise_event + game events doesn't get filtered. Event filter table -> function. It generates this function at runtime, which is sertainly not fast. It could cache functions for filter tables, which adds a bit of usage complexity.

I wanted to get all your thoughts on this, if it's useful, and how it should be exposed to the programmer.

Maybe it's also good to just see how modules will be structured. Idk, this is new territory for me :D

Here is my current source as a reference, but it would change of course.

raiguard commented 4 years ago

Wow, that's pretty cool! But I don't personally think it's worth the performance penalty. Event filters are not supposed to be used for matching purposes, it's supposed to increase performance by telling the game not to pass the event to your mod unless you need it. You need to do matching anyway if you're using an event for more than one thing ever.

This is new territory for me as well, I've never been involved in a project with multiple people before :D

0ptera commented 4 years ago

How is this useful/used in practice?

Event filters only usage is cutting down the amount of calls a mod receives. Does this take care of checking for matching filters inside functions called from filtered events?

JanSharp commented 4 years ago

alright, i thought about it again, and here is how i would have implemented it. something like it anyway:

local event_filter_gen = require("__flib__.control.event-filter-generator")

local on_built_entity_filter = {{filter = "crafting-machine"}}
local on_built_entity_filter_func =
  event_filter_gen.generate(on_built_entity_filter, "created_entity")
-- created_entity is the name of the entity field in the event table.
-- it is the most performant to define it here.
-- it is not an option to just have an 'or' over all possible names
-- as you can see when searching for "return" in
-- https://github.com/JanSharp/EventDataValidatorGenerator/blob/master/FactorioEventDataValidator/Lua/event-specific-definitions.txt
-- there are too many different names

-- but maybe it's best to actually give it the event names
-- and the generator takes care of finding the right fields

script.on_event(defines.events.on_built_entity, function(event)
  if event.mod_name then -- if it came from script.raise_event
    if not on_built_entity_filter_func(event) then return end
  end

  -- handle event
end, on_built_entity_filter)

compared to

local crafting_machine_types = {
  ["assembling-machine"] = true,
  ["rocket-silo"] = true,
  ["furnace"] = true,
}

script.on_event(defines.events.on_built_entity, function(event)
  if event.mod_name then -- if it came from script.raise_event
    if not crafting_machine_types[event.created_entity.type] then return end
  end

  -- handle event
end, {{filter = "crafting-machine"}})

it could also be setup so that it can be used to filter events which don't have filters such as script_raised_built.

generally i see it being most useful for all of these, but just in general it means you never have to think about what to check.

And it would always be as optimized as possible, which means even though it adds a function call, chances are it's still faster.

0ptera commented 4 years ago

How useful is that really? Most mods I've seen need a check for one specific crafting machine or a subset of machine names like

script.on_event(defines.events.on_built_entity, function(event)
  local entity = event.created_entity or event.entity 
  if my_machines[entity.name] then

  end
end

In those cases any group check is just a waste of time.

JanSharp commented 4 years ago

Hm, yea maybe you're right

JanSharp commented 4 years ago

Well actually, jumped the gun a bit, that means most filters are pretty useless. The only simple and useful ones are (ghost) type, (ghost) name, force and the ones for LuaEntityDamagedEventFilters.

All other ones are hardly ever used, but when using them it's actually super annoying. Though for that case, this entire function generation system would be overkill. simply exposing that table i liked would help with that.

0ptera commented 4 years ago

For those few cases where it's useful to resolve entity types from filter we should make that table directly available.

Perhaps create a module specifically for lookup tables. __flib__.lookup_tables.filter_definitions

JanSharp commented 4 years ago

This is no longer needed since 0.18.27. So i'll close it.