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

[eradicator]

Btw, here's another usecase for manually raising on_build: If i use create_entity{raise_build=true} then another mod could destroy the created entity before i even ever see it. Therefor

Code: Select all

function()
create_entity{raise_build=false}
--dostuff
script.raise_event(defines.events.on_built,...)
end

means i have to care about less shit that other mods might do to my entities.

Why bother wasting computing time configuring an entity that's just going to be destroyed?

Destruction is only one of the many examples of how another mod could change the entity in ways that are incompatible with what i want to do with it, before i get to touch it. And ofc i don't know if or how other mods have changed it, so i'd have to check every property of *my* newly created entities as if they were foreign objects, just on the off-chance that some other broken mod fucks them up.

Want to spawn an assembler with modules? Better clear the module inventory first.
Want to spawn an assembler with a recipe? Better make sure the in/output inventories are empty.
Etcpp.

Not being able to rely on newly created entities being actually fresh and untouched makes any further manipulation a potential error source.

[mrvn]

You say you can check the arguments for all the special raise_X() functions you consider valid for scripts to raise.

So what is stopping you from simply doing those checks in raise_event() using a simple

Code: Select all

switch(event_type) {
    case ENTITY_BUILT: ... break;
    ...
    default: if (event_type < USER_EVENT) raise NotAllowedToRaiseEvent(event_:type);
}

Same gain in protection against garbage without creating a new API.

[Rseding91]

You say you can check the arguments for all the special raise_X() functions you consider valid for scripts to raise.

So what is stopping you from simply doing those checks in raise_event() using a simple

Code: Select all

switch(event_type) {
    case ENTITY_BUILT: ... break;
    ...
    default: if (event_type < USER_EVENT) raise NotAllowedToRaiseEvent(event_:type);
}

Same gain in protection against garbage without creating a new API.

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.

As I said before; the more I think about this the more it makes sense to just disable mods from ever calling game events and being done with it. It shouldn't have existed in the first place.

[eradicator]

As I said before; the more I think about this the more it makes sense to just disable mods from ever calling game events and being done with it. It shouldn't have existed in the first place.

The more i hear you say this the more i get the feeling this was never "opinions wanted".

[Boodals]

The way I see it, any mod may want to simulate an action that normally would raise an event, for example Bluebuild raises the player_built event. script_raised_built just wouldn't work for it because there is no proper way to pass the player, item stack, etc. In my mod, I have (planned) a scripted radar that scans another surface, so I'd need to raise the on_chunk_charted event. The same could happen with just about any of the other events.

Unless you are going to add script_raised events (or the equivalent raise_X function) for just about every event that exists, then this change will kill some mods, and prevent many from having proper mod compatibility. But at that point, just use the same event so modders don't have to register to the game event and the script_raise event.
So, make a proper system which designates what properties of events are optional, then in raise_event check that it has at least all of the mandatory variables.

• No mods will break (unless they are already broken by incorrectly raising events without all the mandatory variables).
• You can filter out invalid entities and whatnot (or just throw an error).
• Extra variables can still be added to events so mods can identify specific events from other mods (that is a feature that many mods rely on, do not remove it).

As a plus you can now be sure that any variable marked as mandatory will always be populated in C++, and the API documentation can use the same data to always be up to date and correct.
It'll be a bit of work to be sure, and I don't know the systems well enough to know how big of a change it'll be, but compared to the stuff I've seen you code, it shouldn't be too bad.

[Rseding91]

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.

[Bilka]

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.

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.

[Rseding91]

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.

[quyxkh]

How about setting a "mod_raised" entry in event tables passed by mods, the name of the raising mod? Mod authors could then cheaply blacklist or whitelist or whatever to avoid trouble.

[Rseding91]

How about setting a "mod_raised" entry in event tables passed by mods, the name of the raising mod? Mod authors could then cheaply blacklist or whitelist or whatever to avoid trouble.

If we (Wube) end up doing anything it really only makes sense to make changes that are going to benefit the 99%: the people who aren't passing garbage to events but just listening to game events.

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

[pleegwat]

How about, for every default event that a mod might also send, you add a second mod-only event. Then a receiving mod can pick if they want to support (and validate) mod-generated events, or ignore them and just handle (guaranteed valid) default events.

[eradicator]

How about setting a "mod_raised" entry in event tables passed by mods, the name of the raising mod? Mod authors could then cheaply blacklist or whitelist or whatever to avoid trouble.

If we (Wube) end up doing anything it really only makes sense to make changes that are going to benefit the 99%: the people who aren't passing garbage to events but just listening to game events.

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

By that logic it would be sufficient to add script.on_script_raised_event, so that the normal script.on_event doesn't see anything raised by script.raise_event.

