Breaking change/fix: script.raise_event rework - opinions wanted

[mrvn]

I was thinking about what it would take to check everything passed through rase_event and it's just not a task that I think any sane human would want to sign up for. There are currently 144 events. The checks for all of them on the C++ side is around 3000 lines of code and that's not counting all of the functions they call themselves. The Lua version is typically 4 times larger due to all the mechanics of interacting with Lua data on the C++ side.

That's (at a guess) around 12'000 lines of code that someone would have to write all just to handle the rotten-eggs case of mods doing things wrong.

That doesn't make sense to me to do.

I would happily do it as a volunteer. Having some experience with C++ (25+ years) and with C++ templates incl. type lists (15+ years) I would like to at least try it. Assuming that you already have validators (i.e. functions) for all types involved in event handlers, it looks to me like one validator wrapper class for each type coming from LUA, one template class that can accept a list of these validator wrappers as template parameter and finally 144 different template instances (one for each event type). I would see it as ~3k lines of code + documentation depending on how many things (and how easily) I would be able to reuse. Ideally, I would not write a single validation piece of code and only reuse what you already have so that the same pieces of code are not spread on multiple places.

It's 12'000 lines of code if you want to check all events as opposed to not checking anything.

If on the other hand you write a "event_X" LUA binding for every event it's still 12'000 lines of code for all the checks. Plus 144 lua bindings adding another 1'000 lines of code.

Now compare that to keeping the existing event function and doing a switch/case. That's still 12'000 lines of code for all the checks plus 500 lines of code for the switch/case. And that's what I was talking about. Those +1'000 lines extra for lua bindings vs. 500 lines for switch/case. I would think adding checks to the existing one function should be simpler than adding 144 new functions with individual checks.

And all of that assumes you allow all events to be raised. Any event you don't allow reduces both the 12'000 lines of code to check, lua binding and the switch/case.

[KUDr]

And all of that assumes you allow all events to be raised. Any event you don't allow reduces both the 12'000 lines of code to check, lua binding and the switch/case.

What I tried to say is that it would be much less if done properly. For each event you need one line of code per argument for switch (~500 lines) plus you need one validator for each type of argument. Most of the types that come to the 'event' table are the same as inputs to other (already existing) APIs. So their validators must already exist. And for remaining ~20 types we need to write new validators. Even with all LUA calls necessary to get them out of event table it should take ~50 lines per argument type (~1000 lines). This is together 1500 lines that actually do something. With another ~1500 lines for all the C++ ballast we are at 3000 lines. And this is very pessimistic approach.

[PyroFire]

With the Lua event filters coming out in the next experimental version of the game (2~ hours after writing this if the release doesn't blow up)
it was brought up that filtering for the script-raised-X events would be nice.
But then, it was brought up that we have no way to verify anything given to us from script.raise_event and it got me thinking that it would be nice to change how script.raise_event works.

My proposal:

Change script.raise_event so it only allows custom event IDs.
Mods would no longer be allowed to call script.raise_event(defines.events.X, ...).
Instead, for key events that make sense that a mod may want to trigger it - there would be script.raise_X(...) for that specific event.
The raise_X(...) would accept the exact parameters that the event requires and the C++ logic could make sure that mods never do raise_event(...) with bad data.


There 2 obviously downsides to this:

1. It's a breaking change. So, we wouldn't do it during one of the minor releases but some larger release where it's already known we plan on breaking things.

2. If we (Wube) don't provide a function for raise_X(...) for a given event then mods just can't raise it.

#1 is kind of unavoidable and if we're breaking things I think it would mostly be "ok".
#2 I have mixed feelings on. Part of me says mods really shouldn't be raising game events anyway and in the (rare) cases they do - we can add in the API.

The benefits:

* Mods would never need to worry that other mods pass garbage data in events.
* We (Wube) can add event filtering for the script-raised-X events.

What I want to know is: what does the modding community think about this?

I have mixed feelings about the event filtering in general.
Making a wrapper to generate filter tables from an event distributor took quite a while to write.
Setting that aside...

script.raise_event.
What is it useful for?
What kind of functionality is enabled by it that is unique to it?
Not much.

I'm thinking about that time i used raise_event to "trick/exploit" factorissimo into giving me the ability to teleport them between surfaces, but this is pretty niche use case.

@eradicator also mentioned bluebuild simulating a built event with a player passed with it.
This is probably the only practical use for script.raise_event, which is to signal other mods to react to a simulated game behaviour e.g. when a player mines an entity, but isn't holding right click (done by script).

So to put it one way, i think raise_event could be removed without really harming anything, but at the same time you're removing unique lua behavior to do certain things that would otherwise not be possible without it.

The more I think about it the more I think it just doesn't make sense to me for mods to really ever raise game events.

raise_event enables a certain level of universal cross-mod compatability.
Although i agree that it is under-used, i disagree that under-use is a valid reason to remove it.

Since we added the function call raising to the create_entity, revive_entity, and destroy_entity things, I haven't needed to raise any base game events manually

Maybe you haven't, but I have.

https://mods.factorio.com/mod/planetorio
https://mods.factorio.com/mod/planetorio_dangOreus

In this specific situation, i am referring to the fact that dangOreus relies on built_entity events to destroy things on resource tiles.
But planetorio is meant to add some overrides, so that dangOreus only affects the surfaces that are flagged to use that specific "mod surface environment", and equivocally, dangOreus's on_built_entity events (among others e.g. chunk_generated)

Obviously this is currently functioning without issue, but that's because the dangoreus mod was modified with a hijack.lua to override the script.on_event function to proactively register it with the mods template.
Planetorio then internally validates and raises the templates and events and such on its own when its appropriate to do so.

The point i'm getting at here is this is a prime example of the essential, unavoidable need for an ability to raise base game events manually, including for create_entity, in some capacity.

So you raise on_built_entity with missing parameters and break mods subscribing to it normally. I really prefer subscribing to "script_raised_built" over having my mods broken by mod authors raising bad events.

I agree and disagree.
Specifically: When a mod raises an event, this is included with the debug trace.
However, I also should not be getting bug reports for mods raising bad events, but i probably will because my mod is on top.
More on this below.

Not wanting to write all of those + not having a nice way to ensure no extra data is passed in that shouldn't be there.

Don't you already have all of those (if a mod destroys an entity on on_built_entity, the event is not raised for any more mods down the chain)?
Why is this not already being used for raise_event?
And what is the issue with extra data being passed?
I don't see anything breaking if an ignored variable is in the event table.
In fact, such behavior might be rather useful for certain situations.

So it seems like there are 2 options:

1. Leave it as is and just let every mod developer suffer from poorly written mods that call game events with wrong data.

2. Remove raise_event for game events so mod developers never need to think about game events being wrong but break any mod which calls game events.

To me, that seems like a simple answer: #2. The majority of mods do not use raise_event but do suffer from the broken ones that do. The minority use raise_event and are spoiling it for the majority.

There was an invariant that mods have broken: game events are not supposed to ever be called unless all parameters are valid according to the event specs. If anyone can break that invariant then it's useless. See https://factorio.com/blog/post/fff-272 where I talked about that concept before.

I completely disagree with both ideas, and i would support further implementation of any features or functions that enabled more control over the game world, even if such features would be unstable if not correctly handled.
e.g. I would LOVE a way to arbitrarily connect entities such as belts and their neighboring transport lines, inserters and their pickup/destination, rails and their "next rail" neighbour, connecting power networks between surfaces etc.
These things are likely to break the game in horrible ways if not correctly managed, but this is NOT a reason to avoid allowing this kind of behavior.

What about

3. Keep raise_event but validate its parameters for game events.

4. Replace raise_event for game events with an equivalent function that validates all parameters.
- a. The replacements are implemented when asked for.
- b. All replacements should be available from the start.

From reading this thread, people seem to lean towards 3 or 4b, while 4a is your original proposal. I personally would also support these two options (3 and 4b) over the other options, as there has not been a reason given as to why they could not be done.

I might have another alternative.

The issue seems to come down to (1) event filters, and (2) the error handling

(1) Event Filters for script.raise_event.
For the events that support filters, i don't see any issue running these through some blind/simple validation, enough so that the filtering happens.
Any extra/bad data in those event data tables falls under...

(2) Error Handling.
The issue is i'm going to get a bug report for something that isn't broken in my mod, but was actually caused by another mod raising an event with bad data.
Currently, factorio will throw a lua error message with debug trace pointing to the function it failed on, leading most players to believe that this is where the error is occurring.
And everyone wants to be helpful when reporting bugs, so they "self-diagnose" and assume that is the mod that is failing.
Fact is by allowing more than 1 mod to show in a debug trace (i don't see how you would Disallow it), it becomes unavoidable that mod authors will need to trace another's mod to figure out why something isn't working.
I have lost count of the number of other mods i've had to trace to fix bugs.
And most of them aren't even caused by script.raise_event.
I don't believe the one or two extra bug reports each mod might get caused by raise_event with bad data from a few mods is enough to justify removing this kind of functionality entirely.
Particularly when you consider the volume of bug reports that authors are already getting caused by other mods doing bad things, or even worse, when players have mods installed that are very clearly not going to work together.
Pristine Perfect Example: https://mods.factorio.com/mod/starshipt ... 000d967623 Seriously. -_- https://mods.factorio.com/mod/Hovercraf ... 000bfc714c


So, what about

5. Use your existing validation code and existing intention and not worry about the small stuff e.g. seen where on_built_entity will check if the entity is still valid before raising the event for the next mod in the chain.
This simply reduces the size of the problem by focusing on the main properties.
But as it has been noted many times, the use-cases of raising events is rather limited.
E.g. literally No One is going to script.raise_event( https://lua-api.factorio.com/latest/eve ... ce_deleted ) unless a surface is actually being deleted, but by then the game would just raise the event on its own without needing the script.raise_event because for any potential use-case, you could just use surface.clear().
All other scenarios would be "tricking/exploiting" another mod into unintended behavior (see factorissimo/cloning/surface teleport example above).
This means that you can ignore *certain* events without issue as there is no reason or purpose to raising them manually, and this applies to all suggestions so far whether you do script.special_raise_X_Y_and_Z(special_function_params) and a few are missed, or if you just leave it as-is and only put validation on the events that were given filters.

I was thinking about what it would take to check everything passed through rase_event and it's just not a task that I think any sane human would want to sign up for.
There are currently 144 events.
The checks for all of them on the C++ side is around 3000 lines of code and that's not counting all of the functions they call themselves.
The Lua version is typically 4 times larger due to all the mechanics of interacting with Lua data on the C++ side.

That's (at a guess) around 12'000 lines of code that someone would have to write all just to handle the rotten-eggs case of mods doing things wrong.

That doesn't make sense to me to do.

What makes even less sense is how such validation is already occurring, but cannot be used for this purpose?
This particularly applies to the events that filters can be applied to, which, as you said in OP:

it was brought up that filtering for the script-raised-X events would be nice.

Consider you've described this as

(at a guess) around 12'000 lines of code

You could reduce the amount of work you've already tasked yourself with significantly by only validating events that actually have filters for them.
In all other scenarios, the problem is resolved by marking bad/buggy mods as incompatible.
So we could make another suggestion out of this - Silently marking mods as incompatible. It's like (?) in your info.json, but instead of a question mark it's (!)

[Rseding91]

People seem to be misunderstanding how the entire Lua API works... which Isn't too difficult to do since none of you can see it.

The game is C++. When a C++ "game event" happens the logic goes over the event listeners and lets them know it happened. That eventually makes its way into the "put the C++ data into the Lua state that's listening to this event and run the Lua event handler". For example - the entity-died event does that through this C++ function:

Code: Select all

//? Called when an entity dies.
//? Can be filtered using $ref(LuaEntityDiedEventFilters)
//? $param(entity, LuaEntity)
//? $param(cause, LuaEntity, $optional) The entity that did the killing if available.
//? $param(loot, LuaInventory) The loot generated by this entity if any.
//? $param(force, LuaForce, $optional) The force that did the killing if any.
bool LuaGameScript::pushOnEntityDied(const GameAction& event)
{
  auto& data = event.getEntityDiedData();
  if (data.entity->isToBeDeleted())
    return false;

  // entity
  new LuaEntity(data.entity, this->luaState);
  lua_setfield(this->luaState, -2, "entity");

  // cause
  if (data.cause && !data.cause->isToBeDeleted())
  {
    new LuaEntity(data.cause, this->luaState);
    lua_setfield(this->luaState, -2, "cause");
  }

  // loot
  new LuaInventory(data.inventory, this->getMap(), this->luaState);
  lua_setfield(this->luaState, -2, "loot");

  // force
  if (!data.forceID.isZero() && this->getMap().forceManager.getForces().has(data.forceID))
  {
    new LuaForce(this->getMap(), data.forceID, this->luaState);
    lua_setfield(this->luaState, -2, "force");
  }

  return true;
}

This function uses the C++ entity-died event data struct which looks like this:

Code: Select all

  struct EntityDiedData
  {
    EntityWithHealth* entity = nullptr;
    Entity* cause = nullptr;
    TargetableInventory* inventory = nullptr;
    ForceID forceID;
  };

On the Lua side it gets:

entity = LuaEntity (the entity that died)
cause = LuaEntity (the entity that killed the entity - optional)
loot = LuaInvnetory (the loot generated by this entity dying)
force = LuaForce (the force that killed this entity - optional)

Now, that function takes a C++ struct and creates the Lua wrappers that get passed to the Lua event handler. That is a 1-way trip. You can't just use that same function to "validate the data gotten from lua is correct for this event".

I've written the function that would have to be used to do that validation just for this example. Note the function isn't actually complete because there are parts that simply don't work. Also note it took me 20 minutes to write this and I already knew literally every line I would have to write and where every function was that I would need to use.

Code: Select all

int32_t LuaBootstrap::luaRaiseEntityDied(lua_State* L)
{
  LuaHelper::argCheck(L, 1);
  luaL_checktype(L, 1, LUA_TTABLE);

  GameActionData::EntityDiedData eventData;

  lua_getfield(L, 1, "entity");
  Entity* entity = LuaHelper::parseEntity(L, -1, nullptr);
  lua_pop(L, 1);
  if (!entity->isEntityWithHealth())
    throw ScriptException("Given entity is not an entity with health.");
  eventData.entity = static_cast<EntityWithHealth*>(entity);

  lua_getfield(L, 1, "cause");
  if (!lua_isnil(L, -1))
    eventData.cause = LuaHelper::parseOptionalEntity(L, -1, nullptr);
  lua_pop(L, 1);

  lua_getfield(L, 1, "loot");
  luaL_checktype(L, -1, LUA_TTABLE);
  LuaInventory* luaInventory = dynamic_cast<LuaInventory*>(LuaHelper::parseLuaBindableObject(L, -1));
  lua_pop(L, 1);
  if (!luaInventory)
    throw ScriptException("Given object is not a LuaInventory.");
  if (!luaInventory->valid())
    throw ScriptException("Given inventory doesn't exist anymore.");
  // This doesn't work because the event is setup to store TargetableInventory* and there's currently no way to get that back from LuaInventory
  // I would have to go add a way to get TargetableInventory* from LuaInventory (can be done)
  // eventData.inventory = luaInventory->getTargetableInventory();

  lua_getfield(L, 1, "force");
  if (!lua_isnil(L, -1))
    eventData.forceID = LuaHelper::parseForce(L, this->script->getMap(), -1);
  lua_pop(L, 1);

  this->script->eventDispatcher->run(LuaEventType::ON_ENTITY_DIED,
                                     this->script->getMap().entityTick,
                                     &LuaGameScript::pushOnEntityDied, GameAction(GameAction::EntityDied, eventData));
  return 0;
}

So, in summary: no. We can't just use the same checks that the game event uses to validate when mods call raise_event.

[Rseding91]

Addendum: I messed up the example function I gave and Bilka pointed out my mistake. That was another 5 minutes to fix that. So... I failed in writing the first of the 144 functions. Meaning: I don't think anyone is going to write those functions and have it be correct.

Meaning: they would all need tests written which is even more time and even more code to write.

[eradicator]

you're removing unique lua behavior to do certain things that would otherwise not be possible

This is basically my main point. While i do understand that writing all those tests is a herculean task, i simply disagree with the reason we're even talking about this. @Rsed: Your main reason given is that you want to remove a minor source of possible crashes and possibly some tiny performance gains (though i hear that one actually still needs lua-side filters anyway, so i am very skeptical about the usefulness of the latter). This i percieve to be part of a general tendency to cut edge cases to make it technically impossible for mods to do anything that might be remotely "unsafe" or that you personally don't like or understand - what i previously called "dev over-control syndrome".

A few examples of the edge-case history:
1) Runtime flow limit adjustments for the electric energy interface were cut due to performance optimizations of the electrical system. This irreparably broke my power flow regulator mod.
2) Custom rail categories were removed because (paraphrased from memory) "it doesn't make sense because they'd still use the same signals". Why not leave it to the modders to decide if something makes sense or not?
3) Removal of the tool slot because (praphrase) "i haven't seen any mods that do anything interesting with it yet".
4) Attempt to remove fluid temperature for same reason as 3).
5) Removal of LuaPlayer.cursor_position for unknown reason. So having the player click with some fake/invisible placeable item/capsule is the only option left sometimes.