Honestly i can't believe the "12000 lines of code" story though. What exactly is there that makes it so huge to check. I mean the c code obviously doesn't care or it would never have worked? If i had to implement it on my mod side i'd just make a list that for every event contains the key names, and the expected type for each value. I mean, 95% of all event content is LuaEntity, LuaGuiElement, position or player_index...so what is it that makes this so expensive to do?

Also i still think you're overestimating how many people will ever use something minor as "event filters for script_raised_built". 99% of tutorials won't mention it. 100% of mods that have more than one event handler for the event can't meaningfully use it (this includes anything using stdlib). 99% of mods don't even have event handlers because all they do is data stage.

[justarandomgeek]

100% of mods that have more than one event handler for the event can't meaningfully use it (this includes anything using stdlib).

Sounds like someone shoehorning multiple mods into a single package, rather than actually separating them properly!

[eradicator]

100% of mods that have more than one event handler for the event can't meaningfully use it (this includes anything using stdlib).

Sounds like someone shoehorning multiple mods into a single package, rather than actually separating them properly!

Sounds like "i don't have a use case for it so i'm against it".

[Boodals]

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.

All you need to check is: whether a given variable is missing when its not optional, the lua type, and sometimes the prototype type. That to me sounds like one line of code for each variable in each event, and no more than 1000 lines for the system which handles it all.

[mrvn]

You say you can check the arguments for all the special raise_X() functions you consider valid for scripts to raise.

So what is stopping you from simply doing those checks in raise_event() using a simple

Code: Select all

switch(event_type) {
    case ENTITY_BUILT: ... break;
    ...
    default: if (event_type < USER_EVENT) raise NotAllowedToRaiseEvent(event_:type);
}

Same gain in protection against garbage without creating a new API.

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.

As I said before; the more I think about this the more it makes sense to just disable mods from ever calling game events and being done with it. It shouldn't have existed in the first place.

Sorry, but that just makes no sense at all.

If the LUA binding for raise_X can check the arguments passed in then the binding for raise_event can also check them just the same. Naming the function something different in no way changes what you can and can not check.

As for not passing in extra data:

1) That would break for example blueprints for Deadlocks Beltboxes because a handful of mods couldn't pass in extra args. Which is kind of a hack but necessary because the game does not allow the user to define weather a placed underground belt or loader has input or output type. The extra arg set by mods tells the beltboxes not to adjust the input/output type to match the belts and inventories around it. Otherwise you couldn't blueprint a beltbox that has been disabled by being turned around.

Note: placing a loader over a ghost also ignores the input/output configuration of the ghost. Like assembler keeping the recipes the loaders should keep the input/output type.

2) The function gets arguments passed in, either separately or a table of key:value pairs. What is stopping you from only copying known keys from the functions arguments into the event you pass back to the event handler?

As said, that excuse just makes no sense at all. If you say "it's hard" I can see that. If you say "it's too much work" I can see that too. If you say "I simply don't like it so I won't do it" I can see that too. But saying it can't be done is plain wrong.

[justarandomgeek]

Sounds like we've got a few volunteers to write all the checks

[jan1i3]

Looks like the the best solution would be to validate every events data.
issue: implementation.

i don't know if you (WUBE) would accept this being like a community effort of a couple of people who are willing to implement this for you (if they exist, even)
if yes, then that would be a possible solution.
(I can see several issues with this, no need to go over that right now tho)


alternative:
mods cannot raise game events.
however if needed they can still communicate with each other explicitly using custom events.
that way mods can still be compatible with each other.

issue:
mods can no longer simulate game events, even if they would do it properly.
but this is extremely rare (i would think, and it seems like)
in those cases it would make more sense for those modders to ask for specific api changes, for example:
when charting a chunk, optional parameter on that function to trigger the on_chunk_charted event.
(not requesting custom raise_X if possible, tho that is a possibility. I just don't like the inconsistency behind that solution)


issue that exist regardless:
when a mod places an entity, and with it raises the on_script_built event, now any mod that modifies entities when they are built could break mods that are simply building entities and doing something with them immediatelly after. neither of the mods need to be doing something wrong.

idea:
change the behaviour of on_script_<something>, so that it happens after the mod causing it is done for this tick... or done for now, or however you want to call it. (that mods code is currently not being executed anymore)
can you, or how to detect that?
well, that depends on the current implementation of several things, that i don't know.
but I feel like (and hope) this would be possible.



i personally would be fine with the 'alternative' solution - yes it can make some things simply impossible, but so be it... and it's rare!
and if that idea about on_script_ stuff is possible, then these solutions would at the very least keep everything mentioned in this thread so far possible (i think)

[mrvn]

Sounds like we've got a few volunteers to write all the checks

Give me all the raise_X functions and I will happily put a "switch(event->type)" around it for you. That takes like 5 minutes to cut&paste.

[KUDr]

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.