I think that "edge cases" are often the lasts straws that allow a few bold modders to create mods that are actually unique, and i fear that once all edges are cut, the only mods left will mostly be the same - 95% of all mods already consist only of "more machines and recipes". And collaboration is not a solution in a post-script.raise-modsphere, because it's realistically impossible to know which other mods care about which events.

I realize that in a non-ideal world we can't have everything, and i won't even try to imagine how annoying i might be from your point of view. I'm just posting my thoughts and see what comes of it, and hopefully i don't hurt anyone in the process.

[Bilka]

While i do understand that writing all those tests is a herculean task, i simply disagree with the reason we're even talking about this. @Rsed: Your main reason given is that you want to remove a minor source of possible crashes and possibly some tiny performance gains (though i hear that one actually still needs lua-side filters anyway, so i am very skeptical about the usefulness of the latter).

You need the lua-side filtering because the proposal from this thread isn't implemented. If it were, you could get rid of the lua-side filtering,

[Bilka]

A few examples of the edge-case history:
1) Runtime flow limit adjustments for the electric energy interface were cut due to performance optimizations of the electrical system. This irreparably broke my power flow regulator mod.
2) Custom rail categories were removed because (paraphrased from memory) "it doesn't make sense because they'd still use the same signals". Why not leave it to the modders to decide if something makes sense or not?
3) Removal of the tool slot because (praphrase) "i haven't seen any mods that do anything interesting with it yet".
4) Attempt to remove fluid temperature for same reason as 3).
5) Removal of LuaPlayer.cursor_position for unknown reason. So having the player click with some fake/invisible placeable item/capsule is the only option left sometimes.

1) That's in the same vein as the different fluid properties being removed for the faster algorithm in 0.17. I dislike both of those removals. But at least the power flow limit can be "emulated" by changing the buffer size.
2) They didnt do anything. What's the problem with removing them considering that they did nothing?
4) The proposal was made due to potential performance improvements, not "because modders dont use it".
5) It's not deterministic. The cursor position isn't directly sent over the network, that would murder it.

[Rseding91]

... This i percieve to be part of a general tendency to cut edge cases to make it technically impossible for mods to do anything that might be remotely "unsafe" or that you personally don't like or understand - what i previously called "dev over-control syndrome". ...

You're not wrong. That is my goal: to remove anything remotely unsafe and make it impossible for mods to cause issues. It's not always possible but that is the goal.

I come from a background of playing with Minecraft mods and talking with the mod authors about the horrible issues they have because of other mods doing things wrong/stupidly and I don't want to stick that on anyone else.

[KUDr]

People seem to be misunderstanding how the entire Lua API works... which Isn't too difficult to do since none of you can see it.

...

I've written the function that would have to be used to do that validation just for this example. Note the function isn't actually complete because there are parts that simply don't work. Also note it took me 20 minutes to write this and I already knew literally every line I would have to write and where every function was that I would need to use.

...

So, in summary: no. We can't just use the same checks that the game event uses to validate when mods call raise_event.

Many thanks for taking your valuable time and explaining it on example. It was clear and understandable.

I still think that writing all necessary validators is the right way to go. And I also still think that I can do it (with your help, of course).

Meaning: they would all need tests written which is even more time and even more code to write.

Agree. This is a potential source of problems in the future, so it has to be testable.

...

You're not wrong. That is my goal: to remove anything remotely unsafe and make it impossible for mods to cause issues. It's not always possible but that is the goal.

Yes, defensive programing is the best way to get stable product. Mod API should be as strict as possible.

However, there are a few points mentioned by others that I would like to see there too:
1. Events received from other mods should be distinguishable as such. For example a special field in the event table called mod_name (string) added automatically by the game.
2. Allow additional event data to be added by the mod. Not to the event table itself, but to the event.mod_data (table). This will make it clear to all that it was added by a mod, not by the game.
3. Allow to register more event handlers for the same event. This was a "nice to have" feature in the past but with event filters it gets more important.

This is the API I would like.

[Bilka]

However, there are a few points mentioned by others that I would like to see there too:
1. Events received from other mods should be distinguishable as such. For example a special field in the event table called mod_name (string) added automatically by the game.

So the first mention of this was not a joke then? Because mod_name is already a thing right now...

[PyroFire]

the first of the 144 functions. Meaning: I don't think anyone is going to write those functions and have it be correct.

Well ok, if repurposing existing code isn't going to work, my other big point was about how there are actually little to no uses for about half of the events, so they can be skipped entirely (like raising a surface destroy event... why?).
This ideally reduces the workload down to 72 functions.
Much more manageable already

This brings us back to the purpose of the thread i suppose, doesn't it?
A question for the community.
Give a good reason to keep script.raise_event - the factorio devs consider it as creating more problems than it solves, and does not lead to worthwhile functionality. Currently i agree with this assessment.
The only exception there is, seeing as with bluebuild, unless someone can show a better way to achieve what it currently does, the events it raises should be ported over.

In the spirit of giving functional and actionable feedback, i have gone through the list of events and have noted the following that may have potential uses for a mod to manually raise it.

And while we're upgrading things, let's also add a function which i consider to be missing:
script_raised_set_tiles -- Does exactly what it says on the tin.
tiles :: array of LuaTile: The tile prototype and position data.
surface_index :: uint: The surface the tile(s) are built on.

Combine with surface.set_tiles{raise_event=true}


Now, starting with the "Only if it isn't already raised internally" stuff

Inventory Changes:
https://lua-api.factorio.com/latest/eve ... ry_changed
https://lua-api.factorio.com/latest/eve ... ry_changed
https://lua-api.factorio.com/latest/eve ... _equipment

Chart Tags:
https://lua-api.factorio.com/latest/eve ... _tag_added
https://lua-api.factorio.com/latest/eve ... g_modified
https://lua-api.factorio.com/latest/eve ... ag_removed

I will not be counting these, as i am assuming the game already raises these when called by script.

-------------------------------------------------------------

Biter Base Built: I'm pretty sure this is only called when biters naturally build bases. Scripts cannot raise this behavior.
https://lua-api.factorio.com/latest/eve ... base_built

Market Item Purchased: Given the fact that a market doesn't exist in the base game, i don't think there would be a way for scripts to raise this behavior unless it works with the built-in market entity (or something -- not sure). This event seems useful, how do i raise it naturally?
https://lua-api.factorio.com/latest/eve ... _purchased

The Maybe's: Stuff i'm not sure about.
https://lua-api.factorio.com/latest/eve ... gs_changed -- Only if (1) it's possible for scripts change the difficulty at all and (2) this event isn't already raised internally.
https://lua-api.factorio.com/latest/eve ... nged_state -- Trains are weird and the mod options are somewhat limited. I'd prefer to keep a raise event for this one if possible, just to keep as much functionality over trains as possible.

Yep. A mere 4 events that would need to be kept as they serve some useful function or purpose that definitely would not otherwise be possible.

-------------------------------------------------------------


Now let's talk about the meat of the issue, so to speak.

Simulation Events: These events allow mods to forcibly raise a "simulated" player action.
* https://lua-api.factorio.com/latest/eve ... ted_entity

https://lua-api.factorio.com/latest/eve ... afted_item
https://lua-api.factorio.com/latest/eve ... ed_capsule
https://lua-api.factorio.com/latest/eve ... ransferred

https://lua-api.factorio.com/latest/eve ... ilt_entity
https://lua-api.factorio.com/latest/eve ... built_tile
https://lua-api.factorio.com/latest/eve ... ned_entity
https://lua-api.factorio.com/latest/eve ... mined_item
https://lua-api.factorio.com/latest/eve ... mined_tile
https://lua-api.factorio.com/latest/eve ... mined_item

https://lua-api.factorio.com/latest/eve ... ilt_entity
https://lua-api.factorio.com/latest/eve ... built_tile
https://lua-api.factorio.com/latest/eve ... obot_mined
https://lua-api.factorio.com/latest/eve ... ned_entity
https://lua-api.factorio.com/latest/eve ... mined_tile
https://lua-api.factorio.com/latest/eve ... _pre_mined

That's 16 15 events there.

But let's not forget the script_raised_* which more or less speak for themselves.
https://lua-api.factorio.com/latest/eve ... ised_built
https://lua-api.factorio.com/latest/eve ... ed_destroy
https://lua-api.factorio.com/latest/eve ... sed_revive
https://lua-api.factorio.com/latest/eve ... _set_tiles << please?

So that's 20 19, plus the 4 from earlier, so 24 23 functions in total.
Already making some huge progress, but we can do better.

Remember the use of raising these events are to "simulate" a player (or robot) action.
The existing (and future) script_raised_* is almost sufficient to achieve that goal.
All it needs are some optional data in the event table.

For all the events, script_raised_built, and script_raised_set_tile, script_raised_destroy and script_raised_revive need to be given these parameters so that mods can "credit" a raised event to a particular entity or player.
source :: (optional) LuaEntity: a source entity (usually a robot, could also be a character/player? A biter? anything) that should be credited as the thing that built/destroyed/revived the entity/tile.

In the case of script_raised_destroy, a flag is also needed to denote whether the entity was destroyed because it was destroyed, or if it was mined. This avoids the need for script_raised_mined_* functions. It's already here with destroy as a flag.
was_mined :: (optional) bool: Whether the destroyed entity was destroyed by something having mined it or not. Defaults to false.

With these small changes and the addition of script_raised_set_tile, we've bumped off the need for all 12 of the *_built_* and *_mined_* events, bringing the chunk of 16 events down to 4 3.
And those are:

https://lua-api.factorio.com/latest/eve ... afted_item -- Used to simulate the re-crafting of an existing item stack, or something. Not sure, this particular event is very niche use. But it basically flags an inventory changed event as coming from a "craft" action.
https://lua-api.factorio.com/latest/eve ... ed_capsule -- Simulating a player having using a particular item at a given position. Not much else can replicate this.
https://lua-api.factorio.com/latest/eve ... ransferred -- Required to raise a simulated inventory transfer event with a "destination/source" entity and a player.

* https://lua-api.factorio.com/latest/eve ... ted_entity -- It's either allowing simulation of this, or adding script_raised_entity_rotated (internally) when an entities orientation/direction is changed by script, with the same player & source parameters as the others to "credit" the action to something. Note: This can be raised by calling LuaEntity.rotate({by_player=game.player}), and therefore no longer belongs in this list


-------------------------------------------------------------

SUMMARY

Assuming the following additions/changes are made to existing functionality:
- addition of script_raised_set_tile event
- "source" parameter added to script_raised_* credit a script raised action to something.
- "was_mined" parameter added to script_raised_destroy to flag a destroy action as having been mined or not.

Only the following events need to be changed:

-- Simulation events: 4 3 --
* https://lua-api.factorio.com/latest/eve ... ted_entity

https://lua-api.factorio.com/latest/eve ... afted_item
https://lua-api.factorio.com/latest/eve ... ed_capsule
https://lua-api.factorio.com/latest/eve ... ransferred

-- Script raised events: 4 --
https://lua-api.factorio.com/latest/eve ... ised_built
https://lua-api.factorio.com/latest/eve ... ed_destroy
https://lua-api.factorio.com/latest/eve ... sed_revive
https://lua-api.factorio.com/latest/eve ... _set_tiles << please?

-- General purpose events: 4 --
https://lua-api.factorio.com/latest/eve ... base_built
https://lua-api.factorio.com/latest/eve ... _purchased
https://lua-api.factorio.com/latest/eve ... gs_changed
https://lua-api.factorio.com/latest/eve ... nged_state


Total validation functions needed: 12 11.
Now that sounds like a far more humanly-doable number of functions to write (and get it correct the first time) than 144.
Thoughts/ideas?

[KUDr]

However, there are a few points mentioned by others that I would like to see there too:
1. Events received from other mods should be distinguishable as such. For example a special field in the event table called mod_name (string) added automatically by the game.

So the first mention of this was not a joke then? Because mod_name is already a thing right now...

In the API docs I have found only script.mod_name, not event.mod_name. Where is it in the doc, please?

[Bilka]

However, there are a few points mentioned by others that I would like to see there too:
1. Events received from other mods should be distinguishable as such. For example a special field in the event table called mod_name (string) added automatically by the game.

So the first mention of this was not a joke then? Because mod_name is already a thing right now...

In the API docs I have found only script.mod_name, not event.mod_name. Where is it in the doc, please?

https://lua-api.factorio.com/latest/Lua ... p.on_event

[KUDr]

https://lua-api.factorio.com/latest/Lua ... p.on_event

Thanks! I didn't expect it there... My mistake.

[movax20h]

Hi, I am not a mod author, but separate raise_X functions do have advantage of improving things like code completion (thus leading to less errors and probably a bit easier to read all around in the code), simplifying documentation and examples. By having separate signatures and API, the game engine doesn't need to do validation of required features, as that could be done by the Lua itself essentially. Crashing (or throwing exceptions) script on incorrect call is just fine, and will improve performance slightly be removing unnecessary checks. Game engine can just assume the correct call, and if the caller does something wrong, they will are to deal with consequences instead. Engine and API shouldn't encourage incorrect use of APIs, by trying to workaround incorrect calls or ignore them. Just crash. This is the only way to force people to fix their usage.

[PyroFire]

Hi, I am not a mod author,

I wish i stopped there.
I hope this doesn't come across as rude, it's just...
You're painfully incorrect, so hope i can explain this well enough.

but separate raise_X functions do have advantage of improving things like code completion

by nature as long as someone is willing to work on it, the factorio base code will never be complete.
Same goes for mods.
Any project can practically be improved ad infinitum.

(thus leading to less errors and probably a bit easier to read all around in the code)

That's why the devs want to change things. But at the same time, it's more moving parts and more code, so, not really.

simplifying documentation and examples.

These changes actually complicates the documentation and examples further due to more functions involved, more variables, and more events.

By having separate signatures and API,

signatures?

the game engine doesn't need to do validation of required features,

These changes add more validation, particularly if you're trying to write an event distributor (which i have already done for the new filters).

as that could be done by the Lua itself essentially.

Could be, but consider raising the event once for each of the 50~100 mods a player might have installed.
And then consider that the checks each of those mods makes against all of its internal caches, keywords and typings.
Why waste that processing time in lua when the mod should already know (or be able to calculate) beforehand the kinda of entities it wants to see.
The lua overhead must be quite massive to do that, particularly for events like on damaged.

Crashing (or throwing exceptions) script on incorrect call is just fine,

Not exactly. Crashes mean something is broken or not working correctly. A premium product often entails stability and reliability, which is what the devs have clearly expressed their goals to be:

That is my goal: to remove anything remotely unsafe and make it impossible for mods to cause issues. It's not always possible but that is the goal.

and will improve performance slightly be removing unnecessary checks.

C checks are significantly faster than lua checks.
For this very reason, the function table_size exists solely for efficiency and because it is used so frequently.
The cost of adding few checks before distributing the event tables is more efficient than making each mod do this themselves.

Game engine can just assume the correct call, and if the caller does something wrong, they will are to deal with consequences instead.

But what if my mod is raising the event correctly and it's, in fact, the dependent that is causing the issue? (not dealing with the natural event correctly)
This cannot be resolved without one mod author reviewing another's code.
Running raise_event through C filters would help make error reporting more accurate - only the mod that is broken gets the error, and only show that name.

Engine and API shouldn't encourage incorrect use of APIs, by trying to workaround incorrect calls or ignore them. Just crash. This is the only way to force people to fix their usage.

And in the process - flood the factorio bug reports page and the mods discussion pages pointing to things that aren't actually bugs.

[eradicator]

A few examples of the edge-case history:
1) Runtime flow limit adjustments for the electric energy interface were cut due to performance optimizations of the electrical system. This irreparably broke my power flow regulator mod.
2) Custom rail categories were removed because (paraphrased from memory) "it doesn't make sense because they'd still use the same signals". Why not leave it to the modders to decide if something makes sense or not?
3) Removal of the tool slot because (praphrase) "i haven't seen any mods that do anything interesting with it yet".
4) Attempt to remove fluid temperature for same reason as 3).
5) Removal of LuaPlayer.cursor_position for unknown reason. So having the player click with some fake/invisible placeable item/capsule is the only option left sometimes.

1) That's in the same vein as the different fluid properties being removed for the faster algorithm in 0.17. I dislike both of those removals. But at least the power flow limit can be "emulated" by changing the buffer size.
2) They didnt do anything. What's the problem with removing them considering that they did nothing?
4) The proposal was made due to potential performance improvements, not "because modders dont use it".
5) It's not deterministic. The cursor position isn't directly sent over the network, that would murder it.

1) No, it can not be meaningfully emulated if i need to know how much energy was produced/consumed. An EEI does not remember how much energy it has produced/consumed (and "store more data in entities" requests are usually declined so i haven't tried...), so in order to have lazy on_tick handlers that don't check every single tick the buffer size has to be larger than one tick, which makes the power graph a jaggy mess without runtime control of flow_limit.
2) As far as i remember in old versions they *did* prevent trains from being placed or driveing onto wrong rails, much like assembler recipe categories work now.
4) "Current mod usage" was part of the reasoning:

• There is not much support, usage or interactions for temperature in the game.

What are your thoughts?
Do you do something super interesting that is only possible with temperature?
Would it be a huge loss if we removed temperature?
If it were to be removed, would you request some functionality in its place?
Do you have any differing suggestions for the fluid and temperature systems?

5) Hm...the old api doc does indeed say "write-only", must have remembered that wrong.

... This i percieve to be part of a general tendency to cut edge cases to make it technically impossible for mods to do anything that might be remotely "unsafe" or that you personally don't like or understand - what i previously called "dev over-control syndrome". ...

You're not wrong. That is my goal: to remove anything remotely unsafe and make it impossible for mods to cause issues. It's not always possible but that is the goal.

I come from a background of playing with Minecraft mods and talking with the mod authors about the horrible issues they have because of other mods doing things wrong/stupidly and I don't want to stick that on anyone else.

As in real life "security" is the opposite of "freedom". It's a gradient with pros and cons on both sides.

Give a good reason to keep script.raise_event - the factorio devs consider it as creating more problems than it solves, and does not lead to worthwhile functionality. Currently i agree with this assessment.

I think the issue of renaming script.raise_event to script.raise_some_event_name is completely different. If @Rsed wants to limit which events are raisable and the required validators somehow pop into existance then scrip.raise_event can simply raise an error if someone tries to raise an unraisable event. This makes for nicer mod code due to reusability of defines.events.foobar and doesn't clutter the api doc with descriptions for every single raiser function. Also while there have been a few mentions of mods that raise events wrong, nobody seems to think about mods that raise events *right*? Keeping the api name would mean that not-broken mods continue to work without a change if the events they raise are still supported.

[PyroFire]

I think the issue of renaming script.raise_event to script.raise_some_event_name is completely different. If @Rsed wants to limit which events are raisable and the required validators somehow pop into existance then scrip.raise_event can simply raise an error if someone tries to raise an unraisable event. This makes for nicer mod code due to reusability of defines.events.foobar and doesn't clutter the api doc with descriptions for every single raiser function. Also while there have been a few mentions of mods that raise events wrong, nobody seems to think about mods that raise events *right*? Keeping the api name would mean that not-broken mods continue to work without a change if the events they raise are still supported.

But this still leaves open the issues which the changes are intended to resolve, and, as previously explained,

Making sweeping changes that allow the 1% to continue to write bad mods makes no sense to me.

Unless you can name an event and give a purpose or reason for needing script.raise_event, i doubt any of them will actually be kept because it seems like the devs are already intent on removing them or changing how it works because, as it has been stated,

The more I think about it the more I think it just doesn't make sense to me for mods to really ever raise game events.

@Rseding i hope i have given you at least something of a reason why a few of the events should be raised by a mod, at least under some circumstances.

[mrvn]

SUMMARY

Assuming the following additions/changes are made to existing functionality:
- addition of script_raised_set_tile event
- "source" parameter added to script_raised_* credit a script raised action to something.
- "was_mined" parameter added to script_raised_destroy to flag a destroy action as having been mined or not.

Only the following events need to be changed:

-- Simulation events: 4 --
https://lua-api.factorio.com/latest/eve ... afted_item
https://lua-api.factorio.com/latest/eve ... ted_entity
https://lua-api.factorio.com/latest/eve ... ed_capsule
https://lua-api.factorio.com/latest/eve ... ransferred

-- Script raised events: 4 --
https://lua-api.factorio.com/latest/eve ... ised_built
https://lua-api.factorio.com/latest/eve ... ed_destroy
https://lua-api.factorio.com/latest/eve ... sed_revive
https://lua-api.factorio.com/latest/eve ... _set_tiles << please?

-- General purpose events: 4 --
https://lua-api.factorio.com/latest/eve ... base_built
https://lua-api.factorio.com/latest/eve ... _purchased
https://lua-api.factorio.com/latest/eve ... gs_changed
https://lua-api.factorio.com/latest/eve ... nged_state


Total validation functions needed: 12.
Now that sounds like a far more humanly-doable number of functions to write (and get it correct the first time) than 144.
Thoughts/ideas?

That was a deep look at the events. But what remains makes me wonder: Why doesn't the game raise the events already?

Any action taking by scripts that would normally raise an event when done by the player or C++ code should just raise the event when done by mods. Drop all the "script_raised_*" stuff. If knowing something was done by script is desired (and yes, it is) then add a field to the events "bool script_raised" or more flexible "string mod_name", which would be nil when not mod raised. If doing things without raising the events is desired/necessary then add an option to the action (e.g. create_entity) to be quiet.

So if some script like bluebuilt revives a ghost the C++ code automatically emits the right event. No need for the mod to do anything and therefore no need to have the raise_event with complex parameter checking. The cost for making that work right would be the addition of script_raised/mod_name to each event on the c++ side. Would that be easier?